Detect `=` -> `:` typo in let bindings #45452

estebank · 2017-10-22T17:24:28Z

When encountering a let binding type error, attempt to parse as
initializer instead. If successful, it is likely just a typo:

fn main() {
    let x: Vec::with_capacity(10);
}

error: expected type, found `10`
 --> file.rs:3:31
  |
3 |     let x: Vec::with_capacity(10, 20);
  |         --                    ^^
  |         ||
  |         |help: did you mean assign here?: `=`
  |         while parsing the type for `x`

Fix #43703.

rust-highfive · 2017-10-22T17:24:38Z

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

nikomatsakis

r=me modulo the fact that it displays ?:

nikomatsakis · 2017-10-23T17:02:50Z

src/test/ui/suggestions/type-ascription-instead-of-initializer.stderr

+12 |     let x: Vec::with_capacity(10, 20);
+   |         --                    ^^
+   |         ||
+   |         |help: did you mean assign here?: `=`


The ?: looks funny, don't we have some convention for dealing with that? cc @oli-obk

Yea:

rust/src/librustc_errors/diagnostic.rs

Lines 231 to 236 in 9ae6ed7

/// The message

/// * should not end in any punctuation (a `:` is added automatically)

/// * should not be a question

/// * should not contain any parts like "the following", "as shown"

/// * may look like "to do xyz, use" or "to do xyz, use abc"

/// * may contain a name of a function, variable or type, but not whole expressions

nikomatsakis · 2017-10-23T17:04:43Z

src/libsyntax/parse/parser.rs

+                err.emit();
+                // As this was parsed successfuly, continue as if the code has been fixed for the
+                // rest of the file. It will still fail due to the emitted error, but we avoid
+                // extra noise.


nikomatsakis · 2017-10-23T17:06:57Z

src/libsyntax/parse/parser.rs

