Don't perform 'commit cleanup' on regions of code with syntax errors *and* multi-line strings. #18372
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
|
Tagging @dotnet/roslyn-ide @Pilchie |
Pilchie
requested changes
Apr 3, 2017
| @@ -12,10 +15,8 @@ Namespace Microsoft.CodeAnalysis.Editor.VisualBasic.UnitTests.CaseCorrecting | |||
| Private Async Function TestAsync(input As XElement, expected As XElement, Optional interProject As Boolean = False) As Tasks.Task | |||
| If (interProject) Then | |||
| Await TestAsync(input, expected.NormalizedValue) | |||
| Await TestAsync(input, expected.NormalizedValue) | |||
There was a problem hiding this comment.
We just did this twice for fun? @heejaechang do you know why this was here?
| @@ -25,13 +25,13 @@ internal interface ICodeCleanerService : ILanguageService | |||
| /// <summary> | |||
| /// This will run all provided code cleaners in an order that is given to the method. | |||
| /// </summary> | |||
| Task<Document> CleanupAsync(Document document, ImmutableArray<TextSpan> spans, ImmutableArray<ICodeCleanupProvider> providers, CancellationToken cancellationToken); | |||
| Task<Document> CleanupAsync(Document document, ImmutableArray<TextSpan> spans, ImmutableArray<ICodeCleanupProvider> providers, CancellationToken cancellationToken = default(CancellationToken)); | |||
There was a problem hiding this comment.
This shouldn't be optional on an internal interface.
|
|
||
| Private Sub ProcessToken(token As SyntaxToken, result As ArrayBuilder(Of TextSpan)) | ||
| If token.ContainsDiagnostics Then | ||
| Dim parentMultiLineNode = GetMultiLineContainer(token.Parent) |
There was a problem hiding this comment.
For each token, we walk up and then back down to all the Descendant tokens? Is there something better we can do here?
There was a problem hiding this comment.
For each token that contains a syntax diagnostic, yes.
One thing we could potentially do is just skip tokens that intersect the last reported bad span. That seems like an easy check to add.
|
@Pilchie Ready for another review. |
Pilchie
approved these changes
Apr 3, 2017
c0e04c5 to
97a35ca
Compare
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.
Fixes: https://devdiv.visualstudio.com/DevDiv/_workitems?id=397014&triage=true&_a=edit
Because VB automatically makes an unterminated string into a multi-line string it's easily possible for someone to not terminate a string, and cause that to wildly change how code is parsed. Specifically, the unterminated string will eventually terminate at the start of the next string in the file. And the contents of that next string will then be parsed as code.
If we then cleanup this code, we end up really screwing with text that user intends to be the contents of a string literal (i.e. text like "as" gets corrected to "As" and so on).
To avoid this, we determine spans of code that we consider off-limits while cleaning things up. Right now that includes regions of code that have syntax errors and which contain multiline strings. This is a general heuristic that works well, but which can be tweaked/augmented in the future if we run into more cases where we do not want to cleanup.