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.

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

Merged
merged 3 commits into from
Mar 15, 2025

Conversation

ds5678
Copy link
Contributor

@ds5678 ds5678 commented Mar 10, 2025

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.

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.

- 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
Labels
None yet
Projects
None yet
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