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

[zackmdavis]

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 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! :smile:

[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-nursery/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

[ehuss]

I'm still having a hard time understanding the issue here. For the multiple-candidates case, it is not machine applicable. How does that confuse rustfix? Shouldn't it just be ignoring those?

I'm not a fan of splitting into multiple children because it causes the message to be duplicated multiple times, and there's no simple way to know how they should be combined.

[zackmdavis]

@ehuss Thanks for commenting!

I'm still having a hard time understanding the issue here.

Rephrasing/summarizing: Rustfix and friends need to distinguish between the following two cases:

• A compiler suggestion offers multiple different possible fixes
  • the "possible candidates are found in other module" message being an example
• A compiler suggestion offers a single fix that happens to involve editing more than one non-contiguous span
  • for example, imagine a "parenthesize this expression" suggestion: you'd want to insert both a ( before the beginning of the expression and an ) after the end

These need to be distinguishable from looking at the JSON, because in the first case (multiple alternative fixes), we only want to apply one of the alternatives, but in the second case, we want to apply all the span-edits (in the parentheses example, inserting ( but not ) would be wrong).

For the multiple-candidates case, it is not machine applicable. How does that confuse rustfix? Shouldn't it just be ignoring those?

While I agree that mutually-exclusive suggestions on the same span shouldn't be marked as MachineApplicable, that doesn't resolve the problem, in my view, because the "applicability" concept is conceptually orthogonal to the multiple-suggestions-vs.-multispan-single-suggestions issue, which does have practical implications if, for example, future versions of cargo fix may want to provide an interactive mode for manually accepting-or-rejecting suggestions (returning again to the parentheses example, we wouldn't want to prompt the user to separately approve or reject the insertion of ( and that of )).

I can update my tool [...] Currently with multiple spans I'm able to show a succinct list of suggestions

Is your tool open-source, so that we can better undertand your use-case? (I glanced at your GitHub repositories, but didn't see anything obviously relevant.)

---

But, yes, the message duplication issue makes me feel less confident that #58221 is what we actually want than when I proposed it yesterday. In a way, it's just "lifting the problem up one level": instead of conflating multiple-fixes and multispan-single-fixes, it instead conflates multiple-possible-fixes-for-the-same-error and multiple-distinct-error-suggestions. (The analogy even extends right down to the use of .flat_map.)

The reason #58221 is tempting anyway is because it's a relatively small, pragmatic change to the JSON output (most diagnostics won't be affected and will emit exactly the same JSON) that will let us fix #53934, which we really do want to fix. In contrast, reworking the output to correctly reflect the compiler's three-level CodeSuggestion/Substitution/SubstitutionPart internal representation would be a bigger breaking change, which is a bit much for something available in the stable compiler that people are already depending on. (Although the idea of a version 2 of the JSON output has been floated.)

I want to study/test how RLS handles the output from #58221—and your code, @ehuss, if you can and want to make it available—before we decide how to proceed.

[estebank]

The reason #58221 is tempting anyway is because it's a relatively small, pragmatic change to the JSON output (most diagnostics won't be affected and will emit exactly the same JSON) that will let us fix #53934, which we really do want to fix. In contrast, reworking the output to correctly reflect the compiler's three-level CodeSuggestion/Substitution/SubstitutionPart internal representation would be a bigger breaking change, which is a bit much for something available in the stable compiler that people are already depending on. (Although the idea of a version 2 of the JSON output has been floated.)

It's a bigger change, but I feel it is the right call to make in the medium term. The are no cases of multiple suggestions with multiple spans in the compiler today, mainly because it's an uncommon occurrence, which is why this json representation bug hasn't been fixed in all this time, but it is an arbitrary restriction. I believe that having an "incomplete" but workable solution is reasonable if we commit to have a "json2" output, and I also think we have a good story around how to release and evolve the new version.

[ehuss]

It's Sublime Text with the https://github.com/rust-lang/rust-enhanced/ plugin.

Perhaps a new field could be added, which would be backwards compatible.

Another possibility is that rustfix could see that the spans are identical. Are there cases where that wouldn't address it?

[zackmdavis]

Another possibility is that rustfix could see that the spans are identical.

I considered this (with a negative disposition) in the initial issue description—

(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.)

but now that I have more information about how dire the backwards-compatibility situation is (such that #58221 would seem to imply coordinated changes to rustfix and the Sublime plugin and maybe RLS, and still introduce a new problem with duplicate messages), it's starting to look way more appealing.

Upthread, @oli-obk wrote:

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.

Oli, do you remember (can you link to) the example from Clippy?

---

(Scheduling note: unfortunately, today is the last day of the all-hands meeting, and due to prior commitments, I'm not going to have more open-source dev time until the 24th at the earliest. :slightly_frowning_face: )

[oli-obk]

Oli, do you remember (can you link to) the example from Clippy?

I checked, but was unable to find it again... I thought it was something with for loops or if let.

[estebank]

There's another suggestion that's quite common that would nice to add run-rustfix to: "" + String::new()"

[nikomatsakis]

This issue is blocking progress on the 2021 Edition. Mara and I did a read-through. I wanted to document the current state as I understand it and propose a resolution.

Current state

As currently used, the set of suggestions ('spans') in the JSON potentially include many exclusive or incompatible edits. It is not possible to reliably determine which set of edits belong to one logical change.

However, for all existing lints that use the applicability MachineApplicable, each such suggestion form a logical unit that must currently applied in full. Other suggestions within the same lint may not be part of that logical unit.

Proposed resolution

The simplest fix to move forward is to document the existing meaning and to consider adding a better structure in the future. The document semantics for suggestions would be:

• Note that the "grouping" of the suggestions is not specified. The suggested spans may represent multiple logical edits or independent, exclusive suggestions. There is one special case, however, where the semantics are defined: all MachineApplicable lints must belong to a single logical edit, and hence all of them must be applied together. Those MachineApplicable edits must not depend on any other suggestions which are not MachineApplicable (in other words, i there are any MachineApplicable suggestions, there must be a single logical edit that consists of all MachineApplicable suggestions and no other suggestions).

This is suboptimal and we should try to fix it in the future. However, it documents the current state. Moreover, it makes some sense, and follows from the current comment, which states that machine applicable suggestions 'should be automatically applied'. If there are multiple valid choices with distinct semantics, then this sentence does not apply. That is essentially a form of applicability we have not yet added.

Of course, it could happen that there are multiple equivalent ways to do the same thing; in that case, we should just pick one, not give the user multiple choices. It's certainly conceivable that a situation could arise in the future where we really do want to present multiple applicable choices to the user and we can't pick one for some reason. But in that case we will have to solve this issue more completely at that time.

Proposed action

• Add a comment like the above into rustc
• Revert https://github.com/rust-lang/rustfix/pull/156 on that basis
• Leave this issue open to track adding some grouping mechanism and migrating existing tooling

Examples

• Multiple exclusive, not machine applicable suggestions: Choosing the correct trait the user intended is such an example. It is no affected by the choice here.
• One machine applicable choice that preserves semantics, other possible options: the panic! migration lint offers the user a semantics-preserving edit from panic!("{}") to panic_any!("{}") but also offers a (not machine applicable) suggestion to provide a value for th {} placeholder. This works ok here because we apply all machine applicable things but not other things (and those non-machine-applicable suggestions are indeed part of a distinct logical edit.)
• Multiple machine applicable choices that are independent: No example presently exists.

Questions

• Does this cover existing concerns? In particular, is the assertion that no lints presently exist that offer exclusive machine-applicable suggestions true?

[nikomatsakis]

cc @zackmdavis -- I know it's been a while, but as the author of this issue I just wanted to make sure you saw that ☝️ and had a chance to give your opinion on it =)

[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.