disambiguate between multiple suggestions and a single multi-span suggestion; or, JSON error format is not round-trippable

[zackmdavis]

Summary

To solve rust-lang/rustfix#141 (and one can only assume that RLS faces a similar problem), we need to be able to tell the difference between multiple suggestions (of which we likely only want to apply one), and a single suggestion that happens to touch multiple spans. The DiagnosticBuilder API supports this distinction by offering separate span_suggestions and multipart_suggestion methods. However, it looks like the actual JSON output conflates these two cases?! (I hope I've simply misunderstood something; @estebank @oli-obk @killercup @nrc, please tell I'm just being stupid and wrong and confused here; the embarrassment of that would be less bad than than the headache of having to fix this.)

Diagnosis

The relevant layout of fields is this: Diagnostic has a vec of many CodeSuggestions, a CodeSuggestion has a vec of many Substitutions, and a Substitution has a vec of many SubstitutionParts.

span_suggestions pushes one CodeSuggestion with multiple Substitutions (each of which has a single SubstitutionPart) onto an existing diagnostic-builder. (So, arguably either the method name span_suggestions or the field name suggestions is a misnomer—it's the "substitutions" that are plural here, not the "suggestions"; you'd have to call .span_suggestion multiple times to get multiple elements in suggestions. But leave that aside for the moment.)

multipart_suggestion pushes one CodeSuggestion with one Substitution with multiple SubstitutionParts onto an existing diagnostic-builder.

All this is fine. The problem comes when we serialize diagnostics to JSON for --error-format json. The serialized diagnostic format contains a children field whose elements are also serialized diagnostics (with no children themselves). The suggestions are converted and included as "children", but in doing so, we flat-map all the substitution parts together, making it impossible to know with certainty which parts came from the same Substitution.

Concrete examples

The following program (taken from the rustfix test suite, but let's call it ambiguous-display.rs here) fails to compile because Display is not in scope. (Actually, it'll still fail after fixing that, but that doesn't matter for our purpose here.)

fn main() {
    let xs = vec![String::from("foo")];
    let d: &Display = &xs;
    println!("{}", d);
}

We get two mutually-exclusive suggestions to use std::fmt::Display and std::path::Display, issued in librustc_resolve.

The terminal error output is:

error[E0412]: cannot find type `Display` in this scope
 --> ambiguous-display.rs:3:13
  |
3 |     let d: &Display = &xs;
  |             ^^^^^^^ not found in this scope
help: possible candidates are found in other modules, you can import them into scope
  |
1 | use std::fmt::Display;
  |
1 | use std::path::Display;

The JSON error format is:

{
    "message": "cannot find type `Display` in this scope",
    "code": {
        "code": "E0412",
        "explanation": "\nThe type name used is not in scope.\n\nErroneous code examples:\n\n```compile_fail,E0412\nimpl Something {} // error: type name `Something` is not in scope\n\n// or:\n\ntrait Foo {\n    fn bar(N); // error: type name `N` is not in scope\n}\n\n// or:\n\nfn foo(x: T) {} // type name `T` is not in scope\n```\n\nTo fix this error, please verify you didn't misspell the type name, you did\ndeclare it or imported it into the scope. Examples:\n\n```\nstruct Something;\n\nimpl Something {} // ok!\n\n// or:\n\ntrait Foo {\n    type N;\n\n    fn bar(_: Self::N); // ok!\n}\n\n// or:\n\nfn foo<T>(x: T) {} // ok!\n```\n\nAnother case that causes this error is when a type is imported into a parent\nmodule. To fix this, you can follow the suggestion and use File directly or\n`use super::File;` which will import the types from the parent namespace. An\nexample that causes this error is below:\n\n```compile_fail,E0412\nuse std::fs::File;\n\nmod foo {\n    fn some_function(f: File) {}\n}\n```\n\n```\nuse std::fs::File;\n\nmod foo {\n    // either\n    use super::File;\n    // or\n    // use std::fs::File;\n    fn foo(f: File) {}\n}\n# fn main() {} // don't insert it for us; that'll break imports\n```\n"
    },
    "level": "error",
    "spans": [
        {
            "file_name": "ambiguous-display.rs",
            "byte_start": 64,
            "byte_end": 71,
            "line_start": 3,
            "line_end": 3,
            "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": "ambiguous-display.rs",
                    "byte_start": 0,
                    "byte_end": 0,
                    "line_start": 1,
                    "line_end": 1,
                    "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;\n\n",
                    "suggestion_applicability": "Unspecified",
                    "expansion": null
                },
                {
                    "file_name": "ambiguous-display.rs",
                    "byte_start": 0,
                    "byte_end": 0,
                    "line_start": 1,
                    "line_end": 1,
                    "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;\n\n",
                    "suggestion_applicability": "Unspecified",
                    "expansion": null
                }
            ],
            "children": [],
            "rendered": null
        }
    ],
    "rendered": "error[E0412]: cannot find type `Display` in this scope\n --> ambiguous-display.rs:3:13\n  |\n3 |     let d: &Display = &xs;\n  |             ^^^^^^^ not found in this scope\nhelp: possible candidates are found in other modules, you can import them into scope\n  |\n1 | use std::fmt::Display;\n  |\n1 | use std::path::Display;\n  |\n\n"
}

