[GrahamTheCoder]

Fixes icsharpcode#1301

Summary

Root cause: DeclarationNodeVisitor.VisitConstructorBlock treats a constructor's first statement as a call to another constructor when the member name's text is New. In MyBase.New, New is lexed as an identifier, so this text check is what does the work. It missed three forms, and each was left in the body as base.ctor(...); (CS0117/CS7036):

• Casing: HasMatchingText compared case-sensitively, so MyBase.new(value) and MYBASE.NEW(value) weren't recognised.
• No parentheses: a parameterless MyBase.New has no InvocationExpressionSyntax, so it was never looked at.
• Call keyword: Call MyBase.New(value) is a CallStatementSyntax, not an ExpressionStatementSyntax, so it was never looked at either.

Fix:

• HasMatchingText (CodeConverter/Util/SyntaxTokenExtensions.cs) now compares with OrdinalIgnoreCase:
  • VB keywords are case-insensitive, and the method only takes VB SyntaxKinds.
  • Its only caller is this constructor check, so nothing else is affected.
• A new GetConstructorInitializerCall decides whether the first statement is a constructor call. It was extracted in pure-refactor commits. It accepts:
  • an expression statement or a Call statement,
  • with or without parentheses,
  • only a simple member access (.) whose name is New and whose target is MyBase (→ : base(...)) or Me/MyClass (→ : this(...)).
  • It returns null for anything else. The arguments are converted only when it really is a constructor call.
• Dictionary access Me!New() (meaning Me("New")()) is no longer treated as a constructor call:
  • On master, exact-casing Me!New() already became : this(), a self-call (CS0516).
  • The casing and Call support would otherwise have widened that, and Call Me!New() would have compiled with a silent behaviour change.

Reviewer attention

• I kept the syntactic text check and did not switch to a semantic one (GetSymbolInfo resolving to MethodKind.Constructor):
  • In compiling VB, X.New as a statement is only valid when X is MyBase, Me or MyClass, so the text check is exact.
  • An escaped MyBase.[New](...) is not a constructor call: VB reports BC30456. The check ignores it correctly because the token text includes the brackets.
  • The syntactic check keeps working when semantic info is missing, for example an unresolved base type.
• The first commit on the branch still has its "WIP:" message. I didn't force-push to reword it. Squash-merge would tidy it.

Pre-existing, not fixed

• ByRef argument needing a temporary in a constructor initializer: the output is : base(ref ph<guid>) (CS0103), for any casing.
  • Example: base Sub New(ByRef r As Integer), derived MyBase.New(value + 1) or MyBase.New(SharedProp).
  • Before this PR, non-canonical casing gave base.ctor(...) in that case, so it was broken either way.
• Invoking a delegate returned through dictionary access as a statement: Me!New() converts to this["New"]; (CS0201), which drops the invocation. That's why its test only asserts that no constructor initializer is produced.

Review

The blind review converged after 3 rounds:

• Round 1 found the Call form and some missing tests.
• Round 2 found the Me!New dictionary-access bug and two quality points.
• Round 3 found only quality points and nitpicks. Two were applied: the switch now returns directly, and the self-verifying test's log labels were renamed.

Declined:

• Commit 8f1f379 is labelled "pure refactor" but no longer converts the arguments of a first statement that isn't a constructor call. The reviewer compared the output with master for ByRef temporaries, comments, lambdas, LINQ, : statement separators and line continuations. The output was identical in every case, so the label stands.
• A borderline doc-comment line about Call: kept, because it states why CallStatementSyntax is handled.

Tests

Tests/CSharp/MemberTests/ConstructorTests.cs has 10 new conversion tests. The first 9 compare against output that compiles (the harness reports any compile errors).

• MyBaseNewWithNonCanonicalCasingAsync: the exact repro from the issue.
• MyBaseNewWithUpperCasingAsync
• MyBaseNewWithoutParenthesesAsync: MyBase.nEw followed by another statement.
• MyBaseNewWithoutParenthesesKeepsCommentAsync: the trailing comment is kept.
• MeAndMyClassNewWithNonCanonicalCasingAsync: Me.new(...) and myclass.NEW(...) become : this(...).
• CallStatementConstructorInitializersWithNonCanonicalCasingAsync: Call MyBase.new(...), call Me.NEW(...) and CALL myclass.New(...).
• MyBaseNewWithNonCanonicalCasingNamedAndOmittedArgumentsAsync: named arguments, and an omitted optional argument.
• FirstStatementNotConstructorCallAsync: Me.Newish(New Base(value)) stays in the body.
• CallStatementNonConstructorWithoutParenthesesAsync: Call Me.Foo stays in the body.
• DictionaryAccessNamedNewIsNotConstructorCallAsync: Me!new() and Call Me!New() produce no constructor initializer. Because of the pre-existing body bug above, its output doesn't compile, so the test asserts on parts of the output rather than the whole text.

Tests/TestData/SelfVerifyingTests/VBToCS/InheritanceTests.vb adds the self-verifying test ConstructorInitializersWithNonCanonicalCasing:

• It runs the casing, no-parentheses, Me/MyClass chain and Call forms.
• It asserts which constructors ran and in what order.

Each behaviour test was seen failing before its fix: base.ctor(...)/CS0117, or : this() for Me!New.

Full suite vs master baseline

• Branch at 0266d0c: 1436 passed, 5 skipped, 0 failed (1441).
• Master baseline: 1425 passed, 5 skipped, 0 failed (1430).
• The difference is exactly the 11 new tests. No other changes.
• The last two commits (a refactor in the switch, and the test label renames) were checked with a filtered run of ConstructorTests and SelfVerifyingTests: 402 passed, 0 failed.
• During review, a FullyQualifiedName~CSharp run on the shared machine hung in MultiFileSolutionAndProjectTests.ConvertVbLibraryOnlyAsync. That test passes on its own on both this branch and master. The hang came from contention on the shared machine, not from this change.

Coverage of changed lines

All changed lines are covered, except the fallback for <other>.New where the target is not MyBase/Me/MyClass, which is invalid in VB. That fallback was the uncovered final else before the refactor.

🤖 Generated with Claude Code

https://claude.ai/code/session_019FEr8FVAmHV9bscZ82CChg

[GrahamTheCoder]

Will merge as icsharpcode#1313 once edge cases around Me!new() and similar are fixed/tested.