From 92e104be10d2cf2669fd4ad214c179d92b3618e1 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Thu, 23 Feb 2017 15:55:53 +0100 Subject: [PATCH 1/8] Add guesses RFC --- text/0000-guesses.md | 238 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 238 insertions(+) create mode 100644 text/0000-guesses.md diff --git a/text/0000-guesses.md b/text/0000-guesses.md new file mode 100644 index 00000000000..104da9a0238 --- /dev/null +++ b/text/0000-guesses.md @@ -0,0 +1,238 @@ +- Feature Name: guess_diagnostic +- Start Date: 2017-02-23 +- RFC PR: (leave this empty) +- Rust Issue: (leave this empty) + +# Summary +[summary]: #summary + +Add a new kind of compiler diagnostic ("guess"), to distinguish "suggestions" +from code snippets that are found by a heuristic or have multiple possible +solutions. Change all current "suggestions" which are not guaranteed to be correct +to "guesses". + +# Motivation +[motivation]: #motivation + +TLDR: `rustfix` should be the semantic companion to `rustfmt` (which is purely syntactical) and automatically change code to be more idiomatic. + +Clippy (and some compiler builtin lints) produce "suggestions", which are code +snippets that can be copy pasted over a part of the linted code to replace, +enhance or remove the detected suboptimal piece of code. In some cases there is +a clearly correct solution, like removing one ampersand on the rhs of the following +let binding (due to it being automatically dereferenced by the compiler anyway): + +```rust +let x: &i32 = &&5; +``` + +This would be a situation where a "suggestion" would be used, since there is no +possible ambiguity or disadvantage to having a tool (e.g. `rustfix`) automatically +remove the ampersand. + +When there is no clear solution (e.g. when the user references a type `Iter`, +but there is no such type in the scope) heuristics can supply "guesses" (e.g. +right now, the compiler supplies an unstructured list of types called `Iter`). + +# Detailed design +[design]: #detailed-design + +## Diagnostics frontend API changes + +The compiler diagnostics API is extended with the following methods: + +```rust +pub fn span_guesses(&mut self, sp: Span, msg: &str, guesses: I) -> &mut Self where I: IntoIterator; +pub fn span_guess(&mut self, sp: Span, msg: &str, guess: String) -> &mut Self { + self.span_guesses(sp, msg, ::std::iter::once(guess)) +} +``` + +`span_guess` is just a convenience method which forwards to `span_guesses`. + +`span_guesses` takes an iterator of guesses to be presented to the user. +The span `sp` is the region of code which will be replaced by one of the code snippets +given by the `guesses` iterator. The span can be zero bytes long, if the text is supposed +to be inserted (e.g. adding a new `use xyz::abc;` at the top of the file) instead of +replacing existing code. + +If multispan replacements (replacing multiple separate spans at once, e.g. when modifying the +pattern in a for loop together with the object being iterated over) are desired, +one should modify the `Diagnostic` object manually. This might be inconvenient, but +multispan replacements are very rare (occurring exactly +[twice in clippy](https://github.com/Manishearth/rust-clippy/search?utf8=%E2%9C%93&q=multispan_sugg)) +and therefor the API should be figured out once/if they become more common. + +## Changes in diagnostic API usage + +### Replace `suggestions` with `guesses` + +All current calls to `span_suggestion` are replaced with `span_guess` if +their replacement is obtained by a heuristic. A new type of tests is added: "suggestion". Tests in the "suggestion" directory must be failing +with an error (or denied lint) and contain a suggestion (not required to +be emitted by the error or lint failing the test). Then the suggestion is +applied, and the test must now compile and run successfully. + +### Change `help` messages containing code snippets to `guesses` + +Whenever a `span_help` is passed a snippet or hand built expression, +it is changed into a `span_guess` or `span_guesses` if multiple helps +are generated in some form of a loop. If `span_guesses` is used, all +arbitrary limits on the number of displayed items is removed. This limit +is instead enforced by the command line diagnostic backend. + +## Json diagnostics backend API changes + +The json object gets a new field `guesses` which is an array of +`Guess` objects. Every `Guess` object contains an array of span + +snippet pairs stating which part of the code should be replaced +with what replacment. There is no need for all guesses to replace +the same piece of code or even require the same number of replacements. + +## Command line diagnostics backend API changes + +Currently suggestions are inserted directly into the error structure as a "help" sub-diagnostic +of the main error, with a special `Option` field set for the replacement text. +This means that backends like the existing command line backend and json backend will need to +process the main error's sub-diagnostics to figure out how to treat the suggestion and extract +that information back out of the sub-diagnostic. + +The backend API is changed to add a `code_hints` field to the main diagnostic, which contains +an enum with a `Suggestion` and a `Guesses` variant. The actual backend will then destructure +these variants and apply them as they see fit best. An implementation of this backend change +exists at https://github.com/rust-lang/rust/pull/39973 (does not implement guesses yet). + +The command line backend will print guesses as a sequence of "help" messages, just like in the following example + +``` +error[E0412]: type name `Iter` is undefined or not in scope + --> :4:12 + | +4 | let t: Iter; + | ^^^^ undefined or not in scope + | + = help: you can import several candidates into scope (`use ...;`): + = help: `std::collections::binary_heap::Iter` + = help: `std::collections::btree_map::Iter` + = help: `std::collections::btree_set::Iter` + = help: `std::collections::hash_map::Iter` + = help: and 8 other candidates +``` + +With the only change being that the `use ...;` is directly inserted into the help messages as shown below. +This has precedent in the r + +``` +error[E0412]: type name `Iter` is undefined or not in scope + --> :4:12 + | +4 | let t: Iter; + | ^^^^ undefined or not in scope + | + = help: you can import several candidates into scope: + = help: `use std::collections::binary_heap::Iter;` + = help: `use std::collections::btree_map::Iter;` + = help: `use std::collections::btree_set::Iter;` + = help: `use std::collections::hash_map::Iter;` + = help: and 8 other candidates +``` + +The actual span where the guess should be inserted is not shown in the command line diagnostics and has +no influence on the display of the guess. + +### Special case: Only one guess + +In case there is only a single guess, there are multiple variants on what can happen. + +* replacement contains `\n` + * display in an extra `help` +* No `note` attached to the main diagnostic + * span of the replacement is the same as the main diagnostic + * attach the replacement string to the guess message + ``` + 42 | abc + | ^^^ did you mean `abcd` + ``` + * span of the replacement is part of the main diagnostic's span + * attach the replacement string to the guess message, even if it's technically wrong + ``` + 42 | Opition + | ^^^^^^^^^^^ did you mean `Option` + * span of the replacement contains the main diagnostic's span + * expand the displayed span to the entire span which should be replaced + ``` + 42 | a.I + | ^^^ did you mean `a::I` + ``` +* `note` already attched to the main diagnostic + * Attach the guess below the note according tho the no-note rules (as if the note were the main diagnostic) + ``` + 5 | ignore(|z| if false { &y } else { z }); + | ^^^ - `y` is borrowed here + | | + | may outlive borrowed value `y` + | did you mean `move |z|` + ``` + +# How We Teach This +[how-we-teach-this]: #how-we-teach-this + + + +What names and terminology work best for these concepts and why? +How is this idea best presented—as a continuation of existing Rust patterns, or as a wholly new one? + +Would the acceptance of this proposal change how Rust is taught to new users at any level? +How should this feature be introduced and taught to existing Rust users? + +What additions or changes to the Rust Reference, _The Rust Programming Language_, and/or _Rust by Example_ does it entail? + +# Drawbacks +[drawbacks]: #drawbacks + +## The new "guess" diagnostic category does not add any benefit + +Citing @nrc in https://github.com/rust-lang/rust/pull/39458#issuecomment-277898885 + +> My work flow for 'automatic application' is that the user opts in to doing this +> in an IDE or rustfix tool, possibly with some further input. +> I don't think there is a need or expectation that after such an application +> the resulting code is guaranteed to be logically correct or even to compile. +> So, I think a suggestion with placeholders would be fine as would a suggestion with multiple choices. +> In other words, I'd rather make suggestions more flexible than add another category of error. + +## JSON and plain text error messages might differ + +Plain text will output many simple guesses as notes on the relevant span (e.g. "did you mean `abcd`") +instead of showing them as full suggestions like: + +``` +42 | abc + | ^^^ unknown identifier +help: did you mean `abcd`? + | +42 | abcd +``` + +It would be confusing for users to get semantically differend kinds of messages depending on how they view the errors. + + +# Alternatives +[alternatives]: #alternatives + +## Don't add a new category, just make everything a suggestion + +This will make it impossible for automatic semantic code rewriting. + +## Don't add a new category, but allow adding multiple suggestions + +This allows changing "help" messages like "did you mean ..." to suggestions, if there are multiple things that apply. + +## Also apply all single-guess rules to suggestions to reduce error verbosity + +This can be done as a later step to improve suggestions + +# Unresolved questions +[unresolved]: #unresolved-questions + +1. Test whether all compiler suggestions are correct by applying them to the run-pass tests (or a new test folder) and checking whether the code still works. From be6fa2db6b38935238a07a6e73c3bb0f06b5397a Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Thu, 2 Mar 2017 20:15:05 +0100 Subject: [PATCH 2/8] Address first round of comments --- text/0000-guesses.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/text/0000-guesses.md b/text/0000-guesses.md index 104da9a0238..207111800e1 100644 --- a/text/0000-guesses.md +++ b/text/0000-guesses.md @@ -9,7 +9,8 @@ Add a new kind of compiler diagnostic ("guess"), to distinguish "suggestions" from code snippets that are found by a heuristic or have multiple possible solutions. Change all current "suggestions" which are not guaranteed to be correct -to "guesses". +to "guesses" or make them not emit a suggestion in the corner cases that would +yield incorrect suggestions. # Motivation [motivation]: #motivation @@ -61,7 +62,7 @@ pattern in a for loop together with the object being iterated over) are desired, one should modify the `Diagnostic` object manually. This might be inconvenient, but multispan replacements are very rare (occurring exactly [twice in clippy](https://github.com/Manishearth/rust-clippy/search?utf8=%E2%9C%93&q=multispan_sugg)) -and therefor the API should be figured out once/if they become more common. +and therefore the API should be figured out once/if they become more common. ## Changes in diagnostic API usage From 417c1acff5b17b3d2efe530eb32e495378d9f672 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Fri, 3 Mar 2017 00:20:02 +0100 Subject: [PATCH 3/8] Update 0000-guesses.md --- text/0000-guesses.md | 35 +++++++++++++++++++++++++---------- 1 file changed, 25 insertions(+), 10 deletions(-) diff --git a/text/0000-guesses.md b/text/0000-guesses.md index 207111800e1..ca43bf14654 100644 --- a/text/0000-guesses.md +++ b/text/0000-guesses.md @@ -21,7 +21,7 @@ Clippy (and some compiler builtin lints) produce "suggestions", which are code snippets that can be copy pasted over a part of the linted code to replace, enhance or remove the detected suboptimal piece of code. In some cases there is a clearly correct solution, like removing one ampersand on the rhs of the following -let binding (due to it being automatically dereferenced by the compiler anyway): +`let` binding (due to it being automatically dereferenced by the compiler anyway): ```rust let x: &i32 = &&5; @@ -101,7 +101,7 @@ that information back out of the sub-diagnostic. The backend API is changed to add a `code_hints` field to the main diagnostic, which contains an enum with a `Suggestion` and a `Guesses` variant. The actual backend will then destructure these variants and apply them as they see fit best. An implementation of this backend change -exists at https://github.com/rust-lang/rust/pull/39973 (does not implement guesses yet). +exists [in a rust PR](https://github.com/rust-lang/rust/pull/39973) (does not implement guesses yet). The command line backend will print guesses as a sequence of "help" messages, just like in the following example @@ -121,7 +121,14 @@ error[E0412]: type name `Iter` is undefined or not in scope ``` With the only change being that the `use ...;` is directly inserted into the help messages as shown below. -This has precedent in the r +This has precedent in the diagnostics for unimported traits: + +``` + = help: items from traits can only be used if the trait is in scope; the following trait is implemented but not in scope, perhaps add a `use` for it: + = help: candidate #1: `use std::borrow::Borrow;` +``` + +For the import guesses this would look like ``` error[E0412]: type name `Iter` is undefined or not in scope @@ -178,22 +185,30 @@ In case there is only a single guess, there are multiple variants on what can ha # How We Teach This [how-we-teach-this]: #how-we-teach-this +There is concern for the teachability of all the different diagnostic levels of rustc. The full current list is +* fatal + * Bug (panic and abort) + * Error (abort at the next convenient stage, but continue with reporting more diagnostics) +* Warning (print and continue) +* sub-diagnostic + * Help (print in its own line and continue) + * Note (print inline in the textual output right after the `^^^^^` under the span) +* code-hints + * Suggestion (print like a help) -What names and terminology work best for these concepts and why? -How is this idea best presented—as a continuation of existing Rust patterns, or as a wholly new one? +This list would get extended by Guesses under the code hint category -Would the acceptance of this proposal change how Rust is taught to new users at any level? -How should this feature be introduced and taught to existing Rust users? +The complexity is not in the diagnostic levels, but in choosing the correct level for a situation. This is fairly straight-forward for errors vs warnings, but in the sub-diagnostic and code-hint levels, it is less clear. -What additions or changes to the Rust Reference, _The Rust Programming Language_, and/or _Rust by Example_ does it entail? +TODO: add decision graph # Drawbacks [drawbacks]: #drawbacks ## The new "guess" diagnostic category does not add any benefit -Citing @nrc in https://github.com/rust-lang/rust/pull/39458#issuecomment-277898885 +Citing @nrc in [the original pull request](https://github.com/rust-lang/rust/pull/39458#issuecomment-277898885) > My work flow for 'automatic application' is that the user opts in to doing this > in an IDE or rustfix tool, possibly with some further input. @@ -236,4 +251,4 @@ This can be done as a later step to improve suggestions # Unresolved questions [unresolved]: #unresolved-questions -1. Test whether all compiler suggestions are correct by applying them to the run-pass tests (or a new test folder) and checking whether the code still works. +1. I could not come up with a nice API for multiple guesses each with multiple spans From aed4d3898334c265ea76ad260a7573615f87ce38 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Fri, 3 Mar 2017 15:27:33 +0100 Subject: [PATCH 4/8] Add decision graph for diagnostics graphviz dot code: ```dot digraph G { A [ label = "Does the message contain\ncode snippets derived\nfrom real code?"] A -> B [ label = "Yes"] A -> C [ label = "No"] B [ label = "Are there multiple\nsnippets that may apply?"] B -> "Guess" [ label = "Yes" ] B -> D [ label = "No" ] D [ label = "Are there situations\nwhere the snippet is plain wrong?"] D -> E [ label = "Yes" ] D -> "Suggestion" [ label = "No" ] E [ label = "Can you detect those\nsituations programmatically?"] E -> G [ label = "Yes" ] E -> "Guess" [ label = "No" ] G [ label = "Suggestion where correct,\nGuess or no code hint otherwise"] C [ label = "Is the message reasonably short?"] C -> "Note" [ label = "Yes" ] C -> "Help" [ label = "No" ] } ``` --- text/0000-guesses.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0000-guesses.md b/text/0000-guesses.md index ca43bf14654..504d16c5bde 100644 --- a/text/0000-guesses.md +++ b/text/0000-guesses.md @@ -201,7 +201,7 @@ This list would get extended by Guesses under the code hint category The complexity is not in the diagnostic levels, but in choosing the correct level for a situation. This is fairly straight-forward for errors vs warnings, but in the sub-diagnostic and code-hint levels, it is less clear. -TODO: add decision graph +![chart](https://cloud.githubusercontent.com/assets/332036/23554529/be228b76-0025-11e7-99e7-627d60f5a328.png) # Drawbacks [drawbacks]: #drawbacks From 1f45338fe6bb65a638b5994061ac8585e2cd6f73 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Fri, 3 Mar 2017 15:28:39 +0100 Subject: [PATCH 5/8] Update 0000-guesses.md --- text/0000-guesses.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/text/0000-guesses.md b/text/0000-guesses.md index 504d16c5bde..acc7d9dba2c 100644 --- a/text/0000-guesses.md +++ b/text/0000-guesses.md @@ -199,7 +199,9 @@ There is concern for the teachability of all the different diagnostic levels of This list would get extended by Guesses under the code hint category -The complexity is not in the diagnostic levels, but in choosing the correct level for a situation. This is fairly straight-forward for errors vs warnings, but in the sub-diagnostic and code-hint levels, it is less clear. +The complexity is not in the diagnostic levels themselves, but in choosing the correct level for a situation. This is fairly straight-forward for errors vs warnings, but in the sub-diagnostic and code-hint levels, it is less clear. + +The following chart should clarify the decision process for subdiagnostics ![chart](https://cloud.githubusercontent.com/assets/332036/23554529/be228b76-0025-11e7-99e7-627d60f5a328.png) From 259d6dea8d1e5a83dfd9a7892e752adb51257a59 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Fri, 3 Mar 2017 15:30:32 +0100 Subject: [PATCH 6/8] Fix typo --- text/0000-guesses.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0000-guesses.md b/text/0000-guesses.md index acc7d9dba2c..ec5df73d348 100644 --- a/text/0000-guesses.md +++ b/text/0000-guesses.md @@ -173,7 +173,7 @@ In case there is only a single guess, there are multiple variants on what can ha | ^^^ did you mean `a::I` ``` * `note` already attched to the main diagnostic - * Attach the guess below the note according tho the no-note rules (as if the note were the main diagnostic) + * Attach the guess below the note according to the no-note rules (as if the note were the main diagnostic) ``` 5 | ignore(|z| if false { &y } else { z }); | ^^^ - `y` is borrowed here From 270293cfe0bba797d10a75960b6384d797f4fde7 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Fri, 3 Mar 2017 15:40:15 +0100 Subject: [PATCH 7/8] Update 0000-guesses.md --- text/0000-guesses.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/text/0000-guesses.md b/text/0000-guesses.md index ec5df73d348..26c47fdd06a 100644 --- a/text/0000-guesses.md +++ b/text/0000-guesses.md @@ -246,11 +246,13 @@ This will make it impossible for automatic semantic code rewriting. This allows changing "help" messages like "did you mean ..." to suggestions, if there are multiple things that apply. -## Also apply all single-guess rules to suggestions to reduce error verbosity +## Add a new "replacement" category -This can be done as a later step to improve suggestions +Denotes the automatically applicable code hints, while suggestions stay as the heuristic category. +This is just nomenclature, but eases the transition, since nothing needs to be changed in existing `span_suggestion` calls. # Unresolved questions [unresolved]: #unresolved-questions 1. I could not come up with a nice API for multiple guesses each with multiple spans +2. Also apply all single-guess rules to suggestions to reduce error verbosity? This can be done later. From 8814b49601ac78d7a851c80ce028d0fa67112a77 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Fri, 3 Mar 2017 18:43:51 +0100 Subject: [PATCH 8/8] Change json to JSON --- text/0000-guesses.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/text/0000-guesses.md b/text/0000-guesses.md index 26c47fdd06a..35c9ed4b957 100644 --- a/text/0000-guesses.md +++ b/text/0000-guesses.md @@ -82,9 +82,9 @@ are generated in some form of a loop. If `span_guesses` is used, all arbitrary limits on the number of displayed items is removed. This limit is instead enforced by the command line diagnostic backend. -## Json diagnostics backend API changes +## JSON diagnostics backend API changes -The json object gets a new field `guesses` which is an array of +The JSON object gets a new field `guesses` which is an array of `Guess` objects. Every `Guess` object contains an array of span + snippet pairs stating which part of the code should be replaced with what replacment. There is no need for all guesses to replace @@ -94,7 +94,7 @@ the same piece of code or even require the same number of replacements. Currently suggestions are inserted directly into the error structure as a "help" sub-diagnostic of the main error, with a special `Option` field set for the replacement text. -This means that backends like the existing command line backend and json backend will need to +This means that backends like the existing command line backend and JSON backend will need to process the main error's sub-diagnostics to figure out how to treat the suggestion and extract that information back out of the sub-diagnostic.