Skip to content
New issue

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

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

Already on GitHub? # 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.

# for free to join this conversation on GitHub. Already have an account? # 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