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

Make derive(Trait) suggestion more accurate #119362

Merged
merged 2 commits into from
Jan 4, 2024

Conversation

estebank
Copy link
Contributor

Only suggest derive(PartialEq) when both LHS and RHS types are the same, otherwise the suggestion is not useful.

@rustbot
Copy link
Collaborator

rustbot commented Dec 27, 2023

r? @wesleywiser

(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 Dec 27, 2023
@@ -2322,7 +2323,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
.iter()
.map(|e| (e.obligation.predicate, None, Some(e.obligation.cause.clone())))
.collect();
self.suggest_derive(err, &preds);
self.suggest_derive(err, &preds, suggest_derive);
}

pub fn suggest_derive(
Copy link
Member

Choose a reason for hiding this comment

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

I find it confusing that this method is called suggest_derive but apparently does more than suggesting #[derive]. It also adds the note the trait[…] […] must be implemented. Adding the flag suggest_derive to … suggest_derive makes it even more confusing. Could this method be renamed or even refactored into two methods potentially allowing us to get rid of the bool parameter altogether?

Copy link
Contributor Author

@estebank estebank Dec 28, 2023

Choose a reason for hiding this comment

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

I wanted to make this as minimal a diff as possible :)

suggest_derive also emits some extra notes (that I didn't want to lose). I'm assuming we added that after the method was created and we didn't modify the method name then. I can either rename the method and keep the argument, or split the logic in two (or three?) methods and call them only when needed.

Copy link
Member

@fmease fmease Dec 28, 2023

Choose a reason for hiding this comment

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

I see :) If you can find a good name for it, go for a rename. You can also do a refactor instead either in this PR or or in a follow-up one, larger diffs are fine by me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think of the change?

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, it's an improvement. Although, the way it's split into two methods doesn't feel right: note_predicate_source_and_get_derives still does two things, it calculates traits and derives and only adds a note for traits but never for derive and we do a bunch of work formatting derives just to throw it away if !suggest_derive.

@fmease fmease assigned fmease and unassigned wesleywiser Dec 28, 2023
@fmease fmease 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 Dec 28, 2023
@estebank estebank added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 28, 2023
Copy link
Member

@fmease fmease left a comment

Choose a reason for hiding this comment

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

r=me with the actionable subset of my most recent suggestions applied or if you find a better way to split those two functions then go for it and re-request a review :)

