Detect when `'static` obligation might come from an `impl` #73783

estebank · 2020-06-27T01:53:52Z

Partly address #71341.

rust-highfive · 2020-06-27T01:53:55Z

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

src/librustc_infer/infer/error_reporting/nice_region_error/static_impl_trait.rs

estebank · 2020-06-27T02:00:23Z

r? @Aaron1011

Aaron1011 · 2020-06-27T02:54:37Z

src/librustc_infer/infer/error_reporting/nice_region_error/static_impl_trait.rs

It would be nice for DiagnosticBuilder to have a concept of 'ordered diagnostics', as there are lots of other places where this kind of logic would be useful. However, it doesn't have to block this PR.

I had a proposal around this at some point. I wanted to change the rendering if the diagnostics would not display in source order.

src/test/ui/associated-types/cache/project-fn-ret-invariant.transmute.stderr

src/librustc_infer/infer/error_reporting/nice_region_error/static_impl_trait.rs

src/librustc_typeck/check/method/confirm.rs

estebank · 2020-07-01T04:46:34Z

src/librustc_infer/infer/error_reporting/nice_region_error/static_impl_trait.rs

Just realized this is wrong, this is meant to check the self_ty instead. It happens to work in the test because it is the only impl.

I'm using an alternative guessing method, but eventually we would want to do the check between self_ty and the original method receiver ty, but I always find it tricky to correctly compare hir::Tys and ty::Tys.

Aaron1011 · 2020-07-01T06:54:41Z

src/librustc_infer/infer/error_reporting/nice_region_error/static_impl_trait.rs

We almost have enough information to do this for foreign crates - we can get the method ident (through AssocItem), and the overall impl span, but not the span of the self type. I think it's fine to just open an issue about extending this error message, though.

For foreign crates we might want to just say "the impl in another crate causes the 'static requirement, deal with it or bother their author 🤷‍♂️" 😬

estebank · 2020-07-01T07:27:46Z

@Aaron1011 are there any items left to tackle?

estebank · 2020-07-01T15:39:04Z

