Do not apply Fix All operations to generated code #33515
Conversation
src/VisualStudio/IntegrationTest/IntegrationTests/CSharp/CSharpCodeActions.cs
Outdated
Show resolved
Hide resolved
|
I was able to find the commit that regressed from attached issue, and it was news to me that we had switched FixAll to skip generated files by default even before that commit. In that sense, your PR just restores the functionality, so I will remove the block. I would still be keen to hear how others feel about us offering a FixAll in document in generated files that fixes nothing. |
I wanted to change the behavior so Fix All operations would work in generated code (#26591), but we need to ensure that, at minimum, changes are not made to generated files when the code fix is initiated in non-generated code. |
src/VisualStudio/IntegrationTest/IntegrationTests/CSharp/CSharpCodeActions.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
The product change and new added tests look fine to me. However, the changes to unskip existing flaky integration tests that are not related to the issue being fixed here should be moved to a separate PR, which shows these integration tests pass consistently for multiple runs. Once that change is done, I can remove the block on this PR.
sharwell
force-pushed
the
no-fixing-generated
branch
from
2179a34 to
13cdc70
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
We should really add some additional comments around the assertion that fix alls in a generated document are offered but don't do anything. As @mavasani is calling out, that experience is 100% broken and doesn't seem to be right to be asserting in a test without a tracking bug and/or large comment saying "we know this is broken, if you fix the behavior go ahead and fix the test".
sharwell
force-pushed
the
no-fixing-generated
branch
from
13cdc70 to
caedd4a
Compare
There are four conditions covered by the tests. Two are tests of behavior with respect to intentional design, and two are tests to detect deviations from the current implemented behavior. All four are now documented for clarity. |
| Assert.Equal(generatedSource, VisualStudio.Editor.GetText()); | ||
|
|
||
| // Verify that a Fix All in Document in the generated file still does nothing. | ||
| // ⚠ This is a statement of the current behavior, and not a claim regarding correctness of the design. |
There was a problem hiding this comment.
Is this worthy of filing a tracking design issue or do we want to wait on customer feedback on this known broken behavior before spending any cycles on it?
There was a problem hiding this comment.
And how hard is this to just fix? 😄
There was a problem hiding this comment.
Is this worthy of filing a tracking design issue or do we want to wait on customer feedback on this known broken behavior before spending any cycles on it?
Fixes #33507
Customer scenario
A customer applies a Fix All in Project or Fix All in Solution operation for Remove Unnecessary Usings, and generated source files (e.g. RESX generated files or WinForms designer code) is changed.
Bugs this fixes
#33507
Workarounds, if any
Apply fixes to documents individually.
Risk
This is generally a measure our how central the affected code is to adjacent
scenarios and thus how likely your fix is to destabilize a broader area of code
Performance impact
Negligible improvement. Less work is done in a case where would should not be done.
Is this a regression from a previous update?
Yes, this is a regression from 15.0.
Root cause analysis
There were previously no tests for this feature behavior. Tests covering numerous edge cases of this feature behavior have been added.
How was the bug found?
Internal report (dogfood).
Test documentation updated?
No.