Claude/fix issue 557 char string default param#1255
Open
GrahamTheCoder wants to merge 9 commits intoicsharpcode:masterfrom
Open
Claude/fix issue 557 char string default param#1255GrahamTheCoder wants to merge 9 commits intoicsharpcode:masterfrom
GrahamTheCoder wants to merge 9 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.
GrahamTheCoder
force-pushed
the
claude/fix-issue-557-char-string-default-param
branch
2 times, most recently
from
49accae to
eca463f
Compare
GrahamTheCoder
commented
Apr 19, 2026
| // For named constant references (e.g. DlM or Module.DlM), build a member access expression. | ||
| // Strip VB's "Global." prefix (VB global namespace qualifier, has no C# identifier equivalent). | ||
| // Annotate for simplification so Roslyn reduces e.g. TestModule.DlM → DlM within TestModule. | ||
| var parts = defaultValueNode.ToString().Trim().Split('.'); |
Member
Author
There was a problem hiding this comment.
Reinventing the wheel
GrahamTheCoder
commented
Apr 19, 2026
| // Annotate for simplification so Roslyn reduces e.g. TestModule.DlM → DlM within TestModule. | ||
| var parts = defaultValueNode.ToString().Trim().Split('.'); | ||
| if (parts.Length > 1 && parts[0] == "Global") parts = parts.Skip(1).ToArray(); | ||
| return ValidSyntaxFactory.MemberAccess(parts).WithAdditionalAnnotations(Simplifier.Annotation); |
Member
Author
There was a problem hiding this comment.
The Simplifier.Annotation should already be added in the simplification pass
…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
…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
…g coalesce (strip VB Global prefix, add Simplifier annotation)
…Split('.')
Handle IdentifierNameSyntax, MemberAccessExpressionSyntax, and QualifiedNameSyntax
(which appears for Global.X references in default value context). Also remove
unnecessary Simplifier.Annotation since simplification is applied automatically
in a second phase.
https://claude.ai/code/session_01AkwUvu3XuCdj3D4axoX4UX
GrahamTheCoder
force-pushed
the
claude/fix-issue-557-char-string-default-param
branch
from
cc3c0d1 to
5c8966e
Compare
Replace the hand-rolled VbNameExprToCsExpr/BuildCharExpressionFromVbSyntax static helpers with a call to the existing expression visitor (_triviaConvertingExpressionVisitor) via AcceptAsync. This handles all VB expression types uniformly without a verbose manual switch. https://claude.ai/code/session_01AkwUvu3XuCdj3D4axoX4UX
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