improve lifetime annotation diagnostics #100976

aliemjay · 2022-08-24T23:48:03Z

register the lifetime annotations in closure signature and function calls under the category ConstraintCategory::TypeAnnotation.
when searching for the "best blame constraint", make sure we report type annotation constraints only if there is no other constraint to blame. Now whenever we get TypeAnnotation error, we can be sure that the lifetime annotation is unnecessarily restrictive and we can safely suggest removing it.
handle the subtle cases of Self where it forces unnecessary constraints and suggest a proper fix.

TODO:

Use of type aliases may impose additional region constraints even when neither the definition nor the use site has an explicit lifetime annotation:

type Same<X> = (X, X);
type Inv<'x> = *mut &'x ();
fn test(s1: Inv<'_>, s2: Inv<'_>) {
    let _: Same<_> = (s1, s2);
}

cc #100725, not closing it though.
resolves #100572
resolves #101393
resolves #69224
resolves #62185

r? @estebank

compiler-errors · 2022-08-25T16:55:58Z

compiler/rustc_borrowck/src/diagnostics/region_errors.rs

+    span: Span,
+) {
+    use rustc_lexer as lex;
+    fn tokenize(mut input: &str) -> impl Iterator<Item = (lex::TokenKind, &str)> {


This makes me somewhat uncomfortable..

Does it really matter? I mean it's only in the error path.

Yeah, I think it does. There are lots of ways you can get spans to point at random things that aren't paths, like macro expansions, parser error recovery paths, etc. A lot of the error handling logic we have in borrowck is super fragile, and I think invoking the lexer of all things in borrowck is suspicious. How much effort would it be to pass down something like a hir id? or a rustc_middle Ty?

I don't think passing Ty makes a difference, I'm trying to detect the use of Self here.
About the HirId, I'm more worried about the correctness of such approach, the mir/thir would reference hir and that's an abstraction leakage IMO.

Probably a better thing can be done here is to compute uses_self: bool field for each type annotation when building thir/mir. Bu this may be out of scope for this PR because I'm targeting a beta-backport. Is it ok to try this in a followup PR? or is there a chance that this gets backported anyway?

register lifetime annotations from closure signature and UFCS calls under `ConstraintCategory::TypeAnnotation` and make sure we don't report type annotation errors unless thy're the only thing to blame.

The diagnostic changes says it all :)

aliemjay · 2022-08-25T18:27:17Z

It should be ready now. Maybe best reviewed commit-by-commit. The output needs some bikeshedding though.

aliemjay · 2022-08-25T18:32:47Z

compiler/rustc_borrowck/src/diagnostics/region_errors.rs

+        let suggested_ty =
+            tcx.fold_regions(tcx.type_of(impl_did), |_, _| tcx.mk_region(ty::ReErased));
+        err.help(format!("consider replacing `Self` with `{suggested_ty}`"));


Is there a way to format the ty with a turbofish? it currently suggests MyStruct<T> instead of MyStruct::<T>.

Also is it better to replace regions with dummy ReVar instead so that it is formatted as MyStruct<'_, T> instead of MyStruct<T>? but that maybe too verbose for the common case.

I think you need something like

let mut printer = ty::print::FmtPrinter::new(self.infcx.tcx, Namespace::ValueNS); // The default is `Namespace::TypeNS ty.print(printer).unwrap().into_buffer()

jackh726 · 2022-08-25T18:34:49Z

src/test/ui/wf/wf-static-method.stderr

@@ -20,8 +20,9 @@ LL | impl<'a, 'b> Foo<'a, 'b, ()> for IndirectEvil<'a, 'b> {
   |      lifetime `'a` defined here
 ...
 LL |         let me = Self::make_me();
-   |                  ^^^^^^^^^^^^^ requires that `'b` must outlive `'a`
+   |                  ^^^^^^^^^^^^^ type annotation requires that `'b` must outlive `'a`


There is really no type annotation here :)

My reasoning is that Self is considered a type annotation, and the subsequent help note should make it clear.

I can change it to use of Self requires ... but this may produce false positives/negatives in some edge cases because the heuristics this relies on is not 100% reliable. This might not be ok given that this is the primary label of the error, but I'm not sure.

I think calling Self a type annotation is a bit of a stretch. I'd like to see what the diagnostics look like with the change.

bors · 2022-08-26T14:00:32Z

☔ The latest upstream changes (presumably #100944) made this pull request unmergeable. Please resolve the merge conflicts.

estebank · 2022-09-01T15:20:48Z

compiler/rustc_borrowck/src/diagnostics/region_errors.rs

+    use rustc_lexer as lex;
+    fn tokenize(mut input: &str) -> impl Iterator<Item = (lex::TokenKind, &str)> {
+        std::iter::from_fn(move || {
+            if input.is_empty() {
+                return None;
+            }
+            let token = lex::first_token(input);
+            let token_str = &input[..(token.len as usize)];
+            input = &input[(token.len as usize)..];
+            Some((token.kind, token_str))
+        })
+    }
+
+    let snippet = tcx.sess.source_map().span_to_snippet(span).unwrap_or_default();
+    let has_self = tokenize(&snippet)
+        .find(|(tok, s)| *tok == lex::TokenKind::Ident && *s == "Self")
+        .and_then(|_| tcx.opt_parent(tcx.typeck_root_def_id(body_did)))
+        .filter(|parent_did| tcx.def_kind(parent_did) == DefKind::Impl);


I was going to suggest try to get the hir id somehow, but that's not available in borrowck. But you can use the body_did and the span to find the right expr by creating a new Visitor:

https://github.com/rust-lang/rust/pull/101002/files#diff-23edf23f7466b97eb3154b82f7b324983287c327937fd66e854b3df6759926ceR645-R751

Once you have that it means that you could even provide a structured suggestion.

apiraino · 2022-10-13T11:47:07Z

@aliemjay is s-waiting-on-review still the correct label? Or is there progress on parallel tracks (I notice the linked PRs)? Thanks for an update (feel free to use rustbot @author if this is not ready for review)

aliemjay · 2022-10-13T12:01:58Z

@rustbot author

JohnCSimon · 2023-01-01T18:55:28Z

@aliemjay
ping from triage - can you post your status on this PR? Thanks

FYI: when a PR is ready for review, send a message containing
@rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.

Dylan-DPC · 2023-05-15T09:05:14Z

Closing this as inactive. Feel free to reöpen this pr or create a new pr if you get the time to work on this. Thanks

rust-highfive assigned estebank Aug 24, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 24, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 24, 2022

aliemjay mentioned this pull request Aug 24, 2022

Borrow checking for static methods became more strict #100725

Open

This comment has been minimized.

Sign in to view

aliemjay force-pushed the lt-annot-dx branch from 6b3f0bb to f1b3a56 Compare August 25, 2022 00:13

This comment has been minimized.

Sign in to view

compiler-errors reviewed Aug 25, 2022

View reviewed changes

aliemjay added 3 commits August 25, 2022 19:58

reliably detect unnecessary lifetime annotations

427dcbc

register lifetime annotations from closure signature and UFCS calls under `ConstraintCategory::TypeAnnotation` and make sure we don't report type annotation errors unless thy're the only thing to blame.

improve annotation spans in thir/mir

86ce958

The diagnostic changes says it all :)

name regions in expected/found borrowck errors

c217c37

aliemjay force-pushed the lt-annot-dx branch from f1b3a56 to e2cbbe4 Compare August 25, 2022 17:24

suggest removing unnecessary lifetime annotations

7e3b351

aliemjay force-pushed the lt-annot-dx branch from e2cbbe4 to 7e3b351 Compare August 25, 2022 17:32

aliemjay marked this pull request as ready for review August 25, 2022 18:26

aliemjay commented Aug 25, 2022

View reviewed changes

jackh726 reviewed Aug 25, 2022

View reviewed changes

estebank reviewed Sep 1, 2022

View reviewed changes

QuineDot mentioned this pull request Sep 3, 2022

Suggest Struct::<'_> instead of Self when lifetime does not match #101393

Open

aliemjay mentioned this pull request Sep 10, 2022

rustc thinks a lifetime bound is necessary when passing an associated type into a nested call #101601

Closed

aliemjay mentioned this pull request Oct 9, 2022

Suggest using a struct's name instead of Self #102840

Closed

rustbot 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 Oct 13, 2022

Dylan-DPC closed this May 15, 2023

Dylan-DPC added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve lifetime annotation diagnostics #100976

improve lifetime annotation diagnostics #100976

aliemjay commented Aug 24, 2022 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

compiler-errors Aug 25, 2022

aliemjay Aug 25, 2022

compiler-errors Aug 25, 2022

aliemjay Aug 25, 2022

aliemjay commented Aug 25, 2022

aliemjay Aug 25, 2022 •

edited

Loading

estebank Sep 1, 2022

jackh726 Aug 25, 2022

aliemjay Aug 25, 2022

aliemjay Aug 25, 2022

jackh726 Aug 25, 2022

bors commented Aug 26, 2022

estebank Sep 1, 2022 •

edited

Loading

apiraino commented Oct 13, 2022

aliemjay commented Oct 13, 2022 •

edited

Loading

JohnCSimon commented Jan 1, 2023

Dylan-DPC commented May 15, 2023

improve lifetime annotation diagnostics #100976

improve lifetime annotation diagnostics #100976

Conversation

aliemjay commented Aug 24, 2022 • edited Loading

This comment has been minimized.

This comment has been minimized.

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

aliemjay commented Aug 25, 2022

aliemjay Aug 25, 2022 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Aug 26, 2022

estebank Sep 1, 2022 • edited Loading

Choose a reason for hiding this comment

apiraino commented Oct 13, 2022

aliemjay commented Oct 13, 2022 • edited Loading

JohnCSimon commented Jan 1, 2023

Dylan-DPC commented May 15, 2023

aliemjay commented Aug 24, 2022 •

edited

Loading

aliemjay Aug 25, 2022 •

edited

Loading

estebank Sep 1, 2022 •

edited

Loading

aliemjay commented Oct 13, 2022 •

edited

Loading