The following program (dot-dot-not-last.rs) will fail to compile because .. can only appear last in a struct pattern.

struct Point { x: isize, y: isize }

fn main() {
    let p = Point { x: 1, y: 2 };
    let Point { .., y, } = p;
}

We get one unique suggestion that needs to touch multiple spans (removing the .. from its original location and inserting it at the end), issued in the parser.

The terminal error output is:

error: expected `}`, found `,`
 --> dot-dot-not-last.rs:5:19
  |
5 |     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
  |
5 |     let Point {  y, .. } = p;
  |                --   ^^^^

The JSON error output is:

{
    "message": "expected `}`, found `,`",
    "code": null,
    "level": "error",
    "spans": [
        {
            "file_name": "dot-dot-not-last.rs",
            "byte_start": 101,
            "byte_end": 102,
            "line_start": 5,
            "line_end": 5,
            "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": "dot-dot-not-last.rs",
            "byte_start": 99,
            "byte_end": 102,
            "line_start": 5,
            "line_end": 5,
            "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": "dot-dot-not-last.rs",
                    "byte_start": 99,
                    "byte_end": 102,
                    "line_start": 5,
                    "line_end": 5,
                    "column_start": 17,
                    "column_end": 20,
                    "is_primary": true,
                    "text": [
                        {
                            "text": "    let Point { .., y, } = p;",
                            "highlight_start": 17,
                            "highlight_end": 20
                        }
                    ],
                    "label": null,
                    "suggested_replacement": "",
                    "suggestion_applicability": "Unspecified",
                    "expansion": null
                },
                {
                    "file_name": "dot-dot-not-last.rs",
                    "byte_start": 106,
                    "byte_end": 107,
                    "line_start": 5,
                    "line_end": 5,
                    "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": "Unspecified",
                    "expansion": null
                }
            ],
            "children": [],
            "rendered": null
        }
    ],
    "rendered": "error: expected `}`, found `,`\n --> dot-dot-not-last.rs:5:19\n  |\n5 |     let Point { .., y, } = p;\n  |                 --^\n  |                 | |\n  |                 | expected `}`\n  |                 `..` must be at the end and cannot have a trailing comma\nhelp: move the `..` to the end of the field list\n  |\n5 |     let Point {  y, .. } = p;\n  |                --   ^^^^\n\n"
}

We'd want Rustfix (and similar tools) to apply both of the suggestions in children[0].spans in the case of dot-dot-not-last.rs, but only one of them (offering a choice in an interactive mode) for ambiguous-display.rs. But how is Rustfix supposed to reliably tell the difference? (In the specific case of span_suggestions, you can check that the start and end spans are the same in children[0].spans, but I'd really rather not rely on that to merely infer something that the format, I argue, should state with certainty.)

This issue should likely receive the A-diagnostics and T-dev-tools (and, one might argue, P-high) labels.

Current proposed resolution

This issue is currently proposed to be left as a future improvement; see this comment for the current status

[oli-obk]

I totally forgot about this :/ I've been wanting to fix this in forever.

you can check that the start and end spans are the same in children[0].spans

That won't work in the general case as the suggestions might be two completely different solution paths that require changing independent code. Clippy has such a suggestion.

I propose we change multipart suggestions to each be a single element of the children array and have them set their own children array with their parts instead of having that directly in the spans field.

They are inherently broken this way, so noone could have used them. Thus I believe we can break this.

We cannot just add some new annotation to the json, because there might be multiple multipart suggestions, and these are even worse as they produce a big flattened list of parts.

[ehuss]

They are inherently broken this way, so noone could have used them. Thus I believe we can break this.

Just a note, for tools (like IDEs) where you manually apply suggestions, it is not a problem, because the user sees the choices and decides which to use. So AFAIK in that scenario it is not broken. I'm not opposed to breaking it, since I can update my tool, but other tool developers may not be aware and changes may affect them.

If you do change the structure, it would be helpful if the PR includes a very clear description of the changes to the json output. I've been wanting to document the JSON format for a while, so maybe this will motivate me to do it! 😄

[oli-obk]

If we change the behaviour, they will just see fewer suggestions, which isn't really breaking.

[zackmdavis]

@oli-obk

We cannot just add some new annotation to the json, because there might be multiple multipart suggestions

I don't understand how multiple multi-part suggestions rules out adding new annotations as a solution? Suppose that in addition to "suggested_replacement" and "suggestion_applicability", DiagnosticSpan also had a "suggestion_number" key, whose value started at 0 for spans belonging to the first suggestion, and incremented for subsequent suggestions. Am I missing some obvious reason why that wouldn't work? (Of course, this would be somewhat hideous—if we can get away with breaking things, your proposal ("multipart suggestions to each be a single element" ...) sounds OK to me.)

[zackmdavis]

Hm, @pietroalbini has a new patch in rustfix and open PR for a rustc lint change that uses multipart_suggestion_ that doesn't change the emitter behavior. So I think that either (a) my analysis above is mistaken, or (b) rust-lang/rustfix#155 has a bug where it fails to distinguish between a single multipart suggestion and multiple distinct suggestions.