Point at capture points for non-`'static` reference crossing a `yield` point #89734

estebank · 2021-10-10T10:39:23Z

error[E0759]: `self` has an anonymous lifetime `'_` but it needs to satisfy a `'static` lifetime requirement
  --> $DIR/issue-72312.rs:10:24
   |
LL |     pub async fn start(&self) {
   |                        ^^^^^ this data with an anonymous lifetime `'_`...
...
LL |         require_static(async move {
   |         -------------- ...is required to live as long as `'static` here...
LL |             &self;
   |             ----- ...and is captured here
   |
note: `'static` lifetime requirement introduced by this trait bound
  --> $DIR/issue-72312.rs:2:22
   |
LL | fn require_static<T: 'static>(val: T) -> T {
   |                      ^^^^^^^

error: aborting due to previous error

For more information about this error, try `rustc --explain E0759`.

Fix #72312.

rust-highfive · 2021-10-10T10:39:25Z

r? @petrochenkov

(rust-highfive has picked a reviewer for you, use r? to override)

estebank · 2021-10-10T10:42:49Z

src/test/ui/traits/trait-upcasting/type-checking-test-4.stderr

+error[E0759]: `x` has lifetime `'a` but it needs to satisfy a `'static` lifetime requirement
+  --> $DIR/type-checking-test-4.rs:43:27
+   |
+LL | fn test_wrong6<'a>(x: &dyn Foo<'a>) -> Option<&'static u32> {
+   |                       ------------ this data with lifetime `'a`...
+LL |     let y = x as &dyn Bar<'_, '_>;
+   |             -             ^^
+   |             |
+   |             ...is captured here...
+LL |
+LL |     y.get_b(); // ERROR
+   |     - ...and is captured here too
+LL |     let z = y;
+LL |     z.get_b() // ERROR
+   |     --------- ...is required to live as long as `'static` here...
+   |
+note: ...and is captured here too
+  --> $DIR/type-checking-test-4.rs:47:5
+   |
+LL |     z.get_b() // ERROR
+   |     ^


This case exemplifies that more Span wrangling might be needed for perfect output, but the bulk of the logic is ready for review.

why does this output not have the note for where the 'static requirement was introduced like the one in the PR description does?

Because the check for it is quite conservative. I'll see what this case is instead.

Edit: all of these are Subtype coercion errors, instead of RelateParamBound:

DEBUG rustc_infer::infer::error_reporting::nice_region_error::static_impl_trait try_report_static_impl_trait(var=EarlyBoundRegion(src/test/ui/traits/trait-upcasting/type-checking-test-4.rs:43:27: 43:29 (#0), "'a"), -> sub=Subtype(TypeTrace { cause: ObligationCauseData { span: src/test/ui/traits/trait-upcasting/type-checking-test-4.rs:47:5: 47:14 (#0), body_id: HirId { owner: DefId(0:20 ~ type_checking_test_4[7abc]::test_wrong6), local_id: 32 }, code: BlockTailExpression(HirId { owner: DefId(0:20 ~ type_checking_test_4[7abc]::test_wrong6), local_id: 31 }) }, values: Types(ExpectedFound { expected: std::option::Option<&'static u32>, found: std::option::Option<&u32> }) }) ReStatic sup=Subtype(TypeTrace { cause: ObligationCauseData { span: src/test/ui/traits/trait-upcasting/type-checking-test-4.rs:43:13: 43:14 (#0), body_id: HirId { owner: DefId(0:20 ~ type_checking_test_4[7abc]::test_wrong6), local_id: 32 }, code: ImplDerivedObligation(DerivedObligationCause { parent_trait_ref: Binder(<&dyn Foo<'a> as std::ops::CoerceUnsized<_>>, []), parent_code: Coercion { source: &dyn Foo<'a>, target: &dyn Bar<'_, '_> } }) }, values: Types(ExpectedFound { expected: dyn Bar<'_, '_>, found: dyn Bar<'a, 'a> }) }) ReFree(DefId(0:20 ~ type_checking_test_4[7abc]::test_wrong6), BrNamed(DefId(0:21 ~ type_checking_test_4[7abc]::test_wrong6::'a), 'a)))

Because the ObligationCauseCode is BlockTailExpression, we could safely point at the return type always with a similar label 🤔

Can you check the last commit @BoxyUwU? I think it should address your requirements.

Ys i like the note! cool :) not gonna mark this review as revolved though because I seem to have taken over you talking about spans... oops

src/test/ui/async-await/issues/issue-72312.rs

petrochenkov · 2021-10-10T13:57:23Z

r? @BoxyUwU @lqd

tmandry · 2021-10-12T01:08:48Z

compiler/rustc_infer/src/infer/lexical_region_resolve/mod.rs

+            if let (Constraint::VarSubVar(_, _), SubregionOrigin::DataBorrowed(_, sp)) =
+                (constraint, origin)
+            {
+                captures.push(*sp);


This doesn't seem to have specificity to the particular variable this constraint references, and I don't see any filtering on the reporting side. What if these captures were associated with multiple region errors?

Also, this code runs in the happy path. Would it make sense to collect these capture regions directly in collect_error_for_expanding_node?

What if these captures were associated with multiple region errors?

I mentioned that in the ticket, I'm not entirely sure about how to perform that filtering correctly (I'm not familiar with this part of inference).

The last commit now performs basic filtering based on where the error happened, but it's very naïve (and even then it already changed the output of one test).

compiler/rustc_infer/src/infer/error_reporting/nice_region_error/static_impl_trait.rs

compiler/rustc_infer/src/infer/lexical_region_resolve/mod.rs

compiler/rustc_infer/src/infer/error_reporting/nice_region_error/static_impl_trait.rs

estebank · 2021-10-13T14:39:56Z

src/test/ui/async-await/issues/issue-72312.nll.stderr

+error[E0521]: borrowed data escapes outside of associated function
+  --> $DIR/issue-72312.rs:13:24
+   |
+LL |       pub async fn start(&self) {
+   |                          -----
+   |                          |
+   |                          `self` is a reference that is only valid in the associated function body
+   |                          let's call the lifetime of this reference `'1`
+...
+LL |           require_static(async move {
+   |  ________________________^
+LL | |             &self;
+LL | |         });
+   | |         ^
+   | |         |
+   | |_________`self` escapes the associated function body here
+   |           argument requires that `'1` must outlive `'static`


Hey @oli-obk heads up: would any of your ongoing PRs affect this output?

#89229 can affect similar situations, not sure if it affects this very case.

Do you think it'll be feasible to get this output with nll?

The only missing thing would be to point at the source of the 'static bound, right? I think we can do that

The interesting thing would be to point at the self inside the async block, because if you didn't have that there this could would otherwise work (but we already mention it in the label, so that's fine). Pointing at the 'static bound would be more important, yes.

Ah yes, for that I think we need to start tracking some extra information in a separate table in borrowck.

BoxyUwU · 2021-10-15T17:57:37Z

I dont know this code so can't really review
r? @lqd
since you were also r? alongside me

estebank · 2021-10-15T18:17:06Z

I had a 1:1 with Niko where he had some thoughts, so I'll dump the responsibility on him :)

r? @nikomatsakis

nikomatsakis

This looks pretty good, left some naming nits @estebank

nikomatsakis · 2021-11-08T17:01:00Z

compiler/rustc_infer/src/infer/error_reporting/nice_region_error/static_impl_trait.rs

+            //    |                               ^^^^^
+            //    |                               |
+            //    |                               this data with an anonymous lifetime `'_`...
+            //    |                               ...is captured here...


I find this comment rather mystifying. What is about the desugaring that causes that, for example? And what will we do with these variables?

nikomatsakis · 2021-11-08T17:01:21Z

compiler/rustc_infer/src/infer/error_reporting/nice_region_error/static_impl_trait.rs

+        } else {
+            (!sup_origin.span().overlaps(return_sp), param.param_ty_span)
+        };
+        err.span_label(capture_point, &format!("this data with {}...", lifetime));


Maybe move this above that big scary if ..?

Moving it before won't work because that if affects capture_point as well.

compiler/rustc_infer/src/infer/error_reporting/nice_region_error/static_impl_trait.rs

compiler/rustc_infer/src/infer/lexical_region_resolve/mod.rs

nikomatsakis · 2021-11-11T15:57:46Z

compiler/rustc_infer/src/infer/lexical_region_resolve/mod.rs

@@ -567,7 +569,29 @@ impl<'cx, 'tcx> LexicalResolver<'cx, 'tcx> {
                    // if this rule starts to create problems we'll
                    // have to revisit this portion of the code and
                    // think hard about it. =) -- nikomatsakis
-                    self.collect_error_for_expanding_node(graph, &mut dup_vec, node_vid, errors);
+
+                    // Obtain the spans for all the capture points for


I don't think we should call these captures -- but I see what you're doing. You're basically finding, for the variable X that failed, each constraint X: Y -- i.e., all variables Y that X flows into. I can see why you might call those "captures", but to me that calls to me upvar capture in closures. I'm thinking about other names:

predecessors: in our graph, a <: b (i.e., b: a) implies an edge a -> b. I don't like this name, it's too confusing which is successor and predecessor (and different parts of the code use distinct conventions).

dataflow_targets: you could say that Y is a "dataflow target" of X, which is a term I just made up, but the point is that data from X flows into Y. But we're not capturing the target, we're capturing the constraint.

influences: these are the constraints that influence the value chosen for X.

inputs: as above, but even more generic

Thoughts?

I like influences.

nikomatsakis · 2021-11-11T16:00:49Z

compiler/rustc_infer/src/infer/lexical_region_resolve/mod.rs

@@ -96,6 +97,7 @@ pub enum RegionResolutionError<'tcx> {
        Region<'tcx>,
        SubregionOrigin<'tcx>,
        Region<'tcx>,
+        Vec<Span>,


needs a comment!

are these the "spans" associated with "influencer constraints"?

nikomatsakis · 2021-11-18T18:24:35Z

@estebank took a quick look, but I think you're still planning more changes, right?

estebank · 2021-11-20T19:40:53Z

but I think you're still planning more changes

I believe the only unaddressed comment is the "mystifying comment". I added that comment to document what the output would be if we didn't do what we're doing there. I don't know how to better explain it, other than outright remove it, and then it will be confusing on why the check is even there 🤔

nikomatsakis

r=me modulo naming nit

nikomatsakis · 2021-12-09T22:05:05Z

compiler/rustc_infer/src/infer/error_reporting/nice_region_error/static_impl_trait.rs

@@ -123,56 +125,100 @@ impl<'a, 'tcx> NiceRegionError<'a, 'tcx> {
            param_name,
            lifetime,
        );
-        err.span_label(param.param_ty_span, &format!("this data with {}...", lifetime));
+
+        let (mention_capture, capture_point) = if sup_origin.span().overlaps(param.param_ty_span) {


Suggested change

let (mention_capture, capture_point) = if sup_origin.span().overlaps(param.param_ty_span) {

let (mention_influencer, influencer_point = if sup_origin.span().overlaps(param.param_ty_span) {

?

…` point ``` error[E0759]: `self` has an anonymous lifetime `'_` but it needs to satisfy a `'static` lifetime requirement --> $DIR/issue-72312.rs:10:24 | LL | pub async fn start(&self) { | ^^^^^ this data with an anonymous lifetime `'_`... ... LL | require_static(async move { | -------------- ...is required to live as long as `'static` here... LL | &self; | ----- ...and is captured here | note: `'static` lifetime requirement introduced by this trait bound --> $DIR/issue-72312.rs:2:22 | LL | fn require_static<T: 'static>(val: T) -> T { | ^^^^^^^ error: aborting due to previous error For more information about this error, try `rustc --explain E0759`. ``` Fix rust-lang#72312.

* take diagnostic logic out of happy-path * sort/dedup once * add more comments

estebank · 2021-12-10T03:19:23Z

@bors r=nikomatsakis

bors · 2021-12-10T03:19:24Z

📌 Commit 40f161a has been approved by nikomatsakis

estebank · 2021-12-10T17:22:53Z

@bors r=nikomatsakis

bors · 2021-12-10T17:22:55Z

📌 Commit d2d9eb3 has been approved by nikomatsakis

bors · 2021-12-10T17:22:56Z

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

Point at capture points for non-`'static` reference crossing a `yield` point ``` error[E0759]: `self` has an anonymous lifetime `'_` but it needs to satisfy a `'static` lifetime requirement --> $DIR/issue-72312.rs:10:24 | LL | pub async fn start(&self) { | ^^^^^ this data with an anonymous lifetime `'_`... ... LL | require_static(async move { | -------------- ...is required to live as long as `'static` here... LL | &self; | ----- ...and is captured here | note: `'static` lifetime requirement introduced by this trait bound --> $DIR/issue-72312.rs:2:22 | LL | fn require_static<T: 'static>(val: T) -> T { | ^^^^^^^ error: aborting due to previous error For more information about this error, try `rustc --explain E0759`. ``` Fix rust-lang#72312.

…askrgr Rollup of 6 pull requests Successful merges: - rust-lang#83174 (Suggest using a temporary variable to fix borrowck errors) - rust-lang#89734 (Point at capture points for non-`'static` reference crossing a `yield` point) - rust-lang#90270 (Make `Borrow` and `BorrowMut` impls `const`) - rust-lang#90741 (Const `Option::cloned`) - rust-lang#91548 (Add spin_loop hint for RISC-V architecture) - rust-lang#91721 (Minor improvements to `future::join!`'s implementation) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rust-highfive assigned petrochenkov Oct 10, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 10, 2021

estebank commented Oct 10, 2021

View reviewed changes

estebank force-pushed the issue-72312 branch from 18b1795 to 1d70ec9 Compare October 10, 2021 10:44

lqd reviewed Oct 10, 2021

View reviewed changes

src/test/ui/async-await/issues/issue-72312.rs Outdated Show resolved Hide resolved

rust-highfive assigned BoxyUwU and unassigned petrochenkov Oct 10, 2021

tmandry reviewed Oct 12, 2021

View reviewed changes

estebank force-pushed the issue-72312 branch from 1ac4b30 to c83782f Compare October 12, 2021 13:28

estebank commented Oct 13, 2021

View reviewed changes

apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 14, 2021

estebank force-pushed the issue-72312 branch from c83782f to 6ba48d0 Compare October 15, 2021 15:13

rust-highfive assigned lqd and unassigned BoxyUwU Oct 15, 2021

rust-highfive assigned nikomatsakis and unassigned lqd Oct 15, 2021

This comment has been minimized.

Sign in to view

estebank force-pushed the issue-72312 branch from 6ba48d0 to 3e17a3a Compare October 28, 2021 19:48

nikomatsakis reviewed Nov 11, 2021

View reviewed changes

estebank force-pushed the issue-72312 branch from 3e17a3a to 87df844 Compare November 16, 2021 22:59

nikomatsakis approved these changes Dec 9, 2021

View reviewed changes

estebank added 3 commits December 10, 2021 03:08

Clean up visual output logic

dd81e98

review comments

ab45ab8

* take diagnostic logic out of happy-path * sort/dedup once * add more comments

estebank added 9 commits December 10, 2021 03:08

Point at return type when it introduces 'static obligation

ee0fd10

Use a more accurate Span for 'static obligation from return type

10a74ac

Update nll test

0ee723e

rebase and update nll test

ff13ad7

Tweak wording

83ce1aa

Review comments

9cc7bd7

Remove field from ErrorValue

d33fa13

review comment

da5b0cc

fix tests after rebase

40f161a

estebank force-pushed the issue-72312 branch from 87df844 to 40f161a Compare December 10, 2021 03:19

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-review Status: Awaiting review from the assignee but also interested parties. labels Dec 10, 2021

This comment has been minimized.

Sign in to view

fmt

d2d9eb3

matthiaskrgr mentioned this pull request Dec 11, 2021

Rollup of 6 pull requests #91775

Closed

matthiaskrgr mentioned this pull request Dec 11, 2021

Rollup of 6 pull requests #91790

Closed

matthiaskrgr mentioned this pull request Dec 11, 2021

Rollup of 6 pull requests #91799

Merged

bors merged commit 7bba5c1 into rust-lang:master Dec 11, 2021

rustbot added this to the 1.59.0 milestone Dec 11, 2021

	let (mention_capture, capture_point) = if sup_origin.span().overlaps(param.param_ty_span) {
	let (mention_influencer, influencer_point = if sup_origin.span().overlaps(param.param_ty_span) {

Point at capture points for non-'static reference crossing a yield point #89734

Point at capture points for non-'static reference crossing a yield point #89734

Conversation

estebank commented Oct 10, 2021 • edited Loading

rust-highfive commented Oct 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estebank Oct 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrochenkov commented Oct 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BoxyUwU commented Oct 15, 2021

estebank commented Oct 15, 2021

This comment has been minimized.

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented Nov 18, 2021

estebank commented Nov 20, 2021

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estebank commented Dec 10, 2021

bors commented Dec 10, 2021

This comment has been minimized.

estebank commented Dec 10, 2021

bors commented Dec 10, 2021

bors commented Dec 10, 2021

Point at capture points for non-`'static` reference crossing a `yield` point #89734

Point at capture points for non-`'static` reference crossing a `yield` point #89734

estebank commented Oct 10, 2021 •

edited

Loading

estebank Oct 10, 2021 •

edited

Loading