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

Suggest similarly named associated items in trait impls #89248

Merged

Conversation

hkmatsumoto
Copy link
Member

Fix #85942

Previously, the compiler didn't suggest similarly named associated items unlike we do in many situations. This patch adds such diagnostics for associated functions, types, and constants.

@rust-highfive
Copy link
Collaborator

r? @oli-obk

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 25, 2021
@hkmatsumoto hkmatsumoto added A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix`. labels Sep 25, 2021
@hkmatsumoto
Copy link
Member Author

r? @estebank

@rust-highfive rust-highfive assigned estebank and unassigned oli-obk Sep 25, 2021
@hkmatsumoto hkmatsumoto force-pushed the suggest-similarly-named-assoc-items branch from 0512297 to 32f4d42 Compare September 25, 2021 16:04
@rust-log-analyzer

This comment has been minimized.

@hkmatsumoto hkmatsumoto force-pushed the suggest-similarly-named-assoc-items branch from 32f4d42 to dc9a931 Compare September 25, 2021 16:27
| ^^^^^^^^^^^^^^
| |
| not a member of trait `Tr`
| help: there is an associated function with a similar name: `method`
Copy link
Member Author

Choose a reason for hiding this comment

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

I have no idea what it is ¯_(ツ)_/¯
We should skip this diagnostic in macros maybe.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably special case macros, but it's not needed right now :-/

Copy link
Contributor

@estebank estebank left a comment

Choose a reason for hiding this comment

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

I'm seeing the new output and it is an improvement, but I think we can get even better output if instead of the Option<Symbol> we kept an Option<DefId>, so that we could show a span_note with the span for the definition of the assoc item in a trait, instead of a span_label or span_suggestion. Part of why I'm saying that is because 1) the current span_suggestion, when applied will completely remove the underlined assoc item with the suggested identifier (not what we want) and 2) we are not making checks to see that the similar named assoc item matches (what if the method has a different number of arguments, for example?).

What do you think?

| ^^^^^^^^^^^^^^
| |
| not a member of trait `Tr`
| help: there is an associated function with a similar name: `method`
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably special case macros, but it's not needed right now :-/

@hkmatsumoto hkmatsumoto force-pushed the suggest-similarly-named-assoc-items branch from dc9a931 to a6eafcb Compare September 28, 2021 15:17
Previously, the compiler didn't suggest similarly named associated items
unlike we do in many situations. This patch adds such diagnostics for
associated functions, types and constants.
@hkmatsumoto hkmatsumoto force-pushed the suggest-similarly-named-assoc-items branch from a6eafcb to cef736f Compare September 28, 2021 15:22
Comment on lines +46 to +60
error[E0046]: not all trait items implemented, missing: `Type`, `foo`, `bar`, `qux`
--> $DIR/suggest-trait-items.rs:11:1
|
LL | type Type;
| ---------- `Type` from trait
LL |
LL | fn foo();
| --------- `foo` from trait
LL | fn bar();
| --------- `bar` from trait
LL | fn qux();
| --------- `qux` from trait
...
LL | impl Foo for A {
| ^^^^^^^^^^^^^^ missing `Type`, `foo`, `bar`, `qux` in implementation
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be nice if we didn't error for missing elements that were suggested, but that's a nice to have, not a must have.

Copy link
Member Author

Choose a reason for hiding this comment

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

Opened #89326

@estebank
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Sep 28, 2021

📌 Commit cef736f has been approved by estebank

@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 Sep 28, 2021
Manishearth added a commit to Manishearth/rust that referenced this pull request Sep 30, 2021
…-assoc-items, r=estebank

Suggest similarly named associated items in trait impls

Fix rust-lang#85942

Previously, the compiler didn't suggest similarly named associated items unlike we do in many situations. This patch adds such diagnostics for associated functions, types, and constants.
Manishearth added a commit to Manishearth/rust that referenced this pull request Sep 30, 2021
…-assoc-items, r=estebank

Suggest similarly named associated items in trait impls

Fix rust-lang#85942

Previously, the compiler didn't suggest similarly named associated items unlike we do in many situations. This patch adds such diagnostics for associated functions, types, and constants.
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 1, 2021
…arth

Rollup of 8 pull requests

Successful merges:

 - rust-lang#88782 (Fix ICE when `start` lang item has wrong generics)
 - rust-lang#89202 (Resolve infered types when complaining about unexpected call type )
 - rust-lang#89248 (Suggest similarly named associated items in trait impls)
 - rust-lang#89303 (Add `#[must_not_suspend]` to some types in std)
 - rust-lang#89306 (thread: implements available_concurrency on haiku)
 - rust-lang#89314 (fix(lint): don't suggest refutable patterns to "fix" irrefutable bind)
 - rust-lang#89370 (CTFE: tweak aggregate rvalue handling)
 - rust-lang#89392 (bootstrap: Update comment in config.library.toml.)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 837ac87 into rust-lang:master Oct 1, 2021
@rustbot rustbot added this to the 1.57.0 milestone Oct 1, 2021
@hkmatsumoto hkmatsumoto deleted the suggest-similarly-named-assoc-items branch October 1, 2021 07:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix`. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

method xxx is not a member of trait XXX doesn't check spelling
7 participants