Refactor suggestion diagnostic API to allow for multiple suggestions #41876

oli-obk · 2017-05-10T11:22:19Z

oli-obk · 2017-05-10T17:40:15Z

The failure is spurious

sophiajt · 2017-05-11T00:45:47Z

The design looks cleaner than what we had before, so design-wise r=me, but it's been a while since I hacked on the compiler. @nrc might be a good one to look at the code, or possibly @arielb1

nrc · 2017-05-11T01:01:22Z

seems fine to me, I haven't done a detailed review, but the API looks good

nagisa · 2017-05-11T06:51:44Z

src/librustc_errors/emitter.rs

+            // to be consistent. We could try to figure out if we can
+            // make one (or the first one) inline, but that would give
+            // undue importance to a semi-random suggestion
+            for sugg in &db.suggestions {


This duplicates the code somewhat. Consider doing this instead:

if let Some((sugg, rest)) = db.suggestions.split_first() { if rest.is_empty() && sugg.substitutes.len() == 1 && ... { show `sugg` as label } else { for sugg in &db.suggestions { show `sugg` proper } } }

I did not know about split_first. That's a great method.

nagisa · 2017-05-11T06:55:30Z

src/librustc_errors/lib.rs

+    /// ```
+    /// vec![(0..7, vec!["a.b", "x.y"])]
+    /// ```
+    pub substitutes: Vec<(Span, Vec<String>)>,


This makes code fairly ugly. Can we just get a struct with named fields for this?

nagisa · 2017-05-11T07:06:55Z

src/librustc_errors/emitter.rs

@@ -1054,38 +1073,44 @@ impl EmitterWriter {
                               -> io::Result<()> {
        use std::borrow::Borrow;

-        let primary_span = suggestion.msp.primary_span().unwrap();
+        let primary_span = suggestion.substitutes[0].0;


Should be abstracted into a method IMO.

nagisa · 2017-05-11T07:19:17Z

src/librustc_errors/lib.rs

-        // remove trailing newline
-        buf.pop();
-        buf
+        for buf in &mut bufs {


This code is weird, mostly due to how push_trailing is used in the loop.

Not sure what you mean. The code is exactly the same as it was before, but produces multiple suggestions instead of one. The only change i made other than the multiple suggestion part is to not add the trailing line if the replacement already produced a newline character and is thus finished.

nagisa · 2017-05-11T07:23:47Z

src/librustc_resolve/lib.rs

@@ -2584,7 +2598,7 @@ impl<'a> Resolver<'a> {
                module = Some(self.graph_root);
                continue
            } else if i == 0 && ns == TypeNS && ident.name == "$crate" {
-                module = Some(self.resolve_crate_var(ident.ctxt));
+                module = Some(self.resolve_crate_var(ident.ctxt, DUMMY_SP));


Is DUMMY_SP here intended?

This is for external crates. I'm not sure if external crates have spans

You could use the span of the use location? Any span is better than DUMMY_SP.

nagisa · 2017-05-11T07:25:23Z

src/librustc_resolve/lib.rs

-                              "".to_owned()
-                          }
-                          ));
+    err.span_suggestions(span, &msg, path_strings);


Why not just call err.span_suggestion multiple times in the loop above?

There's a difference. The API is probably badly named. it would be span_suggestion_with_multiple_substitutions.

I could automate it by checking if the message and span are the same as an existing suggestion and then merge them. This would get rid of the span_suggestions entirely.

nagisa · 2017-05-11T07:27:32Z

src/libsyntax/json.rs

-            RenderSpan::Suggestion(ref suggestion) =>
-                DiagnosticSpan::from_suggestion(suggestion, je),
+            // regular diagnostics don't produce this anymore
+            // will be removed in a later commit


Tag the comment with FIXME please.

nagisa · 2017-05-11T07:33:42Z

src/test/ui/resolve/issue-16058.stderr

-             `use std::io::Result;`
-             `use std::thread::Result;`
+help: possible better candidates are found in other modules, you can import them into scope
+   | use std::fmt::Result;


Are these multiple suggestions or multiple substitutions? If suggestions, how does the one suggestion with multiple substitutions look like? What about multiple suggestions with one substitution each? Multiple suggestions with multiple substitutions?

If they do not look any differently, perhaps we could get rid of the multiple substitutions concept and always use multiple suggestions instead?

multiple substitutions.

multiple suggestions show up with their own help each (just as they did before).

multiple suggestions with multiple substitutions show up as a help each + their substitutions below.

nagisa · 2017-05-11T21:48:53Z

r=me with the dummy_sp somehow removed.

oli-obk · 2017-05-12T09:22:59Z

It was a little more involved to get the span to that function, but it's there now.

nagisa · 2017-05-12T21:57:39Z

@bors r+

bors · 2017-05-12T21:57:40Z

📌 Commit 3f2bbe3 has been approved by nagisa

@nrc

…agisa Refactor suggestion diagnostic API to allow for multiple suggestions r? @jonathandturner cc @nrc @petrochenkov

Rollup of 15 pull requests - Successful merges: #41820, #41860, #41876, #41896, #41912, #41916, #41918, #41921, #41923, #41934, #41935, #41940, #41942, #41943, #41951 - Failed merges:

@nagisa

Refactor internal suggestion API ~~The only functional change is that whitespace, which is suggested to be added, also gets `^^^^` under it. An example is shown in the tests (the only test that changed).~~ Continuation of #41876 r? @nagisa the changes are probably best viewed [without whitespace](https://github.com/rust-lang/rust/pull/45741/files?w=1)

Remove left over dead code from suggestion diagnostic refactoring More cleanups after #41876 and #45741

oli-obk added 2 commits May 10, 2017 13:20

Refactor suggestion diagnostic API to allow for multiple suggestions

67d762d

Example usage of multiple suggestions

e2f781c

rust-highfive assigned sophiajt May 10, 2017

oli-obk added 2 commits May 10, 2017 13:58

Fix tidy issues

9d51d6b

Update a compile-fail test

f859f25

oli-obk force-pushed the diagnosing_diagnostics branch from d2f94e1 to f859f25 Compare May 10, 2017 15:05

oli-obk closed this May 10, 2017

oli-obk reopened this May 10, 2017

TimNN added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 10, 2017

nagisa reviewed May 11, 2017

View reviewed changes

Address PR reviews

644ce5e

oli-obk added 2 commits May 12, 2017 11:21

Weave the span of an import through the resolve code

a54bbf2

Don't use a DUMMY_SP for reporting issues with crate imports

3f2bbe3

oli-obk force-pushed the diagnosing_diagnostics branch from 33dcc19 to 3f2bbe3 Compare May 12, 2017 12:09

Mark-Simulacrum mentioned this pull request May 13, 2017

Rollup of 15 pull requests #41965

Merged

bors merged commit 3f2bbe3 into rust-lang:master May 13, 2017

oli-obk deleted the diagnosing_diagnostics branch May 14, 2017 07:02

oli-obk mentioned this pull request May 14, 2017

incoming breakage rust-lang/rust-clippy#1759

Closed

oli-obk mentioned this pull request Jun 8, 2017

Weird help message suggest to place use randomly, when a macro is called in scope #42548

Closed

oli-obk mentioned this pull request Jun 22, 2017

Tracking issue: improve error messages for IDEs #42823

Closed

6 tasks

oli-obk mentioned this pull request Nov 3, 2017

Refactor internal suggestion API #45741

Merged

oli-obk mentioned this pull request Nov 16, 2017

Remove left over dead code from suggestion diagnostic refactoring #46039

Merged

bors added a commit that referenced this pull request Nov 18, 2017

Auto merge of #46039 - oli-obk:test_suggestions, r=petrochenkov

6160040

Remove left over dead code from suggestion diagnostic refactoring More cleanups after #41876 and #45741

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor suggestion diagnostic API to allow for multiple suggestions #41876

Refactor suggestion diagnostic API to allow for multiple suggestions #41876

oli-obk commented May 10, 2017

oli-obk commented May 10, 2017

sophiajt commented May 11, 2017

nrc commented May 11, 2017

nagisa May 11, 2017

oli-obk May 11, 2017

nagisa May 11, 2017

nagisa May 11, 2017

nagisa May 11, 2017

oli-obk May 11, 2017

nagisa May 11, 2017

oli-obk May 11, 2017

nagisa May 11, 2017

nagisa May 11, 2017

oli-obk May 11, 2017

nagisa May 11, 2017

nagisa May 11, 2017

oli-obk May 11, 2017

nagisa commented May 11, 2017

oli-obk commented May 12, 2017

nagisa commented May 12, 2017

bors commented May 12, 2017

Refactor suggestion diagnostic API to allow for multiple suggestions #41876

Refactor suggestion diagnostic API to allow for multiple suggestions #41876

Conversation

oli-obk commented May 10, 2017

oli-obk commented May 10, 2017

sophiajt commented May 11, 2017

nrc commented May 11, 2017

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

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

nagisa commented May 11, 2017

oli-obk commented May 12, 2017

nagisa commented May 12, 2017

bors commented May 12, 2017