From 3e41c8d2ecb1d5034586477661dcc368d0d82558 Mon Sep 17 00:00:00 2001 From: "Zack M. Davis" Date: Wed, 6 Feb 2019 03:59:07 -0800 Subject: [PATCH 1/2] add JSON error-format UI test expectations This checks in a baseline so that it's easy to see the diff when we change the output in a subsequent commit. These examples are from and for #53934. --- .../issue-53934-multiple-parts.rs | 8 + .../issue-53934-multiple-parts.stderr | 126 ++++++++++++ .../issue-53934-multiple-substitutions.rs | 7 + .../issue-53934-multiple-substitutions.stderr | 185 ++++++++++++++++++ 4 files changed, 326 insertions(+) create mode 100644 src/test/ui/json_error_format/issue-53934-multiple-parts.rs create mode 100644 src/test/ui/json_error_format/issue-53934-multiple-parts.stderr create mode 100644 src/test/ui/json_error_format/issue-53934-multiple-substitutions.rs create mode 100644 src/test/ui/json_error_format/issue-53934-multiple-substitutions.stderr diff --git a/src/test/ui/json_error_format/issue-53934-multiple-parts.rs b/src/test/ui/json_error_format/issue-53934-multiple-parts.rs new file mode 100644 index 0000000000000..b7bc0d8ad40c5 --- /dev/null +++ b/src/test/ui/json_error_format/issue-53934-multiple-parts.rs @@ -0,0 +1,8 @@ +// compile-flags: --error-format pretty-json -Zunstable-options + +struct Point { x: isize, y: isize } + +fn main() { + let p = Point { x: 1, y: 2 }; + let Point { .., y, } = p; +} diff --git a/src/test/ui/json_error_format/issue-53934-multiple-parts.stderr b/src/test/ui/json_error_format/issue-53934-multiple-parts.stderr new file mode 100644 index 0000000000000..9ca657db280be --- /dev/null +++ b/src/test/ui/json_error_format/issue-53934-multiple-parts.stderr @@ -0,0 +1,126 @@ +{ + "message": "expected `}`, found `,`", + "code": null, + "level": "error", + "spans": [ + { + "file_name": "$DIR/issue-53934-multiple-parts.rs", + "byte_start": 166, + "byte_end": 167, + "line_start": 7, + "line_end": 7, + "column_start": 19, + "column_end": 20, + "is_primary": true, + "text": [ + { + "text": " let Point { .., y, } = p;", + "highlight_start": 19, + "highlight_end": 20 + } + ], + "label": "expected `}`", + "suggested_replacement": null, + "suggestion_applicability": null, + "expansion": null + }, + { + "file_name": "$DIR/issue-53934-multiple-parts.rs", + "byte_start": 164, + "byte_end": 167, + "line_start": 7, + "line_end": 7, + "column_start": 17, + "column_end": 20, + "is_primary": false, + "text": [ + { + "text": " let Point { .., y, } = p;", + "highlight_start": 17, + "highlight_end": 20 + } + ], + "label": "`..` must be at the end and cannot have a trailing comma", + "suggested_replacement": null, + "suggestion_applicability": null, + "expansion": null + } + ], + "children": [ + { + "message": "move the `..` to the end of the field list", + "code": null, + "level": "help", + "spans": [ + { + "file_name": "$DIR/issue-53934-multiple-parts.rs", + "byte_start": 164, + "byte_end": 168, + "line_start": 7, + "line_end": 7, + "column_start": 17, + "column_end": 21, + "is_primary": true, + "text": [ + { + "text": " let Point { .., y, } = p;", + "highlight_start": 17, + "highlight_end": 21 + } + ], + "label": null, + "suggested_replacement": "", + "suggestion_applicability": "MachineApplicable", + "expansion": null + }, + { + "file_name": "$DIR/issue-53934-multiple-parts.rs", + "byte_start": 171, + "byte_end": 172, + "line_start": 7, + "line_end": 7, + "column_start": 24, + "column_end": 25, + "is_primary": true, + "text": [ + { + "text": " let Point { .., y, } = p;", + "highlight_start": 24, + "highlight_end": 25 + } + ], + "label": null, + "suggested_replacement": ".. }", + "suggestion_applicability": "MachineApplicable", + "expansion": null + } + ], + "children": [], + "rendered": null + } + ], + "rendered": "error: expected `}`, found `,` + --> $DIR/issue-53934-multiple-parts.rs:7:19 + | +LL | let Point { .., y, } = p; + | --^ + | | | + | | expected `}` + | `..` must be at the end and cannot have a trailing comma +help: move the `..` to the end of the field list + | +LL | let Point { y, .. } = p; + | -- ^^^^ + +" +} +{ + "message": "aborting due to previous error", + "code": null, + "level": "error", + "spans": [], + "children": [], + "rendered": "error: aborting due to previous error + +" +} diff --git a/src/test/ui/json_error_format/issue-53934-multiple-substitutions.rs b/src/test/ui/json_error_format/issue-53934-multiple-substitutions.rs new file mode 100644 index 0000000000000..135eff6468b7a --- /dev/null +++ b/src/test/ui/json_error_format/issue-53934-multiple-substitutions.rs @@ -0,0 +1,7 @@ +// compile-flags: --error-format pretty-json -Zunstable-options + +fn main() { + let xs = vec![String::from("foo")]; + let d: &Display = &xs; + println!("{}", d); +} diff --git a/src/test/ui/json_error_format/issue-53934-multiple-substitutions.stderr b/src/test/ui/json_error_format/issue-53934-multiple-substitutions.stderr new file mode 100644 index 0000000000000..9377c9033a12a --- /dev/null +++ b/src/test/ui/json_error_format/issue-53934-multiple-substitutions.stderr @@ -0,0 +1,185 @@ +{ + "message": "cannot find type `Display` in this scope", + "code": { + "code": "E0412", + "explanation": " +The type name used is not in scope. + +Erroneous code examples: + +```compile_fail,E0412 +impl Something {} // error: type name `Something` is not in scope + +// or: + +trait Foo { + fn bar(N); // error: type name `N` is not in scope +} + +// or: + +fn foo(x: T) {} // type name `T` is not in scope +``` + +To fix this error, please verify you didn't misspell the type name, you did +declare it or imported it into the scope. Examples: + +``` +struct Something; + +impl Something {} // ok! + +// or: + +trait Foo { + type N; + + fn bar(_: Self::N); // ok! +} + +// or: + +fn foo(x: T) {} // ok! +``` + +Another case that causes this error is when a type is imported into a parent +module. To fix this, you can follow the suggestion and use File directly or +`use super::File;` which will import the types from the parent namespace. An +example that causes this error is below: + +```compile_fail,E0412 +use std::fs::File; + +mod foo { + fn some_function(f: File) {} +} +``` + +``` +use std::fs::File; + +mod foo { + // either + use super::File; + // or + // use std::fs::File; + fn foo(f: File) {} +} +# fn main() {} // don't insert it for us; that'll break imports +``` +" + }, + "level": "error", + "spans": [ + { + "file_name": "$DIR/issue-53934-multiple-substitutions.rs", + "byte_start": 129, + "byte_end": 136, + "line_start": 5, + "line_end": 5, + "column_start": 13, + "column_end": 20, + "is_primary": true, + "text": [ + { + "text": " let d: &Display = &xs;", + "highlight_start": 13, + "highlight_end": 20 + } + ], + "label": "not found in this scope", + "suggested_replacement": null, + "suggestion_applicability": null, + "expansion": null + } + ], + "children": [ + { + "message": "possible candidates are found in other modules, you can import them into scope", + "code": null, + "level": "help", + "spans": [ + { + "file_name": "$DIR/issue-53934-multiple-substitutions.rs", + "byte_start": 65, + "byte_end": 65, + "line_start": 3, + "line_end": 3, + "column_start": 1, + "column_end": 1, + "is_primary": true, + "text": [ + { + "text": "fn main() {", + "highlight_start": 1, + "highlight_end": 1 + } + ], + "label": null, + "suggested_replacement": "use std::fmt::Display; + +", + "suggestion_applicability": "Unspecified", + "expansion": null + }, + { + "file_name": "$DIR/issue-53934-multiple-substitutions.rs", + "byte_start": 65, + "byte_end": 65, + "line_start": 3, + "line_end": 3, + "column_start": 1, + "column_end": 1, + "is_primary": true, + "text": [ + { + "text": "fn main() {", + "highlight_start": 1, + "highlight_end": 1 + } + ], + "label": null, + "suggested_replacement": "use std::path::Display; + +", + "suggestion_applicability": "Unspecified", + "expansion": null + } + ], + "children": [], + "rendered": null + } + ], + "rendered": "error[E0412]: cannot find type `Display` in this scope + --> $DIR/issue-53934-multiple-substitutions.rs:5:13 + | +LL | let d: &Display = &xs; + | ^^^^^^^ not found in this scope +help: possible candidates are found in other modules, you can import them into scope + | +LL | use std::fmt::Display; + | +LL | use std::path::Display; + | + +" +} +{ + "message": "aborting due to previous error", + "code": null, + "level": "error", + "spans": [], + "children": [], + "rendered": "error: aborting due to previous error + +" +} +{ + "message": "For more information about this error, try `rustc --explain E0412`.", + "code": null, + "level": "", + "spans": [], + "children": [], + "rendered": "For more information about this error, try `rustc --explain E0412`. +" +} From 6d4c7ea8c16f21ed46d27bbb72157c0a840983d0 Mon Sep 17 00:00:00 2001 From: "Zack M. Davis" Date: Wed, 6 Feb 2019 04:27:39 -0800 Subject: [PATCH 2/2] multiple substitutions become multiple children in JSON output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a proposed fix for #53934. The problem being solved is this: in our internal representation of diagnostics, a `CodeSuggestion` can have one or more `Substitutions`, which can in turn have more than one `SubstitutionPart`s. The vast majority of suggestions have just one `Substitution` and one `SubstitutionPart`, but `.span_suggestions` creates multiple `Substitutions` and `.multipart_suggestion` creates multiple `SubstitutionParts`. Previously, the JSON-emitter was flat-mapping spans into the `children` field of serialized diagnostics, thereby conflating the one-substitution-with-multiple-parts case and the multiple-substitutions-with-one-part-each case, which tools like Rustfix need to be able to distinguish between. Here, we make multiple substitutions appear as multiple children, which is logical. Rustfix was already explicitly bailing out when it found multiple solutions, so there should be no breakage there. Breakage to RLS or third-party tooling may be possible, but is hoped/believed to be acceptable/within-policy (the intent here is to fix outright buggy output, not to constitute an incompatible change to the format). Thanks to Pietro Albini, Esteban Küber, and Pascal Hertleif for discussion at the 2019 All-Hands meeting. --- src/libsyntax/json.rs | 58 ++++----- .../issue-53934-multiple-substitutions.stderr | 11 +- src/test/ui/lint/use_suggestion_json.stderr | 121 ++++++++++++++++-- 3 files changed, 148 insertions(+), 42 deletions(-) diff --git a/src/libsyntax/json.rs b/src/libsyntax/json.rs index cf11ac550b763..63d0187960b7e 100644 --- a/src/libsyntax/json.rs +++ b/src/libsyntax/json.rs @@ -12,7 +12,7 @@ use source_map::{SourceMap, FilePathMapping}; use syntax_pos::{self, MacroBacktrace, Span, SpanLabel, MultiSpan}; use errors::registry::Registry; -use errors::{DiagnosticBuilder, SubDiagnostic, CodeSuggestion, SourceMapper}; +use errors::{DiagnosticBuilder, SubDiagnostic, SourceMapper}; use errors::{DiagnosticId, Applicability}; use errors::emitter::{Emitter, EmitterWriter}; @@ -162,15 +162,32 @@ impl Diagnostic { fn from_diagnostic_builder(db: &DiagnosticBuilder, je: &JsonEmitter) -> Diagnostic { - let sugg = db.suggestions.iter().map(|sugg| { - Diagnostic { - message: sugg.msg.clone(), - code: None, - level: "help", - spans: DiagnosticSpan::from_suggestion(sugg, je), - children: vec![], - rendered: None, - } + // In our internal representation, a `CodeSuggestion` has one + // or more `Substitutions`, each of which has one or more + // `SubstitutionParts`; here, each `Substitution` becomes an + // element in the `children` array + let substitution_children = db.suggestions.iter().flat_map(|suggestion| { + suggestion.substitutions.iter().map(move |substitution| { + Diagnostic { + message: suggestion.msg.clone(), + code: None, + level: "help", + spans: substitution.parts.iter().map(move |suggestion_inner| { + let span_label = SpanLabel { + span: suggestion_inner.span, + is_primary: true, + label: None, + }; + DiagnosticSpan::from_span_label( + span_label, + Some((&suggestion_inner.snippet, suggestion.applicability)), + je + ) + }).collect(), + children: Vec::new(), + rendered: None, + } + }) }); // generate regular command line output and store it in the json @@ -201,7 +218,7 @@ impl Diagnostic { spans: DiagnosticSpan::from_multispan(&db.span, je), children: db.children.iter().map(|c| { Diagnostic::from_sub_diagnostic(c, je) - }).chain(sugg).collect(), + }).chain(substitution_children).collect(), rendered: Some(output), } } @@ -308,25 +325,6 @@ impl DiagnosticSpan { .collect() } - fn from_suggestion(suggestion: &CodeSuggestion, je: &JsonEmitter) - -> Vec { - suggestion.substitutions - .iter() - .flat_map(|substitution| { - substitution.parts.iter().map(move |suggestion_inner| { - let span_label = SpanLabel { - span: suggestion_inner.span, - is_primary: true, - label: None, - }; - DiagnosticSpan::from_span_label(span_label, - Some((&suggestion_inner.snippet, - suggestion.applicability)), - je) - }) - }) - .collect() - } } impl DiagnosticSpanLine { diff --git a/src/test/ui/json_error_format/issue-53934-multiple-substitutions.stderr b/src/test/ui/json_error_format/issue-53934-multiple-substitutions.stderr index 9377c9033a12a..04a6bda7bbb45 100644 --- a/src/test/ui/json_error_format/issue-53934-multiple-substitutions.stderr +++ b/src/test/ui/json_error_format/issue-53934-multiple-substitutions.stderr @@ -121,7 +121,16 @@ mod foo { ", "suggestion_applicability": "Unspecified", "expansion": null - }, + } + ], + "children": [], + "rendered": null + }, + { + "message": "possible candidates are found in other modules, you can import them into scope", + "code": null, + "level": "help", + "spans": [ { "file_name": "$DIR/issue-53934-multiple-substitutions.rs", "byte_start": 65, diff --git a/src/test/ui/lint/use_suggestion_json.stderr b/src/test/ui/lint/use_suggestion_json.stderr index dee7f2f9b160b..b1d1ec4c4c878 100644 --- a/src/test/ui/lint/use_suggestion_json.stderr +++ b/src/test/ui/lint/use_suggestion_json.stderr @@ -121,7 +121,16 @@ mod foo { ", "suggestion_applicability": "Unspecified", "expansion": null - }, + } + ], + "children": [], + "rendered": null + }, + { + "message": "possible candidates are found in other modules, you can import them into scope", + "code": null, + "level": "help", + "spans": [ { "file_name": "$DIR/use_suggestion_json.rs", "byte_start": 417, @@ -144,7 +153,16 @@ mod foo { ", "suggestion_applicability": "Unspecified", "expansion": null - }, + } + ], + "children": [], + "rendered": null + }, + { + "message": "possible candidates are found in other modules, you can import them into scope", + "code": null, + "level": "help", + "spans": [ { "file_name": "$DIR/use_suggestion_json.rs", "byte_start": 417, @@ -167,7 +185,16 @@ mod foo { ", "suggestion_applicability": "Unspecified", "expansion": null - }, + } + ], + "children": [], + "rendered": null + }, + { + "message": "possible candidates are found in other modules, you can import them into scope", + "code": null, + "level": "help", + "spans": [ { "file_name": "$DIR/use_suggestion_json.rs", "byte_start": 417, @@ -190,7 +217,16 @@ mod foo { ", "suggestion_applicability": "Unspecified", "expansion": null - }, + } + ], + "children": [], + "rendered": null + }, + { + "message": "possible candidates are found in other modules, you can import them into scope", + "code": null, + "level": "help", + "spans": [ { "file_name": "$DIR/use_suggestion_json.rs", "byte_start": 417, @@ -213,7 +249,16 @@ mod foo { ", "suggestion_applicability": "Unspecified", "expansion": null - }, + } + ], + "children": [], + "rendered": null + }, + { + "message": "possible candidates are found in other modules, you can import them into scope", + "code": null, + "level": "help", + "spans": [ { "file_name": "$DIR/use_suggestion_json.rs", "byte_start": 417, @@ -236,7 +281,16 @@ mod foo { ", "suggestion_applicability": "Unspecified", "expansion": null - }, + } + ], + "children": [], + "rendered": null + }, + { + "message": "possible candidates are found in other modules, you can import them into scope", + "code": null, + "level": "help", + "spans": [ { "file_name": "$DIR/use_suggestion_json.rs", "byte_start": 417, @@ -259,7 +313,16 @@ mod foo { ", "suggestion_applicability": "Unspecified", "expansion": null - }, + } + ], + "children": [], + "rendered": null + }, + { + "message": "possible candidates are found in other modules, you can import them into scope", + "code": null, + "level": "help", + "spans": [ { "file_name": "$DIR/use_suggestion_json.rs", "byte_start": 417, @@ -282,7 +345,16 @@ mod foo { ", "suggestion_applicability": "Unspecified", "expansion": null - }, + } + ], + "children": [], + "rendered": null + }, + { + "message": "possible candidates are found in other modules, you can import them into scope", + "code": null, + "level": "help", + "spans": [ { "file_name": "$DIR/use_suggestion_json.rs", "byte_start": 417, @@ -305,7 +377,16 @@ mod foo { ", "suggestion_applicability": "Unspecified", "expansion": null - }, + } + ], + "children": [], + "rendered": null + }, + { + "message": "possible candidates are found in other modules, you can import them into scope", + "code": null, + "level": "help", + "spans": [ { "file_name": "$DIR/use_suggestion_json.rs", "byte_start": 417, @@ -328,7 +409,16 @@ mod foo { ", "suggestion_applicability": "Unspecified", "expansion": null - }, + } + ], + "children": [], + "rendered": null + }, + { + "message": "possible candidates are found in other modules, you can import them into scope", + "code": null, + "level": "help", + "spans": [ { "file_name": "$DIR/use_suggestion_json.rs", "byte_start": 417, @@ -351,7 +441,16 @@ mod foo { ", "suggestion_applicability": "Unspecified", "expansion": null - }, + } + ], + "children": [], + "rendered": null + }, + { + "message": "possible candidates are found in other modules, you can import them into scope", + "code": null, + "level": "help", + "spans": [ { "file_name": "$DIR/use_suggestion_json.rs", "byte_start": 417,