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

Consider doc(alias) when providing typo suggestions #107108

Merged

Conversation

sulami
Copy link
Contributor

@sulami sulami commented Jan 20, 2023

This means that

impl Foo {
    #[doc(alias = "quux")]
    fn bar(&self) {}
}

fn main() {
    (Foo {}).quux();
}

will suggest bar. This currently uses the "there is a method with a similar name" help text, because the point where we choose and emit a suggestion is different from where we gather the suggestions. Changes have mainly been made to the latter.

The selection code will now fall back to aliased candidates, but generally only if there is no candidate that matches based on the existing Levenshtein methodology.

Fixes #83968.

@rustbot
Copy link
Collaborator

rustbot commented Jan 20, 2023

r? @Nilstrieb

(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 Jan 20, 2023
@sulami sulami force-pushed the issue-83968-doc-alias-typo-suggestions branch 2 times, most recently from 123d275 to cf5e337 Compare January 20, 2023 08:23
if let sym::doc = attr.name_or_empty() {
if let Some(values) = attr.meta_item_list() {
for v in values {
if v.name_or_empty().as_str() != "alias" {
Copy link
Member

Choose a reason for hiding this comment

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

Could you add alias to the symbols table in rustc_span/src/symbols.rs (I think that was the path) instead of doing this string comparison?

@sulami sulami force-pushed the issue-83968-doc-alias-typo-suggestions branch from cf5e337 to c67108b Compare January 21, 2023 06:56
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 looks mostly fine to me but I'll r? compiler-errors who has more experience with this code than me for a second look whether these are the right places to put this

tests/ui/methods/method-not-found-but-doc-alias.rs Outdated Show resolved Hide resolved
@rustbot rustbot assigned compiler-errors and unassigned Noratrieb Jan 21, 2023
@sulami sulami force-pushed the issue-83968-doc-alias-typo-suggestions branch from c67108b to 8c5843a Compare January 21, 2023 10:39
@compiler-errors
Copy link
Member

The implementation looks fine here. I think this is only invoked on error paths, so the extra computation when probing for candidates is fine.

I have a few nits that you could address if you want:

  1. probe_for_lev_candidates could be renamed to something that indicates probing for alternative method names in general, since it's no longer just levenshtein candidates.
  2. The if nesting could probably be flattened even more in matches_by_doc_alias.

@sulami sulami force-pushed the issue-83968-doc-alias-typo-suggestions branch from 8c5843a to b4af140 Compare January 22, 2023 08:58

Err(MethodError::NoMatch(NoMatchData {
static_candidates,
unsatisfied_predicates,
out_of_scope_traits,
lev_candidate,
lev_candidate: similar_candidate,
Copy link
Member

Choose a reason for hiding this comment

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

The field in NoMatchData should probably be renamed too.

Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

This looks good but lev_candidate should also be renamed in NoMatchData

Afterwards I can approve.

This means that

```rust
impl Foo {
    #[doc(alias = "quux")]
    fn bar(&self) {}
}

fn main() {
    (Foo {}).quux();
}
```

will suggest `bar`. This currently uses the "there is a method with a
similar name" help text, because the point where we choose and emit a
suggestion is different from where we gather the suggestions. Changes
have mainly been made to the latter.

The selection code will now fall back to aliased candidates, but
generally only if there is no candidate that matches based on the
existing Levenshtein methodology.

Fixes rust-lang#83968.
@sulami sulami force-pushed the issue-83968-doc-alias-typo-suggestions branch from b4af140 to f908f0b Compare January 23, 2023 01:07
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

Thanks!

@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jan 23, 2023

📌 Commit f908f0b has been approved by compiler-errors

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

Successful merges:

 - rust-lang#104926 (Move relationships from FulfillmentContext to Inherited)
 - rust-lang#106854 (Add `Arc::into_inner` for safely discarding `Arc`s without calling the destructor on the inner type.)
 - rust-lang#107108 (Consider doc(alias) when providing typo suggestions)
 - rust-lang#107186 (rustdoc: Use correct pseudo-element selector)
 - rust-lang#107192 (Add myself to the mailmap)
 - rust-lang#107195 (Fix typo in universal_regions.rs comment)
 - rust-lang#107203 (Suggest remove deref for type mismatch)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f4f3335 into rust-lang:master Jan 23, 2023
@rustbot rustbot added this to the 1.69.0 milestone Jan 23, 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.

Take doc(alias) into account when providing typo suggestions
6 participants