Skip to content
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

fix: dedup static_candidates before report #111872

Merged
merged 1 commit into from
May 30, 2023
Merged

Conversation

bvanjoi
Copy link
Contributor

@bvanjoi bvanjoi commented May 23, 2023

Fixes #103646

record_static_candidate had been executed twice, resulting in the presence of two identical CandidateSource::Trait(Cat) in static_candidates. This PR aims to deduplication the static_candidates list, allowing it to execute suggest_associated_call_syntax properly.

@rustbot
Copy link
Collaborator

rustbot commented May 23, 2023

r? @jackh726

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 23, 2023
@@ -473,6 +473,9 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
let mut custom_span_label = false;

let static_candidates = &mut no_match_data.static_candidates;

static_candidates.dedup();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a comment why we cannot remove the duplicated call to record_static_candidate?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inherent and extension may append the same item to static_candidates.

For example:

trait Cat {
    fn nya() {}
}

fn uwu<T: Cat>(c: T) {
    c.nya();
}

Both <T as Cat> (inherent) and <_ as Cat> (extension) had been tried.

And another:

trait Cat {
    fn nya() {}
}

fn uwu<T>(c: T) {
    c.nya();
}

<_ as Cat> (extension) should be tried.

Therefore, we can't remove duplicated call of record_static_candidate.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment you added says that in order to improve the diagnostic, we record both inherent and extension cases. But this PR does the opposite: we deduplicate to improve the diagnostic.
You don't need to link to the PR. The 2 lines-long explanation is enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have two questions:

  1. Is it correct to say that we cannot remove the duplicated call of record_static_candidate? Also, is it intended behavior that the unexpected diagnostic of Unhelpful help when calling associated trait function as method #103646 was caused by invoking record_static_candidate for the same item in both assemble_extension_candidates_for_trait and assemble_inherent_impl_probe?
  2. Is static_candidates.dedup() the right approach here? If this statement has too broad impact, can we change the condition to !self.has_applicable_self(&item) && !self.static_candidates.contains(&item) in extension_candidates?

@cjgillot
Copy link
Contributor

Thanks!
@bors r+

@bors
Copy link
Contributor

bors commented May 28, 2023

📌 Commit f78369b has been approved by cjgillot

It is now in the queue for this repository.

@bors 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 May 28, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request May 30, 2023
Rollup of 7 pull requests

Successful merges:

 - rust-lang#107916 (fix comment on Allocator trait)
 - rust-lang#111543 (Uplift `clippy::invalid_utf8_in_unchecked` lint)
 - rust-lang#111872 (fix: dedup `static_candidates` before report)
 - rust-lang#111955 (bootstrap: Various Step refactors)
 - rust-lang#112060 (`EarlyBinder::new` -> `EarlyBinder::bind`)
 - rust-lang#112064 (Migrate GUI colors test to original CSS color format)
 - rust-lang#112100 (Don't typecheck recovered method call from suggestion)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 0c9f87c into rust-lang:master May 30, 2023
@rustbot rustbot added this to the 1.72.0 milestone May 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unhelpful help when calling associated trait function as method
5 participants