multiple substitutions become multiple children in JSON output #58221
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Demonstration of
|
|
☔ The latest upstream changes (presumably #58125) made this pull request unmergeable. Please resolve the merge conflicts. |
bors
added
the
S-waiting-on-author
|
ping from triage @zackmdavis what's the update on this? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a proposed fix for #53934.
The problem being solved is this: in our internal representation of diagnostics, a
CodeSuggestioncan have one or moreSubstitutions, which can in turn have more than oneSubstitutionParts. The vast majority of suggestions have just oneSubstitutionand oneSubstitutionPart, but.span_suggestionscreates multipleSubstitutionsand.multipart_suggestioncreates multipleSubstitutionParts.Previously, the JSON-emitter was flat-mapping spans into the
childrenfield 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).Look specifically at the diff of the second "multiple substitutions become ..." commit to see the diff in the JSON output. (The first commit introduces some
--error-format jsonUI tests specifically to document this.)rust-lang/rustfix#162 is the sister pull-request for rustfix.
Don't merge this yet; the following items need to be resolved first—
cargo fixcan fix a multi-span substitutioncargo fix(requires rustfix built with accept multi-part substitutions rustfix#162)Thanks to @pietroalbini, @estebank, and @killercup for discussion at the 2019 All-Hands meeting.
r? @ghost (attempting to suppress bors autoassignment since this isn't ready yet)