compiler/rustc_hir_typeck/src/method/suggest.rs Outdated Show resolved Hide resolved
&self,
err: &mut Diagnostic,
unsatisfied_predicates: &[(
ty::Predicate<'tcx>,
Option<ty::Predicate<'tcx>>,
Option<ObligationCause<'tcx>>,
)],
) {
) -> Vec<(String, Span, String)> {
Copy link
Member

@fmease fmease Dec 29, 2023

Choose a reason for hiding this comment

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

Hmm, it's only in the error path but I'd love to skip the entire if let Some(diagnostic_name) = self.tcx.get_diagnostic_name(trait_pred.def_id()) check if !suggest_derive but then we'd need to introduce suggested_derive: bool to note_pre_src_and_get_derives :(

compiler/rustc_hir_typeck/src/method/suggest.rs Outdated Show resolved Hide resolved
@fmease fmease 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 Dec 29, 2023
Only suggest `derive(PartialEq)` when both LHS and RHS types
are the same, otherwise the suggestion is not useful.
@estebank
Copy link
Contributor Author

estebank commented Jan 4, 2024

@bors r=fmease

@bors
Copy link
Contributor

bors commented Jan 4, 2024

📌 Commit 2474b37 has been approved by fmease

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 4, 2024
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Jan 4, 2024
…n, r=fmease

Make `derive(Trait)` suggestion more accurate

Only suggest `derive(PartialEq)` when both LHS and RHS types are the same, otherwise the suggestion is not useful.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 4, 2024
…mpiler-errors

Rollup of 10 pull requests

Successful merges:

 - rust-lang#118521 (Enable address sanitizer for MSVC targets using INFERASANLIBS linker flag)
 - rust-lang#119026 (std::net::bind using -1 for openbsd which in turn sets it to somaxconn.)
 - rust-lang#119195 (Make named_asm_labels lint not trigger on unicode and trigger on format args)
 - rust-lang#119204 (macro_rules: Less hacky heuristic for using `tt` metavariable spans)
 - rust-lang#119362 (Make `derive(Trait)` suggestion more accurate)
 - rust-lang#119397 (Recover parentheses in range patterns)
 - rust-lang#119414 (bootstrap: Move -Clto= setting from Rustc::run to rustc_cargo)
 - rust-lang#119417 (Uplift some miscellaneous coroutine-specific machinery into `check_closure`)
 - rust-lang#119540 (Don't synthesize host effect args inside trait object types)
 - rust-lang#119555 (Add codegen test for RVO on MaybeUninit)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 4, 2024
…mpiler-errors

Rollup of 10 pull requests

Successful merges:

 - rust-lang#118521 (Enable address sanitizer for MSVC targets using INFERASANLIBS linker flag)
 - rust-lang#119026 (std::net::bind using -1 for openbsd which in turn sets it to somaxconn.)
 - rust-lang#119195 (Make named_asm_labels lint not trigger on unicode and trigger on format args)
 - rust-lang#119204 (macro_rules: Less hacky heuristic for using `tt` metavariable spans)
 - rust-lang#119362 (Make `derive(Trait)` suggestion more accurate)
 - rust-lang#119397 (Recover parentheses in range patterns)
 - rust-lang#119414 (bootstrap: Move -Clto= setting from Rustc::run to rustc_cargo)
 - rust-lang#119417 (Uplift some miscellaneous coroutine-specific machinery into `check_closure`)
 - rust-lang#119540 (Don't synthesize host effect args inside trait object types)
 - rust-lang#119555 (Add codegen test for RVO on MaybeUninit)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 4, 2024
…iaskrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#118521 (Enable address sanitizer for MSVC targets using INFERASANLIBS linker flag)
 - rust-lang#119026 (std::net::bind using -1 for openbsd which in turn sets it to somaxconn.)
 - rust-lang#119195 (Make named_asm_labels lint not trigger on unicode and trigger on format args)
 - rust-lang#119204 (macro_rules: Less hacky heuristic for using `tt` metavariable spans)
 - rust-lang#119362 (Make `derive(Trait)` suggestion more accurate)
 - rust-lang#119397 (Recover parentheses in range patterns)
 - rust-lang#119417 (Uplift some miscellaneous coroutine-specific machinery into `check_closure`)
 - rust-lang#119539 (Fix typos)
 - rust-lang#119540 (Don't synthesize host effect args inside trait object types)
 - rust-lang#119555 (Add codegen test for RVO on MaybeUninit)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 4, 2024
…iaskrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#118521 (Enable address sanitizer for MSVC targets using INFERASANLIBS linker flag)
 - rust-lang#119026 (std::net::bind using -1 for openbsd which in turn sets it to somaxconn.)
 - rust-lang#119195 (Make named_asm_labels lint not trigger on unicode and trigger on format args)
 - rust-lang#119204 (macro_rules: Less hacky heuristic for using `tt` metavariable spans)
 - rust-lang#119362 (Make `derive(Trait)` suggestion more accurate)
 - rust-lang#119397 (Recover parentheses in range patterns)
 - rust-lang#119417 (Uplift some miscellaneous coroutine-specific machinery into `check_closure`)
 - rust-lang#119539 (Fix typos)
 - rust-lang#119540 (Don't synthesize host effect args inside trait object types)
 - rust-lang#119555 (Add codegen test for RVO on MaybeUninit)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 4, 2024
…iaskrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#118521 (Enable address sanitizer for MSVC targets using INFERASANLIBS linker flag)
 - rust-lang#119026 (std::net::bind using -1 for openbsd which in turn sets it to somaxconn.)
 - rust-lang#119195 (Make named_asm_labels lint not trigger on unicode and trigger on format args)
 - rust-lang#119204 (macro_rules: Less hacky heuristic for using `tt` metavariable spans)
 - rust-lang#119362 (Make `derive(Trait)` suggestion more accurate)
 - rust-lang#119397 (Recover parentheses in range patterns)
 - rust-lang#119417 (Uplift some miscellaneous coroutine-specific machinery into `check_closure`)
 - rust-lang#119539 (Fix typos)
 - rust-lang#119540 (Don't synthesize host effect args inside trait object types)
 - rust-lang#119555 (Add codegen test for RVO on MaybeUninit)

r? `@ghost`
`@rustbot` modify labels: rollup
@compiler-errors
Copy link
Member

@bors rollup

@bors bors merged commit 6526b16 into rust-lang:master Jan 4, 2024
11 checks passed
@rustbot rustbot added this to the 1.77.0 milestone Jan 4, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 4, 2024
Rollup merge of rust-lang#119362 - estebank:restrict-derive-suggestion, r=fmease

Make `derive(Trait)` suggestion more accurate

Only suggest `derive(PartialEq)` when both LHS and RHS types are the same, otherwise the suggestion is not useful.
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.

6 participants