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 case insensitive comparison, besides Levenstein for DYM #46347

Merged
merged 3 commits into from Dec 2, 2017

Conversation

Projects
None yet
8 participants
@raventid
Contributor

raventid commented Nov 28, 2017

Closes #46332

Draft version. The idea is that Levenstein does not work for some cases when we have multiple equal weights for strings. I didn't understand the case with if found != name => Some(found) so it means that new code does not work correctly yet.

At least now I think that we might return all maximal weights from levenstein and think about next cases in priority order:

  1. There is exact match -> None
  2. There is exact match, but case insensitive -> Some(match)
  3. There is some match from levenstein -> Some(matches.take_any)
  4. There is no match -> None

@estebank WDYT?

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Nov 28, 2017

Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @pnkfelix (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

Collaborator

rust-highfive commented Nov 28, 2017

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @pnkfelix (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

Show outdated Hide outdated src/test/ui/issue46332.rs Outdated
Show outdated Hide outdated src/test/ui/issue46332.rs Outdated
Show outdated Hide outdated src/librustc_resolve/lib.rs Outdated
Show outdated Hide outdated src/librustc_resolve/lib.rs Outdated
Show outdated Hide outdated src/librustc_resolve/lib.rs Outdated
@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Nov 29, 2017

Contributor

I didn't understand the case with if found != name

It looks very silly when you write foo and compiler tells you "foo is wrong, use foo instead".
The candidate search isn't precise so it can sometimes find things that have the same name (perfect match from Levenshtein's distance point of view!) but are still inaccessible, so we need to filter them away.

Contributor

petrochenkov commented Nov 29, 2017

I didn't understand the case with if found != name

It looks very silly when you write foo and compiler tells you "foo is wrong, use foo instead".
The candidate search isn't precise so it can sometimes find things that have the same name (perfect match from Levenshtein's distance point of view!) but are still inaccessible, so we need to filter them away.

@estebank

Agree with the comments above.

Show outdated Hide outdated src/librustc_resolve/lib.rs Outdated
Show outdated Hide outdated src/librustc_resolve/lib.rs Outdated
Show outdated Hide outdated src/test/ui/issue46332.rs Outdated
struct TyInt {}
fn main() {
TyUInt {};

This comment has been minimized.

@estebank

estebank Nov 30, 2017

Contributor

You need to add a comment below this to test for errors that do not care for the ui output:

//~^ ERROR cannot find struct, variant or union type `TyUInt` in this scope

The lack of this is why the test failed. It was made mandatory recently.

@estebank

estebank Nov 30, 2017

Contributor

You need to add a comment below this to test for errors that do not care for the ui output:

//~^ ERROR cannot find struct, variant or union type `TyUInt` in this scope

The lack of this is why the test failed. It was made mandatory recently.

This comment has been minimized.

@raventid

raventid Dec 1, 2017

Contributor

Thank you! Fixed the build.

@raventid

raventid Dec 1, 2017

Contributor

Thank you! Fixed the build.

@raventid raventid changed the title from WIP: Add case insensitive comparison, besides Levenstein for DYM to Add case insensitive comparison, besides Levenstein for DYM Dec 1, 2017

@estebank

This comment has been minimized.

Show comment
Hide comment
@estebank

estebank Dec 1, 2017

Contributor

@bors r+

Contributor

estebank commented Dec 1, 2017

@bors r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 1, 2017

Contributor

📌 Commit f18446e has been approved by estebank

Contributor

bors commented Dec 1, 2017

📌 Commit f18446e has been approved by estebank

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Dec 1, 2017

Contributor

An alternative would be to adjust the distance formula to incorporate case differences, e.g. insertion/deletion/arbitrary substitution would still add 1 to the distance, but different case substitution would add 0.5.

Contributor

petrochenkov commented Dec 1, 2017

An alternative would be to adjust the distance formula to incorporate case differences, e.g. insertion/deletion/arbitrary substitution would still add 1 to the distance, but different case substitution would add 0.5.

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 2, 2017

Contributor

⌛️ Testing commit f18446e with merge 7a64987...

Contributor

bors commented Dec 2, 2017

⌛️ Testing commit f18446e with merge 7a64987...

bors added a commit that referenced this pull request Dec 2, 2017

Auto merge of #46347 - raventid:did-you-mean-increase-accuracy, r=est…
…ebank

Add case insensitive comparison, besides Levenstein for DYM

Closes #46332

Draft version. The idea is that Levenstein does not work for some cases when we have multiple equal weights for strings. I didn't understand the case with `if found != name => Some(found)` so it means that new code does not work correctly yet.

At least now I think that we might return all maximal weights from levenstein and think about next cases in priority order:

1) There is exact match -> None
2) There is exact match, but case insensitive -> Some(match)
3) There is some match from levenstein -> Some(matches.take_any)
4) There is no match -> None

@estebank WDYT?
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 2, 2017

Contributor

☀️ Test successful - status-appveyor, status-travis
Approved by: estebank
Pushing 7a64987 to master...

Contributor

bors commented Dec 2, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: estebank
Pushing 7a64987 to master...

@bors bors merged commit f18446e into rust-lang:master Dec 2, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment