Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IDE0078: Code Cleanup applied IDE0078 and broke the code #75122

Closed
shpaass opened this issue Sep 16, 2024 · 5 comments · Fixed by #76183
Closed

IDE0078: Code Cleanup applied IDE0078 and broke the code #75122

shpaass opened this issue Sep 16, 2024 · 5 comments · Fixed by #76183
Assignees
Labels
Area-IDE Bug Feature - IDE0078 Use pattern combinators help wanted The issue is "up for grabs" - add a comment if you are interested in working on it
Milestone

Comments

@shpaass
Copy link

shpaass commented Sep 16, 2024

Version Used: Microsoft Visual Studio Community 2022 (64-bit) - Current Version 17.11.2

Steps to Reproduce:

  1. git clone https://github.com/shpaass/yafc-ce.git
  2. git checkout cd421ef319f9dd354757b5898500bfb13c72f140
  3. Open yafc-ce/FactorioCalc.sln in Visual Studio.
  4. Navigate to Yafc/Windows/ProjectPageSettingsPanel.cs#L222.
  5. Click on the line. See the lightbulb on the left. Click on it. See the first option: Use pattern matching (may change code meaning).
  6. Close the lightbulb without appying. Apply Code Cleanup instead.
  7. See that the target line was changed into a broken code.

From

if (DataUtils.ReadLine(bytes, ref index) != "YAFC" || DataUtils.ReadLine(bytes, ref index) != "ProjectPage") {
    throw new InvalidDataException();
}

To

if (DataUtils.ReadLine(bytes, ref index) is not "YAFC" or not "ProjectPage") {
    throw new InvalidDataException();
}

The expression now has one read operation instead of two, and is always true, which would lead to a guaranteed exception.

Diagnostic Id: "IDE0078: Use pattern matching"

Expected Behavior: The line is not changed or changed without breaking the code.

Actual Behavior: The line is changed. The change broke the code.

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Sep 16, 2024
@CyrusNajmabadi
Copy link
Member

We should probably disable the pattern matching logic if there is a variable passed by-ref. It strongly indicates a mutation across calls that can't be elided.

@CyrusNajmabadi CyrusNajmabadi added Bug help wanted The issue is "up for grabs" - add a comment if you are interested in working on it and removed untriaged Issues and PRs which have not yet been triaged by a lead labels Sep 16, 2024
@CyrusNajmabadi CyrusNajmabadi added this to the Backlog milestone Sep 16, 2024
@github-project-automation github-project-automation bot moved this to InQueue in Small Fixes Sep 16, 2024
@shpaass
Copy link
Author

shpaass commented Sep 16, 2024

@CyrusNajmabadi thank you for the triage!
I'm currently searching for a way to disable all inspections that "may change code meaning", but I haven't found a list on the internet. MSDN articles of the inspections (IDE0078 for instance) also don't mention this aspect.

Is there a list of such inspections somewhere? If not, then what can I do to compile such a list?

@CyrusNajmabadi
Copy link
Member

@CyrusNajmabadi thank you for the triage!
I'm currently searching for a way to disable all inspections that "may change code meaning", but I haven't found a list on the internet. MSDN articles of the inspections (IDE0078 for instance) also don't mention this aspect.

Is there a list of such inspections somewhere? If not, then what can I do to compile such a list?

All fixers can change code meaning. (Not joking)

shpaass added a commit to shpaass/yafc-ce that referenced this issue Sep 16, 2024
This inspection broke some of our code:
dotnet/roslyn#75122

On a side note, it turns out that all inspection-fixers can change
code meaning, so none of the inspections are safe.

IDE0078: Use pattern matching:
https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/style-rules/ide0078-ide0260
shpaass added a commit to shpaass/yafc-ce that referenced this issue Sep 16, 2024
This inspection broke some of our code:
dotnet/roslyn#75122

On a side note, it turns out that all inspection-fixers can change
code meaning, so none of the inspections are safe.

IDE0078: Use pattern matching:
https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/style-rules/ide0078-ide0260
shpaass added a commit to shpaass/yafc-ce that referenced this issue Sep 18, 2024
This inspection broke some of our code:
dotnet/roslyn#75122

On a side note, it turns out that all inspection-fixers can change
code meaning, so none of the inspections are safe.

IDE0078: Use pattern matching:
https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/style-rules/ide0078-ide0260
shpaass added a commit to shpaass/yafc-ce that referenced this issue Sep 18, 2024
This inspection broke some of our code:
dotnet/roslyn#75122

On a side note, it turns out that all inspection-fixers can change
code meaning, so none of the inspections are safe.

IDE0078: Use pattern matching:
https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/style-rules/ide0078-ide0260
@CyrusNajmabadi
Copy link
Member

@sharwell Can you make it so that "Apply Code Cleanup" does not automatically apply the CSharpUsePatternCombinatorsDiagnosticAnalyzer fixes to diagnostics that do not have hte "safe" property attached to them. I do not know how 'code cleanup' makes its determinations. Thanks.

@shpaass
Copy link
Author

shpaass commented Sep 26, 2024

On an unrelated note, apologies for the commit spam. I was rebasing after squashes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE Bug Feature - IDE0078 Use pattern combinators help wanted The issue is "up for grabs" - add a comment if you are interested in working on it
Projects
Status: Completed
Development

Successfully merging a pull request may close this issue.

3 participants