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.

[pietroalbini]

I'd say it's a bug in my PR :(
Didn't know about this issue, cc @killercup

[killercup]

Thanks for catching that, @zackmdavis! This use case totally slipped my mind!

Talking with @pietroalbini about this on discord in #rustfix right now

[pietroalbini]

So, yeah, what we should do to fix this? I'd be willing to do the implementation work.

[estebank]

I think a possible solution would have for multiple suggestions to be considered multiple children. That way, the presented example would remain as is (and tools would have to act accordingly, applying all suggested changes simultaneously, and for cases like "struct not found, import one of the following" being presented as different children with the same message (everything else too really) and a different suggested_replacement:

{
    "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
                }
            ],
            "children": [],
            "rendered": null
        },
        {
            "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::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"
}

Does anyone see a problem with making this change? The only thing I can think of is tools that are updated to interpret the json output in that way would still receive the output from older rustc versions and would do the wrong thing. To avoid that we could add a field to the output signaling how to interpret the output. For rustc output that doesn't carry that signal, continue doing what we're doing now (even if incorrect, at least consistent).

[estebank]

Of course, the above is proposed as a way to keep as much backwards and forwards compatibility as possible. It would still break down in that people might still be able to apply, individually, all of the alternative suggestions in the same place, where really only one should. My ideal change would be to completely rework the output to raise suggestions as a fully supported concept, that could communicate "multiple multipart suggestions" without having to fake it.

[ehuss]

I would prefer to avoid using multiple children. Currently with multiple spans I'm able to show a succinct list of suggestions:

If they were multiple children, they would show up as separate messages with all the extra text for each suggestion.

Is there an example of a machine-applicable suggestion that has this behavior? It seems to me that if there are multiple suggestions where only one is valid, that should not be machine-applicable. I didn't fully understand the motivation for reverting @pietroalbini's PR.

[zackmdavis]

A CodeSuggestion can have multiple mutually-exclusive Substitutions, each of which has parts. Each CodeSuggestion becomes an element in the children field in the serialized Diagnostic

Current layout for diagnostic with one suggestion with multiple substitutions—

• children has one element
• each of children[0].spans is one of the distinct, mutually-exclusive substitutions

Current layout for diagnostic with one suggestion with single multi-part substitution—

• children has one element
• each of children[0].spans is a part of the same substitution

This is bad because rustfix has no way to distinguish these two very different situations!!

---

Proposed new layout for diagnostic with one suggestion, multiple substitutions—

• children has multiple elements, one for each proposed substitution
• children[i].spans are the parts of that subtitution

That is, instead of each CodeSuggestion getting/becoming an element in the children array of the serialized-Diagnostic, each Substitution gets/becomes an element of the children. For most diagnostics, this makes no difference (a typical CodeSuggestion only has one substitution). The only diagnostics that should change are those with multiple Substitutions, which should be pretty rare (only created by .span_suggestions, which is called six times in the rustc codebase), giving us a small backwards-incompatibility surface area.

TODO—

• read rustfix/src/lib.rs and confirm that this makes sense in terms of rustfix's "Solutions" concept
• code up this change to the JSON emitter
• appropriate changes to rustfix
• ask RLS folks if/how this impacts them
• ping ehuss on if/how this impacts them
• PR!!

[killercup]

notes from all hands meeting

[camsteffen]

is the assertion that no lints presently exist that offer exclusive machine-applicable suggestions true?

I think so. Looking over Clippy, I found 3 lints that offer mutually exclusive suggestions. But they are not MachineApplicable. The lints are (FWIW) zero_prefixed_literal, clone_double_ref, misrefactored_assign_op.

[pnkfelix]

@oli-obk and @estebank : Do you think that issue #53934 should remain at P-high? Or should it be downgraded to P-medium at this point, especially considering Niko’s writeup from May of 2021?

[pnkfelix]

(I discussed with @estebank ; we're going to leave this at P-high and they're going to see about enlisting some assistance to work on it.)

[pnkfelix]

Also, since @estebank has agreed to mentor someone on this, I'm going to tag it as E-mentor.

@rustbot label: +E-mentor

[yerke]

@rustbot claim

[pnkfelix]

Visiting for P-high review

@estebank @yerke how is progress moving along here?

[pauleaster]

I was going to work on this but started a new job in February that took my spare time. I had started on this but didn't get much further than duplicating the error on my compiler build.

Things have settled down now and I should have more time to get to this and I want to work more on rust in my spare time.

If someone else wants to do it faster then I'm happy to let them do it, though.