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

Add #[rustc_confusables] attribute to allow targeted "no method" error suggestions on standard library types #112239

Merged
merged 1 commit into from
Jul 16, 2023

Conversation

jieyouxu
Copy link
Member

@jieyouxu jieyouxu commented Jun 3, 2023

After this PR, the standard library developer can annotate methods on e.g. BTreeSet::push with #[rustc_confusables("insert")]. When the user mistypes btreeset.push(), BTreeSet::insert will be suggested if there are no other candidates to suggest. This PR lays the foundations for contributors to add rustc_confusables annotations to standard library types for targeted suggestions, as specified in #59450, or to address cases such as #108437.

Example

Assume BTreeSet is the standard library type:

// Standard library definition
#![feature(rustc_attrs)]

struct BTreeSet;

impl BTreeSet {
    #[rustc_confusables("push")]
    fn insert(&self) {}
}

// User code
fn main() {
    let x = BTreeSet {};
    x.push();
}

A new suggestion (which has lower precedence than suggestions for misspellings and only is shown when there are no misspellings suggestions) will be added to hint the user maybe they intended to write x.insert() instead:

error[E0599]: no method named `push` found for struct `BTreeSet` in the current scope
  --> test.rs:12:7
   |
3  | struct BTreeSet;
   | --------------- method `push` not found for this struct
...
12 |     x.push();
   |       ^^^^ method not found in `BTreeSet`
   |
help: you might have meant to use `insert`
   |
12 |     x.insert();
   |       ~~~~~~

error: aborting due to previous error

@rustbot
Copy link
Collaborator

rustbot commented Jun 3, 2023

r? @WaffleLapkin

