KEMBAR78
Improve null case handling in MatchLegacySwitchOnStringWithDict by ds5678 · Pull Request #3422 · icsharpcode/ILSpy · GitHub
Skip to content

Improve null case handling in MatchLegacySwitchOnStringWithDict #3422

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Mar 15, 2025
Merged

Improve null case handling in MatchLegacySwitchOnStringWithDict #3422

merged 3 commits into from
Mar 15, 2025
+164 −2

Conversation

ds5678
Copy link
Contributor

@ds5678 ds5678 commented Mar 10, 2025 •
edited
Loading

Updated the condition for nullValueCaseBlock to ensure it is not null and not equal to defaultBlock.

Problem

Resolves #3421

Solution

  • Any comments on the approach taken, its consistency with surrounding code, etc.
  • Which part of this PR is most in need of attention/improvement?
  • At least one test covering the code changed
    • Is a test necessary for this? If so, I'm not sure what the best way to write that test is.

@ds5678
Fix null check in MatchLegacySwitchOnStringWithDict
cbe8dd4 
Updated the condition for `nullValueCaseBlock` to ensure it is not null and not equal to `defaultBlock`.
@ds5678 ds5678 changed the title Fix null check in MatchLegacySwitchOnStringWithDict Add null check in MatchLegacySwitchOnStringWithDict Mar 10, 2025
@siegfriedpammer
Copy link
Member

Thank you very much for looking into this problem and providing a fix!

I think a test case would be nice, but is not strictly necessary. Judging from the .NET Framework version, this is code generated by the C# 2.0 compiler and I think you could only add an IL pretty test, similar to the test in your last PR.

The fix is very simple and fixing an obvious flaw, if you look at the code that handles C# 1.0 switch on string (using Hashtable), you can see that a check for Leave instructions (where the defaultBlock would be null) is present there, but not in the C# 2.0 case.

@ds5678
Copy link
Contributor Author

ds5678 commented Mar 15, 2025

I added a test, but I think I discovered a larger issue.

IL_0000: ldarg.0
IL_0001: ldfld string Issue3421::name
IL_0006: stloc.0
IL_0007: ldloc.0
IL_0008: brfalse IL_0093

IL_0093: br IL_00b3

IL_00b3: ret

ILSpy seems to be erasing this null check and treats it as the default case.

@ds5678
Enhance null handling in switch transformations
e4000c8 
- Updated `Issue3421.cs`.
- Updated `MatchLegacySwitchOnStringWithDict` to check for `leaveContainer` and handle null sections accordingly.
- Introduced an overload for `AddNullSection` to accept `ILInstruction` as the body, improving flexibility.
- Modified existing `AddNullSection` to utilize the new overload, allowing for varied body types in `SwitchSection`.
@ds5678
Copy link
Contributor Author

ds5678 commented Mar 15, 2025

I found the issue. It should be good now.

@ds5678 ds5678 changed the title Add null check in MatchLegacySwitchOnStringWithDict Improve null case handling in MatchLegacySwitchOnStringWithDict Mar 15, 2025
@siegfriedpammer
Copy link
Member

Thank you for the contribution!

@siegfriedpammer siegfriedpammer merged commit a264217 into icsharpcode:master Mar 15, 2025
5 checks passed
@ds5678 ds5678 deleted the fix-issue-3421 branch March 15, 2025 11:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Bug: null target for null switch case in legacy switch on string with dictionary

2 participants

@ds5678 @siegfriedpammer