Fix IntroduceVariable when the textSpan is whitespace#28983
Fix IntroduceVariable when the textSpan is whitespace#28983JoeRobich merged 1 commit intodotnet:masterfrom
Conversation
|
|
sharwell
left a comment
There was a problem hiding this comment.
Looks good but note the specific comments before merge
There was a problem hiding this comment.
💡 Explanation of the scenario would be helpful
Additional trivia exists between 'textSpan' and the start of the next token, ...
Then, the ... might be replaced with something indicating why it's important:
... so the two are not considered adjacent
There was a problem hiding this comment.
❔ Out of curiosity, many of the new tests failed prior to the change?
There was a problem hiding this comment.
All four added tests were failing before the fix.
|
|
697d176 to
13d2889
Compare
There was a problem hiding this comment.
In the description of the PR/commit, it says it causes a "crash", but it should just be showing the gold bar and letting VS continue, right? This verbiage can mislead management when deciding whether to accept a fix for a particular release (though in this case you're targeting master, which is open now, so it doesn't matter as much).
Overall, I believe this PR makes the world a better place with a targeted fix for the reported scenarios. I can't actually make the product crash or show the gold bar, so I wasn't able to try out other scenarios that may have been missed here and just haven't been reported yet (what if everything is in leading trivia instead of trailing trivia? etc.). @JoeRobich can you help me repro the failure?
I thought there might be a regression introduced in the service, but I've tested it and for some reason there's no regression there. I'm fine accepting a fix with this general approach.
But, I've played with this feature in the IDE, and I personally find it difficult to understand. It working or not depends on the details of where the span starts and ends, and whether they're exactly touching tokens, or if they're just a position versus a span, etc. This was written before we really discussed general guidelines for refactorings back in May (though I also don't think we ever published that document), and it takes the stricter approach (in an attempt to avoid light bulb pollution) instead of the guidelines more lenient approach (of making the refactoring Just Work as best as possible when even remotely reasonable). (Try various positions and spans in int x = 8 * 8 + 7 * 5; to see what I mean.) As a side effect, I find the existing code in this method hard to follow (at least in its current form without good comments).
We discussed the possibility of opportunistically fixing these up as we went along and encountered them. If that's what we want to do to chip away at the problem, this could be our chance.
@jinujoseph @kuhlenh What do you think?
There was a problem hiding this comment.
If the textSpan.End and the newStart are equal, then there isn't trivia between the selection and the next token (so the comment isn't correct). I was worried this would introduce a regression in that case (non-empty selection that starts in whitespace and ends at the start of the next token), but that doesn't appear to work in the IDE currently either (I suspect due to some of the logic below that I don't immediately understand). I'll comment more on this in my overall review feedback.
There was a problem hiding this comment.
This comment only makes sense if you understand the first comment on line 200, track through how the tokens are discovered and mutated, and convince yourself that this is when the span is entirely in the trivia between two tokens (and not starting/ending in two different trivias) (unless there's any non-whitespace trivia in there for some reason??).
I actually like the explanation in your commit/PR description better since it's trying to describe the full thing (whitespace span surrounded by whitespace).
There was a problem hiding this comment.
I haven't read the 9000 lines of pre-existing tests, but are there any with spans like this that also end touching the next token? e.g. x = [| |]0; I mentioned this case elsewhere, and I'm not sure if the behavior (no refactoring offered) is intentional or not.
There was a problem hiding this comment.
Also, are there test cases similar to this one where the span is in whitespace (just like this) but the leading token contains, additionally, a /**/ so that the check for all trivia being whitespace on line 198 of the service goes the opposite way?
|
Preserving the unit test failure URL in case tests get restarted: https://ci.dot.net/job/dotnet_roslyn/job/master/job/windows_debug_unit64_prtest/15567/ |
If the textSpan is a span of whitespace that is itself surrounded by whitespace, trying to recover by getting the next token and creating a new textSpan will result in a gold bar. This change prevents the gold bar by checking the proposed newStart position and returning if it is past the end position. This fixes dotnet#27949 and fixes dotnet#28665.
13d2889 to
a4ba814
Compare
dpoeschl
left a comment
There was a problem hiding this comment.
Lgtm! We can consider the overall feature design separately.
|
retest windows_release_unit32_prtest |
|
From time to time I still get the same error.
|
|
@sheinv I think this fix will only be present in VS 2019, unfortunately (milestone is 16.0). |
@dpoeschl - as a user, getting a gold bar signifies that VS has become unstable and needs to be closed and reopened or I'm just going to get more gold bars (especially editor related ones, I left VS running after the first one today only to be smacked by this one a few minutes later). I'm not sure there is any difference in productivity loss between restarting to avoid additional problems and restarting because vs crashed. This impact to productivity should be getting management's attention regardless of the verbiage. |
One is data loss, one is productivity here. These are different things, and people should be aware of which is happening to make the most informed decision possible. The position is not that this is not important, or should not be taken. It's simply that the impact needs to be accurately described so that people who are considering what to do can properly assess things. |
|
If you want accurate then the description should be in a less subjective measure than can be had from personal assumptions of the differences between terms, triggering words, and votes (the worst measure possible). |
If the textSpan is a span of whitespace that is itself surrounded by
whitespace, trying to recover by getting the next token and creating a
new textSpan will result in a gold bar. This change prevents the gold bar by
checking the proposed newStart position and returning if it is past
the end position.
This fixes #27949 and fixes #28665.