+        let (err, ty) = if self.eat(&token::Colon) {
+            // Save the state of the parser before parsing type normally, in case there is a `:`
+            // instead of an `=` typo.
+            let parser_snapshot_before_type = self.clone();


Is this something we do elsewhere? Maybe we should find some way to make it more obvious that people might be cloning the parser like this? It sort of surprises me, but I think it makes sense, and I can't see any obvious reasons it will cause problems.

I believe the first time something like this was introduced to the parser was in #42578 to verify wether a as usize < b could be parsed and if it could, emit a suggestion for the correct code (a as usize) < b. It might have been used elsewhere since. I thought about adding a method to the parser but balked at the idea to not give the impression that we should use of this error recovery style everywhere.

petrochenkov · 2017-10-23T17:33:30Z

I dislike this continuing tendency of issuing custom diagnostics for all possible accidental typos and mistakes without any account for probability of them occurring in practice.
The number of possible typos is unlimited, so diagnostics added to the compiler are based on whether someone bothered to submit an issue for a specific mistake or not.
It's not necessary to fulfill all A-diagnostics requests, many of them would be better closed as WONTFIX instead, especially if they require the compiler to go out of its way somehow, e.g. doing parser snapshots on happy path even if the mistake almost never happens.
We should rather attempt to make more generic error reporting, applicable to wider range of situations, better.

</nonblocking rant>

estebank · 2017-10-24T13:48:11Z

I dislike this continuing tendency of issuing custom diagnostics for all possible accidental typos and mistakes without any account for probability of them occurring in practice.

Sadly, other than adding remote logging to the compiler, all we can rely on is our own impressions, experience and anecdotal evidence on how often some errors happen to make those decisions. That being said, I understand the dislike for making the compiler more complex for very small corner cases.

The number of possible typos is unlimited, so diagnostics added to the compiler are based on whether someone bothered to submit an issue for a specific mistake or not.

This is true, and it would be fool hardy endeavor to even try. That being said, I wouldn't be surprised if there's a funnel for filed tickets of "1 filed <- 10 affected <- 1000 annoyed". This PR in particular "fixes" the issue filed in a general enough way that doesn't necessarily help experienced developers (they know where to look), but helps heaps to newcomers by pointing them in the right direction by going "Hey! The error happens there, but look over here too!". Can this become annoyingly verbose? Sure, and there's a conversation to be had about reducing the verbosity of existing errors, but don't underplay how useful it can be for newcomers to go out of our way to explain anything remotely non-obvious.

[aside] This makes me think, may be there would be value to have a mode where the spans still appear but the label text and suggestions are not shown, for experienced devs. An intermediary between current output and "short messages". In this case, something along the lines of:

error: expected type, found `10`
 --> file.rs:3:31
  |
3 |     let x: Vec::with_capacity(10, 20);
  |          -                    ^^

It's not necessary to fulfill all A-diagnostics requests, many of them would be better closed as WONTFIX instead, especially if they require the compiler to go out of its way somehow, e.g. doing parser snapshots on happy path even if the mistake almost never happens.

#42578 is an example of a diagnostic that could not be accomplished as cleanly otherwise, and that remediates a problem has tripped multiple people before. It is definitely a corner case, but the usability gain is in my eyes clear.

We should rather attempt to make more generic error reporting, applicable to wider range of situations, better.

I agree with this, but I disagree that for example this PR doesn't do that. Sure, I'm adding the rollback machinery to be able to infer that what was written as the binding's type could be parsed as the initializer, but in the general case it also points out to the binding saying "The parse error you've got over there happened while looking at this bindings type", which is much more generally useful, specially when dealing with code spanning multiple lines.

I would also go as far as to claim that even when improving the general errors is always a great use of our time, even if this is sometimes just not possible, covering a wide range of small corner cases with very localized and relevant error messages can change the user experience from "this is hard, but understandable" to "this is magical".

The universe of possible typos is unlimited, but the amount of likely typos and mistakes is much more constrained. The ones I've seen most often in person and in the issue tracker fall under one of two categories: "keys close together", for which we shouldn't be doing anything special over what we're already doing, and "rust/other language mismatch", like Struct.new() instead of Struct::new(), which I think we should.

I would like to have a longer conversation on this in order to hear more of your thoughts.

nikomatsakis · 2017-10-25T13:27:23Z

One thing I've been really wanting to do that's related here is to see some progress on "telemetry". That is, I think we need some way to log information about what people have tried to compile, what error messages they've gotten, and how they responded to these error messages so that we can collect data. Obviously this would need to be an "opt-in" and transparent process, but I imagine many people would happily agree. That would be tremendously useful in helping us gauge priorities.

I'd love to see some concerted effort here -- probably the best thing would be to start super small. i.e., have some website where people can copy-and-paste an error message that they found confusing and hit a simple button. But it's going to take effort on the back-end to triage, categorize, and diagnose this sort of thing as well.

In my ideal world, this would be the domain of the compiler error messages Working Group long term. =)

nikomatsakis · 2017-11-01T13:42:13Z

Well, let's make a final decision here! I don't really know if this is a common source of confusion, but it certainly seems to me like the kind of thing that one could stare at for hours before having a 🤦‍♀️ moment, so I'm inclined to r+, but also to (independently) make a real effort towards trying to get some data on which to base future decisions.

When encountering a let binding type error, attempt to parse as initializer instead. If successful, it is likely just a typo: ```rust fn main() { let x: Vec::with_capacity(10); } ``` ``` error: expected type, found `10` --> file.rs:3:31 | 3 | let x: Vec::with_capacity(10, 20); | -- ^^ | || | |help: did you mean assign here?: `=` | while parsing the type for `x` ```

estebank · 2017-11-08T02:45:41Z

@nikomatsakis created #45857 to track the telemetry talk. Are there any further changes I should make to this PR?

nikomatsakis · 2017-11-08T17:53:15Z

@bors r+

bors · 2017-11-08T17:53:16Z

