Checker:: Execute levenshtein before other context checking #39291

Freyskeyd · 2017-01-25T12:34:31Z

As explain here i think it's better to check for a miss typing before checking context dependent help.

struct Handle {}

struct Something {
     handle: Handle
}

fn main() {
     let handle: Handle = Handle {};

     let s: Something = Something {
         // Checker detect an error and propose a solution with `Handle { /* ... */ }`
         // but it's a miss typing of `handle`
         handle: Handle 
    };
}

Ping: @nagisa for #39226

Signed-off-by: Freyskeyd simon.paitrault@gmail.com

rust-highfive · 2017-01-25T12:34:37Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @arielb1 (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.

nagisa · 2017-01-25T12:36:03Z

Needs a test at the very least.

nagisa · 2017-01-25T12:40:21Z

Question: does the other suggestion fire at all anymore?

e.g. I would expect rustc to report both hints in case of say:

struct Handle { field: () }
struct Something { handle: Handle }
fn x(handl: Handle) -> Something { 
     Something { handle: Handle } // could be both forgotten initialization and typo, and Handle can be initialized, because its interior not private in this context.
}

Freyskeyd · 2017-01-25T12:43:20Z

Mmh i don't think it will check for both hints. I will check if i can do it.

petrochenkov · 2017-01-25T13:08:13Z

r- from me
the point of #39226 is that suggestion Handle { /* fields */ } is unusable due to private fields, so the context dependent help should perform field privacy check.
Prioritizing semi-random Levenshtein over manually crafted help seems like a bad idea in general.

petrochenkov · 2017-01-25T13:29:48Z

Showing all applicable labels is an alternative too, but I'd like to see first how the result looks visually.

mrhota · 2017-02-23T22:28:10Z

@Freyskeyd ping. what's the story here?

Freyskeyd · 2017-02-24T08:50:32Z

hi @mrhota, I didn't work on it for a while (no time for the moment).

I can close this PR if it's a problem.

arielb1 · 2017-03-07T09:35:15Z

@bors r=nrc

You are more familiar with this code.

bors · 2017-03-07T09:35:16Z

📌 Commit 3d38dbd has been approved by nrc

bors · 2017-03-07T09:35:27Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: rustbuild: Use copies instead of hard links #39518

bors · 2017-03-07T09:35:28Z

📌 Commit 3d38dbd has been approved by nrc

bors · 2017-03-09T10:43:29Z

🔒 Merge conflict

bors · 2017-03-12T19:26:46Z

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

alexcrichton · 2017-04-01T22:55:26Z

@Freyskeyd gah sorry for the delay here!

It looks like there's some errors on Travis though that would prevent this from landing?

leoyvens · 2017-04-10T14:38:46Z

Is this supposed to be approved? I think @arielb1 meant to r? nrc rather than r=nrc

arielb1 · 2017-04-13T15:08:26Z

r? @nrc

carols10cents · 2017-04-17T14:20:18Z

Yeah, looks like there are some test failures: https://travis-ci.org/rust-lang/rust/jobs/210479172#L4366

Let us know if you need any help @Freyskeyd!

Freyskeyd · 2017-04-18T08:10:38Z

@carols10cents I will make an update this week. I will let you know if I need some help :)

thank's!

Freyskeyd

Can I have your opinion on the reference modification please? cc @carols10cents

Freyskeyd · 2017-04-19T12:42:56Z

src/test/ui/resolve/privacy-struct-ctor.stderr

@@ -5,6 +5,7 @@ error[E0423]: expected value, found struct `Z`
   |         ^
   |         |
   |         did you mean `Z { /* fields */ }`?
+   |         did you mean `S`?


It's unrevelante here. Right?

Are you asking if it's ok that your code change changes these suggestions? I'm not sure :-/ since nrc is on paternity leave right now... @jonathandturner are you the next best person to help out around error message suggestions?

yes, I'm asking if it's ok to update that part of suggestion or not. We keep both suggest but is it valid to tell user that, maybe, he misstyped it?

It's ok, but Levenshtein suggestions should probably be turned off globally for single-letter identifiers (not necessarily in this PR).

petrochenkov · 2017-04-19T14:27:47Z

src/test/ui/resolve/levenshtein.stderr

-   |          ^^^ did you mean `Bar`?
+   |          ^^^
+   |          |
+   |          not found in this scope


Fall-back labels should not be reported if any other labels are reported.

Hi @petrochenkov, could you clarify a bit on how @Freyskeyd would fix this so that fall-back labels aren't reported?

#39291 (comment)

petrochenkov · 2017-04-19T14:28:57Z

src/librustc_resolve/lib.rs

