Better diagnostics with mismatched types due to implicit static lifetime #87244

jackh726 · 2021-07-18T06:26:56Z

I think this is my first diagnostics PR...definitely happy to hear thoughts on the direction/implementation here.

I was originally just trying to solve the error above, where the lifetime on a GAT was causing a cryptic "mismatched types" error. But as I was writing this, I realized that this (unintentionally) also applied to a different case: wf-in-foreign-fn-decls-issue-80468.rs. I'm not sure if this diagnostic should get a new error code, or even reuse an existing one. And, there might be some ways to make this even more generalized. Also, the error is a bit more lengthy and verbose than probably needed. So thoughts there are welcome too.

This PR essentially ended up adding a new nice region error pass that triggers if a type doesn't match the self type of an impl which is selected because of a predicate because of an implicit static bound on that self type.

r? @estebank

estebank

The new output is great! I have some nitpicks that I'd like to address before we land this.
Particularly I'd like to tweak the resulting output to be as short and clear as possible to people that might not know what a "requirement" or a "bound" is, or why a 'static lifetime requirement is involved.

I'm not sure if this diagnostic should get a new error code, or even reuse an existing one.

If we can add a new error code that'd be great, but it is perfectly fine to land things like this without an error code, at least at the beginning, as long as the error itself contains as much information as an error code index entry would.

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

src/test/ui/generic-associated-types/issue-78113-lifetime-mismatch-dyn-trait-box.stderr

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

estebank · 2021-07-19T18:15:20Z

src/test/ui/wf/wf-in-foreign-fn-decls-issue-80468.stderr

+LL | pub struct Wrapper<T: Trait>(T);
+   |                       ^^^^^


In this case the 'static lifetime obligation is introduced by the type parameter (T: Trait is implicitly 'sized). It'd be nice to proactively handle this, detect this case and mention that.

Actually, this is because the lifetime on Ref in impl Trait for Ref {} falls back to 'static in resolution.

The lifetime for Ref in pub fn repro(_: Wrapper<Ref>) on the other hand defaults to an anonymous lifetimes.

A diagnostic here still might be useful to say something like "this lifetime is being assumed as 'static'"

I think an extra diagnostic here might make sense for another PR?

estebank

I think an extra diagnostic here might make sense for another PR?

I would prefer if it was in the same PR, but I'm ready to merge this after addressing my nitpicks if you don't have the time to try and handle that case.

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

src/test/ui/wf/wf-in-foreign-fn-decls-issue-80468.stderr

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

jackh726 · 2021-07-20T03:49:23Z

Ok @estebank I modified the output just a bit, in order to make it a bit better in cases where there is an explicit static, multiple implicit statics, or when the impl isn't local.

I also added a small note to the assumed static in cases of missing lifetimes. This isn't in the implicit static error message, but should maybe be enough for someone to figure out what's going on. I'd rather land this and maybe work more on that another time.

estebank · 2021-07-20T05:31:35Z

compiler/rustc_ast_lowering/src/path.rs

@@ -336,6 +336,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
                            insertion_sp,
                            suggestion,
                        );
+                        err.note("assuming a `'static` lifetime...");


Not a fan of this new note, but I see your point.

estebank · 2021-07-20T05:31:42Z

@bors r+

bors · 2021-07-20T05:31:43Z

📌 Commit ae02491 has been approved by estebank

bors · 2021-07-20T10:56:14Z

⌛ Testing commit ae02491 with merge da7d405...

bors · 2021-07-20T13:36:56Z

☀️ Test successful - checks-actions
Approved by: estebank
Pushing da7d405 to master...

pnkfelix · 2021-07-20T19:22:09Z

This was flagged as causing a moderate regression with respect to instruction counts in the compiler: perf.rlo link

Most of the regressions are under the 1% threshold, but there's a lot of them that exceed the 0.2% threshold we use for non-noisy benchmarks.

Was this PR, which I understand adds an extra region error pass, expected to cause that much additional overhead? Skimming, I see a place where a Vec was replaced with a HashMap, maybe it is from that change?

jackh726 · 2021-07-20T19:31:13Z

@pnkfelix nope that's unexpected; the pass should should only run after a region error has been generated. The only thing that could have possibly had an effect, I think, is https://github.com/rust-lang/rust/pull/87244/files#diff-34d5893dd95fe09f9a0fd3341efacd1c21853bb34ba29d9d79bb9af26bb8a0a0R1906-R1911. Which is...not much of a change.

Mark-Simulacrum · 2021-09-16T03:03:11Z

Log of investigation work:

Selected the most prominent regression, stm32f4-check full.

First checked the self-profile view on perf.rlo for any changes in # of queries run. That can point to obvious causes, but not in this case. This means we need some local cachegrind diff'ing.