(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 Jun 3, 2023
Copy link
Member

@Noratrieb Noratrieb left a comment

Choose a reason for hiding this comment

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

this may be something that could be made stable in the #[diagnostic] namespace in the future (see #111780 and the RFC). cc @weiznich you may be interseted

compiler/rustc_hir_typeck/src/method/suggest.rs Outdated Show resolved Hide resolved
compiler/rustc_hir_typeck/src/method/suggest.rs Outdated Show resolved Hide resolved
tests/ui/attributes/rustc_confusables.rs Outdated Show resolved Hide resolved
@jieyouxu jieyouxu force-pushed the targeted-no-method-suggestions branch from 407117c to 7240568 Compare June 3, 2023 15:33
@est31
Copy link
Member

est31 commented Jun 3, 2023

The suggestion made in the thread seems relevant, to use doc(alias) instead. Since #107108 it is already used for providing the needed suggestions.

Or is it explicitly wanted that if you search for BtreeSet::push then BtreeSet::insert should not show up? That seems unfortunate to me.

@jieyouxu
Copy link
Member Author

jieyouxu commented Jun 3, 2023

The suggestion made in the thread seems relevant, to use doc(alias) instead. Since #107108 it is already used for providing the needed suggestions.

Oops, I have no clue how I missed that... I will close this PR then.

Or is it explicitly wanted that if you search for BtreeSet::push then BtreeSet::insert should not show up? That seems unfortunate to me.

I think it's just so happens that BTreeSet::push doesn't have a #[doc(alias)] for insert, so I never noticed while working on this PR.

Sorry about this!

@jieyouxu jieyouxu closed this Jun 3, 2023
@est31
Copy link
Member

est31 commented Jun 3, 2023

@jieyouxu note that this is only my private opinion and I don't know if others agree... :) I did not want to discourage you, and it might be too early to close this PR just yet. I think it's definitely worth to discuss having an attribute separate from #[doc(alias = ...)], and it's really great that you provided an implementation, so thanks a lot for that!

I also dimly remember that either the libs (api) team or the rustdoc team expressed that they want to doc(alias) to be deployed very sparingly, but I can't find the specific thread, it was not in the tracking issue #50146 at least.

But on the other hand, adding a push alias for insert seems in-line with #91383 and #97565 which seem to still be present.

edit: the policy seems to be here.

@jieyouxu
Copy link
Member Author

jieyouxu commented Jun 3, 2023

I think it's definitely worth to discuss having an attribute separate from #[doc(alias = ...)]

I suppose I will keep this PR open for now, then, in case having a separate attribute might be useful.

But on the other hand, adding a push alias for insert seems in-line with #91383 and #97565 which seem to still be present.

I think I will open a separate PR for adding a doc-alias for BTreeSet::push to BTreeSet::insert.

@jieyouxu jieyouxu reopened this Jun 3, 2023
@est31
Copy link
Member

est31 commented Jun 3, 2023

FTR the policy can be found here and some links to more discussions can be found here.

@jieyouxu
Copy link
Member Author

jieyouxu commented Jun 3, 2023

Actually, after grepping through library/std and library/core, I don't think something like adding an alias for BTreeSet::insert would pass the doc(alias) policy requirement since it would be something like #[doc(alias = "push")] but push is also used commonly on many other stdlib types like Vec? So maybe a separate attribute might make sense after all, which would only affect no-method-found diagnostics and not documentation.

@est31
Copy link
Member

est31 commented Jun 3, 2023

Fair points... I think ultimately it's up to the libs team if they want doc(alias), #[diagnostic::confusable], or neither for BTreeSet::insert. From a general point of view, it might make sense to be able to not have some suggestion automatically also be a doc alias.

@Noratrieb
Copy link
Member

I think the problem with doc aliases is that if you have too many, it becomes quite hard to navigate the docs because you search for something and get lots of random other results showing up. In the diagnostics here you don't really have that problem as its only limited to impls on the type.

@WaffleLapkin
Copy link
Member

r? compiler

@compiler-errors
Copy link
Member

r? compiler

@rustbot rustbot assigned cjgillot and unassigned compiler-errors Jun 23, 2023
compiler/rustc_hir_typeck/src/method/suggest.rs Outdated Show resolved Hide resolved
compiler/rustc_hir_typeck/src/method/suggest.rs Outdated Show resolved Hide resolved
compiler/rustc_hir_typeck/src/method/suggest.rs Outdated Show resolved Hide resolved
compiler/rustc_hir_typeck/src/method/suggest.rs Outdated Show resolved Hide resolved
compiler/rustc_hir_typeck/src/method/suggest.rs Outdated Show resolved Hide resolved
compiler/rustc_hir_typeck/src/method/suggest.rs Outdated Show resolved Hide resolved
compiler/rustc_hir_typeck/src/method/suggest.rs Outdated Show resolved Hide resolved
@cjgillot cjgillot 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 Jun 24, 2023
@jieyouxu jieyouxu force-pushed the targeted-no-method-suggestions branch from 7240568 to a7cd813 Compare June 24, 2023 14:26
@rustbot
Copy link
Collaborator

rustbot commented Jun 24, 2023

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

@jieyouxu jieyouxu force-pushed the targeted-no-method-suggestions branch from a7cd813 to dab231d Compare June 24, 2023 14:31
@jieyouxu
Copy link
Member Author

@rustbot ready

@rustbot rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 24, 2023
@jieyouxu jieyouxu force-pushed the targeted-no-method-suggestions branch from 9f97dd8 to 410870c Compare July 3, 2023 08:38
@bors
Copy link
Contributor

bors commented Jul 14, 2023

☔ The latest upstream changes (presumably #113328) made this pull request unmergeable. Please resolve the merge conflicts.

compiler/rustc_passes/src/check_attr.rs Outdated Show resolved Hide resolved
compiler/rustc_feature/src/builtin_attrs.rs Outdated Show resolved Hide resolved
tests/ui/attributes/rustc_confusables.stderr Outdated Show resolved Hide resolved
tests/ui/attributes/rustc_confusables.stderr Outdated Show resolved Hide resolved
@jieyouxu jieyouxu force-pushed the targeted-no-method-suggestions branch from 410870c to a06fcba Compare July 16, 2023 10:47
Copy link
Contributor

@cjgillot cjgillot left a comment

Choose a reason for hiding this comment

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

Thanks! 3 nits and r=me.

@@ -370,6 +381,8 @@ passes_invalid_macro_export_arguments = `{$name}` isn't a valid `#[macro_export]

passes_invalid_macro_export_arguments_too_many_items = `#[macro_export]` can only take 1 or 0 arguments

passes_invalid_meta_item_kind = invalid meta item kind
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not used any more.

@@ -276,6 +282,9 @@ passes_expr_not_allowed_in_context =
passes_extern_main =
the `main` function cannot be declared in an `extern` block

passes_failed_to_extract_meta_item =
failed to extract meta item
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove it, it is not used any more.

compiler/rustc_hir_typeck/messages.ftl Outdated Show resolved Hide resolved
"no method" errors on standard library types

The standard library developer can annotate methods on e.g.
`BTreeSet::push` with `#[rustc_confusables("insert")]`. When the user
mistypes `btreeset.push()`, `BTreeSet::insert` will be suggested if
there are no other candidates to suggest.
@jieyouxu jieyouxu force-pushed the targeted-no-method-suggestions branch from a06fcba to 08c77a6 Compare July 16, 2023 11:22
@cjgillot
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jul 16, 2023

📌 Commit 08c77a6 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 Jul 16, 2023
@bors
Copy link
Contributor

bors commented Jul 16, 2023

⌛ Testing commit 08c77a6 with merge c4b1a20fd4e7f3fa31ac9fbf4b3e8dad1f8a912b...

@rust-log-analyzer
Copy link
Collaborator

The job armhf-gnu failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@bors
Copy link
Contributor

bors commented Jul 16, 2023

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 16, 2023
@cjgillot
Copy link
Contributor

Seems spurious: client.read_exact(&mut header) failed with Connection reset by peer
@bors retry

@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 Jul 16, 2023
@bors
Copy link
Contributor

bors commented Jul 16, 2023

⌛ Testing commit 08c77a6 with merge 11da267...

@bors
Copy link
Contributor

bors commented Jul 16, 2023

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 11da267 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 16, 2023
@bors bors merged commit 11da267 into rust-lang:master Jul 16, 2023
@rustbot rustbot added this to the 1.73.0 milestone Jul 16, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (11da267): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.6% [0.6%, 0.6%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.6% [0.6%, 0.6%] 1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 658.073s -> 657.937s (-0.02%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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.

10 participants