+            // Try Levenshtein if nothing else worked.
+            if let Some(candidate) = this.lookup_typo_candidate(path, ns, is_expected) {
+                err.span_label(ident_span, &format!("did you mean `{}`?", candidate));
+                // return err;


Could you also remove this return instead of commenting it out?

@petrochenkov Yep, I just let it here to validate behavior with you.

arielb1 · 2017-04-25T09:31:49Z

ping @petrochenkov & @Freyskeyd - making sure this PR doesn't disappear.

Freyskeyd · 2017-04-25T09:32:54Z

@arielb1 I'm working on it^^

petrochenkov · 2017-04-28T15:35:47Z

src/librustc_resolve/lib.rs

            // Fallback label.
-            err.span_label(base_span, &fallback_label);
+            if err.span.span_labels().is_empty() {


@Freyskeyd
IIRC, span_labels().is_empty() had quirks and didn't work as expected.
From the tests it looks like it still doesn't work.
You can just set some boolean flag if Levenshtein worked and check it here.

petrochenkov · 2017-04-28T15:38:14Z

@Freyskeyd
GitHub doesn't seem to send notifications on rebases, so I missed the update, sorry.
Could you leave some comment here after the next update? I'll r+ immediately then.

Signed-off-by: Freyskeyd <simon.paitrault@gmail.com>

Freyskeyd · 2017-04-28T18:41:02Z

ping @petrochenkov @carols10cents :)

petrochenkov · 2017-04-29T14:25:12Z

@bors r+
Thanks!

bors · 2017-04-29T14:25:12Z

📌 Commit 0d7e6cf has been approved by petrochenkov

bors · 2017-04-29T15:18:57Z

⌛ Testing commit 0d7e6cf with merge b4d3ed6...

@nagisa

Checker:: Execute levenshtein before other context checking As explain [here]() i think it's better to check for a miss typing before checking context dependent help. ```rust struct Handle {} struct Something { handle: Handle } fn main() { let handle: Handle = Handle {}; let s: Something = Something { // Checker detect an error and propose a solution with `Handle { /* ... */ }` // but it's a miss typing of `handle` handle: Handle }; } ``` Ping: @nagisa for #39226 Signed-off-by: Freyskeyd <simon.paitrault@gmail.com>

bors · 2017-04-29T17:42:01Z

☀️ Test successful - status-appveyor, status-travis
Approved by: petrochenkov
Pushing b4d3ed6 to master...

rust-highfive assigned arielb1 Jan 25, 2017

Freyskeyd force-pushed the check_context_E0423 branch 3 times, most recently from a4b67d3 to eece90f Compare March 9, 2017 13:22

Freyskeyd force-pushed the check_context_E0423 branch 3 times, most recently from 46d4f1a to dc56bb1 Compare March 13, 2017 08:29

rust-highfive assigned nrc and unassigned arielb1 Apr 13, 2017

Mark-Simulacrum added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 14, 2017

carols10cents 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 Apr 17, 2017

Freyskeyd force-pushed the check_context_E0423 branch 4 times, most recently from 0f3dbd7 to c487396 Compare April 19, 2017 12:38

Freyskeyd commented Apr 19, 2017

View reviewed changes

petrochenkov reviewed Apr 19, 2017

View reviewed changes

Freyskeyd force-pushed the check_context_E0423 branch from c487396 to 75b550d Compare April 25, 2017 10:33

petrochenkov assigned petrochenkov and unassigned nrc Apr 25, 2017

Freyskeyd force-pushed the check_context_E0423 branch 3 times, most recently from 76e3faa to 5379203 Compare April 28, 2017 15:00

petrochenkov reviewed Apr 28, 2017

View reviewed changes

refac Move levenchstein before context dependent check

0d7e6cf

Signed-off-by: Freyskeyd <simon.paitrault@gmail.com>

Freyskeyd force-pushed the check_context_E0423 branch from 5379203 to 0d7e6cf Compare April 28, 2017 18:40

bors merged commit 0d7e6cf into rust-lang:master Apr 29, 2017

Freyskeyd deleted the check_context_E0423 branch April 29, 2017 18:43

Freyskeyd mentioned this pull request May 1, 2017

Suggests to initialize struct with private fields, instead of looking for typo #39226

Closed

Checker:: Execute levenshtein before other context checking #39291

Checker:: Execute levenshtein before other context checking #39291

Conversation

Freyskeyd commented Jan 25, 2017 • edited Loading

rust-highfive commented Jan 25, 2017

nagisa commented Jan 25, 2017

nagisa commented Jan 25, 2017

Freyskeyd commented Jan 25, 2017

petrochenkov commented Jan 25, 2017

petrochenkov commented Jan 25, 2017

mrhota commented Feb 23, 2017

Freyskeyd commented Feb 24, 2017

arielb1 commented Mar 7, 2017 • edited Loading

bors commented Mar 7, 2017

bors commented Mar 7, 2017

bors commented Mar 7, 2017

bors commented Mar 9, 2017

bors commented Mar 12, 2017

alexcrichton commented Apr 1, 2017

leoyvens commented Apr 10, 2017

arielb1 commented Apr 13, 2017

carols10cents commented Apr 17, 2017

Freyskeyd commented Apr 18, 2017

Freyskeyd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielb1 commented Apr 25, 2017

Freyskeyd commented Apr 25, 2017

Choose a reason for hiding this comment

petrochenkov commented Apr 28, 2017

Freyskeyd commented Apr 28, 2017

petrochenkov commented Apr 29, 2017

bors commented Apr 29, 2017

bors commented Apr 29, 2017

bors commented Apr 29, 2017

Freyskeyd commented Jan 25, 2017 •

edited

Loading

arielb1 commented Mar 7, 2017 •

edited

Loading