(Changing the labels as I presume they were changed due to #73783 (comment). The new heuristic isn't perfect, but should work well enough.)

Aaron1011 · 2020-07-01T17:46:24Z

src/librustc_infer/infer/error_reporting/nice_region_error/static_impl_trait.rs

+                let parent_id = tcx.hir().get_parent_item(*hir_id);
+                match tcx.hir().find(parent_id) {
+                    Some(Node::Item(Item { kind: ItemKind::Trait(..), .. })) => {
+                        // The method being called is defined in the `trait`, but the `'static`


If I understand correctly, this is handling a case like:

trait OtherTrait {} trait Foo { fn bar(&self) {} } impl Foo for dyn OtherTrait {}

where we're using the default implementation from the trait?

I think we can remove the need for this by extending Instance::resolve to provide information about the parent impl item (even when the method implementation) comes from the trait itself
). Could you open an issue to track changing this?

I just reused this method for the Trait::method(binding) case, but I'll look into modifying Instance::resolve regardless.

I did a quick test modifying Instance::resolve and I caused a bunch of unrelated ICEs. I'll do that in a follow up PR.

I'm fine with merging this as-is, unless there are other changes that you'd like to make.

I'm satisfied with the cases I'm handling. I'll revisit this in the future for further improvement but I think can be merged. (Of courses take a last review pass to make sure I didn't introduce anything by accident.)

src/librustc_infer/infer/error_reporting/nice_region_error/static_impl_trait.rs

estebank · 2020-07-21T18:22:02Z

@bors r=nikomatsakis

bors · 2020-07-21T18:22:03Z

📌 Commit 7b05fb5 has been approved by nikomatsakis

…time, r=nikomatsakis Detect when `'static` obligation might come from an `impl` Partly address rust-lang#71341.

Manishearth · 2020-07-22T16:28:24Z

@bors r-

#74638 (comment)

Address rust-lang#71341.

estebank · 2020-07-22T20:21:28Z

@bors r=nikomatsakis

bors · 2020-07-22T20:21:29Z

📌 Commit 889a4d9 has been approved by nikomatsakis

bors · 2020-07-22T20:21:30Z

🌲 The tree is currently closed for pull requests below priority 5, this pull request will be tested once the tree is reopened

@ghost

…arth Rollup of 9 pull requests Successful merges: - rust-lang#73783 (Detect when `'static` obligation might come from an `impl`) - rust-lang#73868 (Advertise correct stable version for const control flow) - rust-lang#74460 (rustdoc: Always warn when linking from public to private items) - rust-lang#74538 (Guard against non-monomorphized type_id intrinsic call) - rust-lang#74541 (Add the aarch64-apple-darwin target ) - rust-lang#74600 (Enable perf try builder) - rust-lang#74618 (Do not ICE on assoc type with bad placeholder) - rust-lang#74631 (rustc_target: Add a target spec option for disabling `--eh-frame-hdr`) - rust-lang#74643 (build: Remove unnecessary `cargo:rerun-if-env-changed` annotations) Failed merges: r? @ghost

rust-highfive assigned eddyb Jun 27, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 27, 2020

estebank mentioned this pull request Jun 27, 2020

Incomprehensible error message involving lifetimes adding extension methods to dyn Trait #71341

Closed

estebank commented Jun 27, 2020

View reviewed changes

src/librustc_infer/infer/error_reporting/nice_region_error/static_impl_trait.rs Outdated Show resolved Hide resolved

rust-highfive assigned Aaron1011 and unassigned eddyb Jun 27, 2020

Aaron1011 reviewed Jun 27, 2020

View reviewed changes

src/test/ui/associated-types/cache/project-fn-ret-invariant.transmute.stderr Outdated Show resolved Hide resolved

Aaron1011 reviewed Jun 27, 2020

View reviewed changes

src/librustc_infer/infer/error_reporting/nice_region_error/static_impl_trait.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

estebank force-pushed the impl-dyn-trait-static-lifetime branch 3 times, most recently from 05475b4 to bdb39ab Compare June 28, 2020 23:17

Aaron1011 suggested changes Jun 29, 2020

View reviewed changes

src/librustc_typeck/check/method/confirm.rs Outdated Show resolved Hide resolved

estebank commented Jul 1, 2020

View reviewed changes

Aaron1011 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 1, 2020

Aaron1011 reviewed Jul 1, 2020

View reviewed changes

estebank force-pushed the impl-dyn-trait-static-lifetime branch from 48f9664 to 5136013 Compare July 1, 2020 07:25

estebank added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 1, 2020

Aaron1011 reviewed Jul 1, 2020

View reviewed changes

src/librustc_infer/infer/error_reporting/nice_region_error/static_impl_trait.rs Show resolved Hide resolved

Aaron1011 reviewed Jul 1, 2020

View reviewed changes

src/librustc_infer/infer/error_reporting/nice_region_error/static_impl_trait.rs Outdated Show resolved Hide resolved

Aaron1011 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 1, 2020

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 21, 2020

Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 22, 2020

Rollup merge of rust-lang#73783 - estebank:impl-dyn-trait-static-life…

763d019

…time, r=nikomatsakis Detect when `'static` obligation might come from an `impl` Partly address rust-lang#71341.

Manishearth mentioned this pull request Jul 22, 2020

Rollup of 10 pull requests #74638

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 22, 2020

estebank added 8 commits July 22, 2020 12:25

Detect when 'static obligation might come from an impl

6513148

Address rust-lang#71341.

Increase accuracy of lifetime bound on trait object impl suggestion

4e08bab

Add more context to diagnostic

6bac3db

Further tweak wording of E0759 and introduce E0767

7bf39fa

Use ty::Instance::resolve to identify 'static bound source

f807437

Partially account for case where used method is from trait

3712dfc

Handle fully-qualified paths and add test cases

53d96b5

Change error code number

889a4d9

estebank force-pushed the impl-dyn-trait-static-lifetime branch from 7b05fb5 to 889a4d9 Compare July 22, 2020 20:12

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 22, 2020

Manishearth mentioned this pull request Jul 22, 2020

Rollup of 9 pull requests #74662

Merged

bors merged commit fe9babb into rust-lang:master Jul 23, 2020

JohnTitor mentioned this pull request Oct 23, 2020

Do not try to report on closures to avoid ICE #78268

Merged

cuviper added this to the 1.47.0 milestone May 2, 2024

Detect when 'static obligation might come from an impl #73783

Detect when 'static obligation might come from an impl #73783

Uh oh!

Conversation

estebank commented Jun 27, 2020

Uh oh!

rust-highfive commented Jun 27, 2020

Uh oh!

Uh oh!

estebank commented Jun 27, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

estebank commented Jul 1, 2020

Uh oh!

estebank commented Jul 1, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

estebank commented Jul 21, 2020

Uh oh!

bors commented Jul 21, 2020

Uh oh!

Manishearth commented Jul 22, 2020

Uh oh!

estebank commented Jul 22, 2020

Uh oh!

bors commented Jul 22, 2020

Uh oh!

bors commented Jul 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Detect when `'static` obligation might come from an `impl` #73783

Detect when `'static` obligation might come from an `impl` #73783