Repository: rust
Issue: disambiguate between multiple suggestions and a single multi-span suggestion; or, JSON error format is not round-trippable
Labels: C-cleanup,A-diagnostics,E-mentor,P-high,T-compiler,E-medium,WG-diagnostics,D-diagnostic-infra

## Summary

To solve rust-lang-nursery/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`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_errors/struct.Diagnostic.html#method.span_suggestions) and [`multipart_suggestion`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_errors/struct.Diagnostic.html#method.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](https://github.com/rust-lang/rust/blob/cd5c26f0eb48c8f32ea86e9f2434d905ff2cfc74/src/librustc_errors/diagnostic.rs#L28) `CodeSuggestion`s, a `CodeSuggestion` [has a vec of many](https://github.com/rust-lang/rust/blob/cd5c26f0eb48c8f32ea86e9f2434d905ff2cfc74/src/librustc_errors/lib.rs#L99) `Substitution`s, and a `Substitution` [has a vec of many](https://github.com/rust-lang/rust/blob/cd5c26f0eb48c8f32ea86e9f2434d905ff2cfc74/src/librustc_errors/lib.rs#L113) `SubstitutionPart`s.

[`span_suggestions` pushes](https://github.com/rust-lang/rust/blob/cd5c26f0eb48c8f32ea86e9f2434d905ff2cfc74/src/librustc_errors/diagnostic.rs#L303-L308) one `CodeSuggestion` with multiple `Substitution`s (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](https://github.com/rust-lang/rust/blob/cd5c26f0eb48c8f32ea86e9f2434d905ff2cfc74/src/librustc_errors/diagnostic.rs#L286-L292) 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](https://github.com/rust-lang/rust/blob/cd5c26f0eb48c8f32ea86e9f2434d905ff2cfc74/src/libsyntax/json.rs#L97-L109) contains a `children` field whose elements are also serialized diagnostics (with no children themselves). The suggestions [are converted](https://github.com/rust-lang/rust/blob/cd5c26f0eb48c8f32ea86e9f2434d905ff2cfc74/src/libsyntax/json.rs#L175-L184) and [included as "children"](https://github.com/rust-lang/rust/blob/cd5c26f0eb48c8f32ea86e9f2434d905ff2cfc74/src/libsyntax/json.rs#L212-L214), but in doing so, we [flat-map all the substitution parts together](https://github.com/rust-lang/rust/blob/cd5c26f0eb48c8f32ea86e9f2434d905ff2cfc74/src/libsyntax/json.rs#L323-L338), 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](https://github.com/rust-lang-nursery/rustfix/blob/ef816e0688f31b0613fdded80f474d30cc5ee46e/tests/edge-cases/skip-multi-option-lints.rs), 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.) 

```rust
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](https://github.com/rust-lang/rust/blob/ee73f80dc963707df3b3da82976556d64cac5752/src/librustc_resolve/lib.rs#L4771-L4785).

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:

```json
{
    "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.

```rustc
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](https://github.com/rust-lang/rust/blob/ee73f80dc963707df3b3da82976556d64cac5752/src/libsyntax/parse/parser.rs#L3904-L3910).

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:

```json
{
    "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](https://github.com/rust-lang/rust/issues/53934#issuecomment-831396123)

<!-- TRIAGEBOT_START -->

<!-- TRIAGEBOT_ASSIGN_START -->

<!-- TRIAGEBOT_ASSIGN_DATA_START$${"user":"yerke"}$$TRIAGEBOT_ASSIGN_DATA_END -->

<!-- TRIAGEBOT_ASSIGN_END -->
<!-- TRIAGEBOT_END -->