# Get our parent/master toolchains
rustup-toolchain-install-master 718d53b0cb7dde93499cb92950d60b412f5a3d05 da7d405357600a76f2b93b8aa41fe5ee5da7885d
# Benchmark our top regressed benchmark in cachegrind
cargo run --release --bin collector -- profile_local cachegrind +718d53b0cb7dde93499cb92950d60b412f5a3d05 before --include stm32f4 --builds Check --runs Full
cargo run --release --bin collector -- profile_local cachegrind +da7d405357600a76f2b93b8aa41fe5ee5da7885d after --include stm32f4 --builds Check --runs Full

This generates results/cgout-{before,after}-stm32f4-Check-Full

Using the output of cg_diff directly doesn't work well -- the hashes for each compiler are embedded into the paths and that makes cg_diff fail. I usually directly sed the cgout files rather than using the built-in --mod-filename support, since it's more general and works OK anyway. rustfilt is also helpful to avoid any crate hashes and such in mangled names that also interfere with diffing.

In this case:

cg_diff results/cgout-{before,after}-stm32f4-Check-Full | \
sed 's@da7d405357600a76f2b93b8aa41fe5ee5da7885d@@' | \
        sed 's@718d53b0cb7dde93499cb92950d60b412f5a3d05@@' | \
        rustfilt | \
        cg_annotate /dev/stdin | less

This produces:

140,274,957 (26.56%)  ???:rustc_trait_selection::traits::select::SelectionContext::match_impl
 55,140,172 (10.44%)  ???:core::ptr::drop_in_place<rustc_middle::traits::ObligationCauseCode>
 50,909,434 ( 9.64%)  /rustc//obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release/build/tikv-jemalloc-sys-8def611f6>
 50,002,422 ( 9.47%)  /rustc//obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release/build/tikv-jemalloc-sys-8def611f6>
 43,048,392 ( 8.15%)  /rustc//obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release/build/tikv-jemalloc-sys-8def611f6>
 32,248,934 ( 6.11%)  /rustc//obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release/build/tikv-jemalloc-sys-8def611f6>
 32,223,227 ( 6.10%)  /rustc//obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release/build/tikv-jemalloc-sys-8def611f6>
 25,111,562 ( 4.75%)  /rustc///library/std/src/sys/unix/alloc.rs:__rdl_alloc
 21,427,608 ( 4.06%)  /rustc//obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release/build/tikv-jemalloc-sys-8def611f6>
 14,349,464 ( 2.72%)  /rustc//obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release/build/tikv-jemalloc-sys-8def611f6>
 14,349,464 ( 2.72%)  /rustc//obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release/build/tikv-jemalloc-sys-8def611f6>
  7,174,732 ( 1.36%)  /rustc//obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release/build/tikv-jemalloc-sys-8def611f6>
  7,174,732 ( 1.36%)  /rustc///library/std/src/alloc.rs:__rdl_alloc
  7,143,206 ( 1.35%)  /rustc//obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release/build/tikv-jemalloc-sys-8def611f6>
  4,673,042 ( 0.88%)  ???:<rustc_middle::ty::sty::TraitRef as rustc_infer::infer::at::ToTrace>::to_trace
 -3,618,984 (-0.69%)  ???:hashbrown::map::RawEntryBuilderMut<K,V,S,A>::from_hash
  3,587,366 ( 0.68%)  /rustc//obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release/build/tikv-jemalloc-sys-8def611f6>
  3,587,366 ( 0.68%)  ???:__rust_alloc
  3,587,366 ( 0.68%)  ???:__rust_dealloc
  3,587,366 ( 0.68%)  /rustc///library/std/src/sys/unix/alloc.rs:__rdl_dealloc
  2,586,312 ( 0.49%)  ???:rustc_infer::infer::InferCtxt::commit_if_ok
  1,605,085 ( 0.30%)  ???:rustc_resolve::Resolver::resolve_ident_in_lexical_scope
    869,968 ( 0.16%)  ???:rustc_span::hygiene::HygieneData::is_descendant_of
    748,057 ( 0.14%)  /rustc//obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release/build/tikv-jemalloc-sys-8def611f6>
    625,536 ( 0.12%)  ???:rustc_infer::infer::at::At::eq_impl_headers

Overall cachegrind is pretty good at pointing in a vague direction -- inlining and such can obscure a little what the actual effects of a change like this are. That said, it is pretty obvious based on the above that the regression is indeed due to the new ObligationCause being created.

The diff above has some more minor changes in other functions - but we can mostly ignore those in this case. The majority of the delta is pretty clearly from extra allocation/deallocation and the creation of the cause code itself.

