-
Notifications
You must be signed in to change notification settings - Fork 12.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't do post-method-probe error reporting steps if we're in a suggestion #125100
Conversation
@@ -91,7 +91,7 @@ pub enum CandidateSource { | |||
impl<'a, 'tcx> FnCtxt<'a, 'tcx> { | |||
/// Determines whether the type `self_ty` supports a visible method named `method_name` or not. | |||
#[instrument(level = "debug", skip(self))] | |||
pub fn method_exists( | |||
pub fn method_exists_for_diagnostic( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find the for_diagnostic
suffix naming convention confusing. I think it means "method exists (as considered for diagnostic purposes)", rather than "method exists for this diagnostic"? It feels like there should be a contrasting method_exists_for_non_diagnostic
method. I'm just not sure how to read it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it means "and you should only call this method if you're doing diagnostics".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kinda don't want to change it though now lol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I can live with it.
r=me once we consider the |
Well I'd like to make it more difficult for people to call methods that aren't correct on the good path. It's not necessary per se, but we are really bad at modularizing diagnostics code in |
This regressed in #120730, which added new calls to Before 1.78, this file still takes a few seconds. After 1.78, it essentially hangs. After this PR, it should take less than a second. |
Beta nominating as it can pretty negatively affect user experience when a programmer has many method errors in large projects. For example: Slow edit_distance on Zulip |
@@ -0,0 +1,157 @@ | |||
#![allow(non_snake_case)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent that you have a test case! Please add a brief comment explaining what this is testing and pointing to this PR.
r=me with the comment on the test. Thanks for fixing this! |
@bors r=nnethercote |
…mpiler-errors Rollup of 7 pull requests Successful merges: - rust-lang#119838 (style-guide: When breaking binops handle multi-line first operand better) - rust-lang#124844 (Use a proper probe for shadowing impl) - rust-lang#125047 (Migrate `run-make/issue-14500` to new `rmake.rs` format) - rust-lang#125080 (only find segs chain for missing methods when no available candidates) - rust-lang#125088 (Uplift `AliasTy` and `AliasTerm`) - rust-lang#125100 (Don't do post-method-probe error reporting steps if we're in a suggestion) - rust-lang#125118 (Use new utility functions/methods in run-make tests) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#125100 - compiler-errors:faster, r=nnethercote Don't do post-method-probe error reporting steps if we're in a suggestion Currently in method probing, if we fail to pick a method, then we reset and try to collect relevant candidates for method errors: https://github.com/rust-lang/rust/blob/34582118afaf00b0eb2d209a90b181c7156b501c/compiler/rustc_hir_typeck/src/method/probe.rs#L953-L993 However, we do method lookups via `lookup_method_for_diagnostic` and only care about the result if the method probe was a *success*. Namely, we don't need to do a bunch of other lookups on failure, since we throw away these results anyways, such as an expensive call to: https://github.com/rust-lang/rust/blob/34582118afaf00b0eb2d209a90b181c7156b501c/compiler/rustc_hir_typeck/src/method/probe.rs#L959 And: https://github.com/rust-lang/rust/blob/34582118afaf00b0eb2d209a90b181c7156b501c/compiler/rustc_hir_typeck/src/method/probe.rs#L985 --- This PR also renames some methods so it's clear that they're for diagnostics. r? `@nnethercote`
[beta] backports - Do not ICE on foreign malformed `diagnostic::on_unimplemented` rust-lang#124683 - Fix more ICEs in `diagnostic::on_unimplemented` rust-lang#124875 - rustdoc: use stability, instead of features, to decide what to show rust-lang#124864 - Don't do post-method-probe error reporting steps if we're in a suggestion rust-lang#125100 - Make `non-local-def` lint Allow by default rust-lang#124950 r? cuviper
Currently in method probing, if we fail to pick a method, then we reset and try to collect relevant candidates for method errors:
rust/compiler/rustc_hir_typeck/src/method/probe.rs
Lines 953 to 993 in 3458211
However, we do method lookups via
lookup_method_for_diagnostic
and only care about the result if the method probe was a success.Namely, we don't need to do a bunch of other lookups on failure, since we throw away these results anyways, such as an expensive call to:
rust/compiler/rustc_hir_typeck/src/method/probe.rs
Line 959 in 3458211
And:
rust/compiler/rustc_hir_typeck/src/method/probe.rs
Line 985 in 3458211
This PR also renames some methods so it's clear that they're for diagnostics.
r? @nnethercote