📌 Commit 9dc7abe has been approved by nikomatsakis

bors · 2017-11-08T20:00:47Z

⌛ Testing commit 9dc7abe with merge 02004ef...

Detect `=` -> `:` typo in let bindings When encountering a let binding type error, attempt to parse as initializer instead. If successful, it is likely just a typo: ```rust fn main() { let x: Vec::with_capacity(10); } ``` ``` error: expected type, found `10` --> file.rs:3:31 | 3 | let x: Vec::with_capacity(10, 20); | -- ^^ | || | |help: did you mean assign here?: `=` | while parsing the type for `x` ``` Fix #43703.

bors · 2017-11-08T22:27:00Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 02004ef to master...

rust-highfive assigned nikomatsakis Oct 22, 2017

estebank force-pushed the colon-typo branch from 5288e9e to 6646254 Compare October 22, 2017 17:35

kennytm added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 22, 2017

nikomatsakis requested changes Oct 23, 2017

View reviewed changes

estebank force-pushed the colon-typo branch from 6646254 to 7a0f051 Compare November 2, 2017 20:26

kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 2, 2017

estebank force-pushed the colon-typo branch from 7a0f051 to cd32bad Compare November 3, 2017 22:09

estebank force-pushed the colon-typo branch from cd32bad to 9dc7abe Compare November 4, 2017 00:39

estebank mentioned this pull request Nov 8, 2017

Provide a way to get feedback on compiler error rates #45857

Open

bors merged commit 9dc7abe into rust-lang:master Nov 8, 2017

est31 mentioned this pull request Dec 1, 2017

Mismatched type error: type of crate instead of std is displayed #46112

Closed

estebank deleted the colon-typo branch November 9, 2023 05:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Detect `=` -> `:` typo in let bindings #45452

Detect `=` -> `:` typo in let bindings #45452

estebank commented Oct 22, 2017

rust-highfive commented Oct 22, 2017

nikomatsakis left a comment

nikomatsakis Oct 23, 2017

oli-obk Oct 24, 2017

nikomatsakis Oct 23, 2017

nikomatsakis Oct 23, 2017

estebank Oct 24, 2017

petrochenkov commented Oct 23, 2017 •

edited

Loading

estebank commented Oct 24, 2017

nikomatsakis commented Oct 25, 2017

nikomatsakis commented Nov 1, 2017

estebank commented Nov 8, 2017

nikomatsakis commented Nov 8, 2017

bors commented Nov 8, 2017

bors commented Nov 8, 2017

bors commented Nov 8, 2017

	/// The message
	/// * should not end in any punctuation (a `:` is added automatically)
	/// * should not be a question
	/// * should not contain any parts like "the following", "as shown"
	/// * may look like "to do xyz, use" or "to do xyz, use abc"
	/// * may contain a name of a function, variable or type, but not whole expressions

Detect = -> : typo in let bindings #45452

Detect = -> : typo in let bindings #45452

Conversation

estebank commented Oct 22, 2017

rust-highfive commented Oct 22, 2017

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis Oct 23, 2017

Choose a reason for hiding this comment

oli-obk Oct 24, 2017

Choose a reason for hiding this comment

nikomatsakis Oct 23, 2017

Choose a reason for hiding this comment

nikomatsakis Oct 23, 2017

Choose a reason for hiding this comment

estebank Oct 24, 2017

Choose a reason for hiding this comment

petrochenkov commented Oct 23, 2017 • edited Loading

estebank commented Oct 24, 2017

nikomatsakis commented Oct 25, 2017

nikomatsakis commented Nov 1, 2017

estebank commented Nov 8, 2017

nikomatsakis commented Nov 8, 2017

bors commented Nov 8, 2017

bors commented Nov 8, 2017

bors commented Nov 8, 2017

Detect `=` -> `:` typo in let bindings #45452

Detect `=` -> `:` typo in let bindings #45452

petrochenkov commented Oct 23, 2017 •

edited

Loading