Since the investigation points at an obvious cause and a pretty 'unavoidable' one, I'm going to go ahead and mark this as triaged. I've also gone ahead and filed #89000 with a possible mitigation, but regardless of whether that works, I don't think this needs further triage/investigation.

@rustbot label +perf-regression-triaged

jackh726 · 2021-09-16T03:16:52Z

Thank you so much for looking into this @Mark-Simulacrum ♥

…enkov Reuse existing shared Lrc for MatchImpl parent This is a small performance win for the hot path, which helps to address this regression: rust-lang#87244 (comment).

rust-highfive assigned estebank Jul 18, 2021

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

jackh726 force-pushed the issue-71883 branch from 3bf33bc to 1af5254 Compare July 18, 2021 06:32

This comment has been minimized.

Sign in to view

jackh726 force-pushed the issue-71883 branch 2 times, most recently from 0542c18 to 69531a4 Compare July 18, 2021 07:26

This comment has been minimized.

Sign in to view

jackh726 force-pushed the issue-71883 branch from 69531a4 to aa0c0c9 Compare July 18, 2021 07:46

estebank reviewed Jul 19, 2021

View reviewed changes

jackh726 force-pushed the issue-71883 branch from aa0c0c9 to 7be9708 Compare July 19, 2021 22:14

Better diagnostics when mismatched types due to implict static lifetime

3cd5ad5

jackh726 force-pushed the issue-71883 branch from 7be9708 to 3cd5ad5 Compare July 19, 2021 22:20

estebank reviewed Jul 20, 2021

View reviewed changes

Better errors when we don't have implicit statics in trait objects

ae02491

jackh726 force-pushed the issue-71883 branch from e687b4d to ae02491 Compare July 20, 2021 03:46

estebank reviewed Jul 20, 2021

View reviewed changes

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 Jul 20, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 20, 2021

bors merged commit da7d405 into rust-lang:master Jul 20, 2021

rustbot added this to the 1.55.0 milestone Jul 20, 2021

bors mentioned this pull request Jul 20, 2021

Support HIR wf checking for function signatures #87265

Merged

jackh726 deleted the issue-71883 branch July 20, 2021 14:10

rylev added the perf-regression Performance regression. label Jul 21, 2021

Mark-Simulacrum mentioned this pull request Sep 16, 2021

Reuse existing shared Lrc for MatchImpl parent #89000

Merged

rustbot added the perf-regression-triaged The performance regression has been triaged. label Sep 16, 2021

jyn514 mentioned this pull request Sep 17, 2021

rustdoc: Pre-calculate traits that are in scope for doc links #88679

Merged

jsha mentioned this pull request Oct 12, 2021

Fix performance regression from #89695 #89732

Closed

jackh726 restored the issue-71883 branch March 12, 2022 18:30

jackh726 deleted the issue-71883 branch March 12, 2022 18:35

jackh726 restored the issue-71883 branch March 12, 2022 18:42

jackh726 deleted the issue-71883 branch March 12, 2022 18:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better diagnostics with mismatched types due to implicit static lifetime #87244

Better diagnostics with mismatched types due to implicit static lifetime #87244

jackh726 commented Jul 18, 2021

This comment has been minimized.

This comment has been minimized.

estebank left a comment •

edited

Loading

estebank Jul 19, 2021

jackh726 Jul 19, 2021

jackh726 Jul 19, 2021

estebank left a comment

jackh726 commented Jul 20, 2021

estebank Jul 20, 2021

estebank commented Jul 20, 2021

bors commented Jul 20, 2021

bors commented Jul 20, 2021

bors commented Jul 20, 2021

pnkfelix commented Jul 20, 2021

jackh726 commented Jul 20, 2021

Mark-Simulacrum commented Sep 16, 2021

jackh726 commented Sep 16, 2021

Better diagnostics with mismatched types due to implicit static lifetime #87244

Better diagnostics with mismatched types due to implicit static lifetime #87244

Conversation

jackh726 commented Jul 18, 2021

This comment has been minimized.

This comment has been minimized.

estebank left a comment • edited Loading

Choose a reason for hiding this comment

estebank Jul 19, 2021

Choose a reason for hiding this comment

jackh726 Jul 19, 2021

Choose a reason for hiding this comment

jackh726 Jul 19, 2021

Choose a reason for hiding this comment

estebank left a comment

Choose a reason for hiding this comment

jackh726 commented Jul 20, 2021

estebank Jul 20, 2021

Choose a reason for hiding this comment

estebank commented Jul 20, 2021

bors commented Jul 20, 2021

bors commented Jul 20, 2021

bors commented Jul 20, 2021

pnkfelix commented Jul 20, 2021

jackh726 commented Jul 20, 2021

Mark-Simulacrum commented Sep 16, 2021

jackh726 commented Sep 16, 2021

estebank left a comment •

edited

Loading