Claude/fix issue 557 char string default param#1255
Draft
GrahamTheCoder wants to merge 6 commits intoicsharpcode:masterfrom
Draft
Claude/fix issue 557 char string default param#1255GrahamTheCoder wants to merge 6 commits intoicsharpcode:masterfrom
GrahamTheCoder wants to merge 6 commits intoicsharpcode:masterfrom
Conversation
GrahamTheCoder
commented
Apr 14, 2026
|
|
||
| // Replace the default value with null | ||
| updatedParams[i] = csParam.WithDefault( | ||
| CS.SyntaxFactory.EqualsValueClause(ValidSyntaxFactory.NullExpression)); |
Member
Author
There was a problem hiding this comment.
This pattern of mutating previously set values is not recommended. We should set it to be null at the point of creation, then separately here we should add the null coalesce and use comments to mention the other place. This is obviously also not ideal, but a conceptual dependency from a distance is generally better than a concrete one where someone else might intercept it in between in the same style otherwise.
…s null + null-coalescing assignment When VB has Optional ByVal strParam As String = charConst, C# can't use a char literal as a string default. Replace the default with null and prepend strParam = strParam ?? charConst.ToString() in the method body. https://claude.ai/code/session_01AkwUvu3XuCdj3D4axoX4UX
…r string parameter Two tests cover the fix for converting VB Optional string parameters with char default values: one using a const reference and one using an inline char literal. https://claude.ai/code/session_01AkwUvu3XuCdj3D4axoX4UX
…ts.cs The TestOverridableAutoPropertyBackingFieldAccessAsync test (issue icsharpcode#827) was mistakenly added to this branch, which only contains the fix for issue icsharpcode#557. The icsharpcode#827 fix code is in a separate PR (icsharpcode#1252) and is not present here, so the test would always fail. https://claude.ai/code/session_01AkwUvu3XuCdj3D4axoX4UX
…lt for String params ExplicitDefaultValue is normalized to the parameter's declared type, so checking `ExplicitDefaultValue is not char` was always true for String params. Instead, retrieve the VB syntax default expression and check its actual type via the semantic model. https://claude.ai/code/session_01AkwUvu3XuCdj3D4axoX4UX
…ix method VisitParameter now sets the default to null directly when it detects a char-typed default for a string parameter, rather than converting to a char expression that FixCharDefaultsForStringParams would later replace. FixCharDefaultsForStringParams now reconstructs the char expression from the VB syntax node via BuildCharExpressionFromVbSyntax, removing the previously-set-then-overwritten value pattern. https://claude.ai/code/session_01AkwUvu3XuCdj3D4axoX4UX
… in else branch The outer 'if (node.Default != null)' block already declares 'defaultValue' on line 449. The branch's new else-branch code was incorrectly redeclaring it, causing CS0136: A local variable named 'defaultValue' is already defined in this scope. Fix: remove the redundant declaration in the else branch and reuse the existing one. https://claude.ai/code/session_01AkwUvu3XuCdj3D4axoX4UX
GrahamTheCoder
force-pushed
the
claude/fix-issue-557-char-string-default-param
branch
from
43e33b4 to
c055eee
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #557