From 351cd1439ed3585fbdc532b85fca185da5dd26d9 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 14 Oct 2026 03:29:36 +0000 Subject: [PATCH 1/3] Write C# member modifiers only where C# allows them [patch] WriteFunctionModifiers wrote the AST's modifiers without looking at the enclosing type, so ordinary C++-shaped declarations produced C# that does not compile: - IsReadOnly became `readonly` on class and interface members (CS0106). It is now spelled only on an instance member of a struct; anywhere else the claim is written down in a note, as a readonly class's is. - A destructor got an access modifier (`public ~Vec()`, CS0106). It now gets none, and inside a struct, which cannot have a finalizer (CS0575), it is written down rather than declared. - An abstract member left its class unmarked (CS0513). A class with an abstract member is now `abstract`, and an interface member drops the `abstract` it already has implicitly. GenerateFunction now takes the enclosing ClassDeclaration rather than its name so the modifiers can see what kind of type they are in. Fixes ktsu-dev/Coder#127 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_018FP1DMG4WYMABYwTfyb2Xd --- Coder.Test/Ast/FunctionShapeTests.cs | 7 +- .../Languages/CSharpMemberModifierTests.cs | 139 ++++++++++++++++++ Coder/Languages/CSharpGenerator.cs | 84 +++++++++-- 3 files changed, 213 insertions(+), 17 deletions(-) create mode 100644 Coder.Test/Languages/CSharpMemberModifierTests.cs diff --git a/Coder.Test/Ast/FunctionShapeTests.cs b/Coder.Test/Ast/FunctionShapeTests.cs index 7772bc1..27dbfe5 100644 --- a/Coder.Test/Ast/FunctionShapeTests.cs +++ b/Coder.Test/Ast/FunctionShapeTests.cs @@ -103,15 +103,18 @@ public void Cpp_SpellsEachKindOfDefinition() } /// - /// A trailing const says a call leaves the receiver unchanged, which C# spells readonly. + /// A trailing const says a call leaves the receiver unchanged, which C# spells readonly on the + /// one kind of member that may carry it, an instance member of a struct. /// [TestMethod] public void ReadOnly_IsSpelledWhereALanguageHasIt() { FunctionDeclaration find = new("find") { ReturnType = "int", IsReadOnly = true }; + ClassDeclaration index = new("Index") { Kind = TypeDeclarationKind.Struct }; + index.Members.Add(find); Assert.Contains("int find() const", new CppGenerator().Generate(find), StringComparison.Ordinal); - Assert.Contains("readonly int find(", new CSharpGenerator().Generate(find), StringComparison.Ordinal); + Assert.Contains("readonly int find(", new CSharpGenerator().Generate(index), StringComparison.Ordinal); } /// diff --git a/Coder.Test/Languages/CSharpMemberModifierTests.cs b/Coder.Test/Languages/CSharpMemberModifierTests.cs new file mode 100644 index 0000000..af5e867 --- /dev/null +++ b/Coder.Test/Languages/CSharpMemberModifierTests.cs @@ -0,0 +1,139 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.Coder.Test.Languages; + +using ktsu.Coder.Ast; +using ktsu.Coder.Languages; +using Microsoft.VisualStudio.TestTools.UnitTesting; + +/// +/// Tests that a member's modifiers are written only where C# allows them. +/// +/// +/// The AST's modifiers are C++'s: a trailing const on any member function, a pure virtual +/// member in an otherwise ordinary class, a destructor with an access specifier. C# allows +/// readonly only on an instance member of a struct (CS0106), no access modifier on a finalizer +/// (CS0106), no finalizer in a struct (CS0575), and an abstract member only in a class that says it is +/// abstract (CS0513). +/// +[TestClass] +public class CSharpMemberModifierTests +{ + /// + /// Tests that a class method that does not modify its instance says so in a note rather than with + /// readonly. + /// + [TestMethod] + public void ReadOnlyClassMethod_IsNotSpelledReadonly() + { + ClassDeclaration counter = new("Counter"); + counter.Members.Add(new FieldDeclaration("count", new TypeReference("int"))); + FunctionDeclaration get = new("get") { ReturnType = "int", IsReadOnly = true }; + get.Body.Add(new ReturnStatement(new VariableReference("count"))); + counter.Members.Add(get); + + string code = new CSharpGenerator().Generate(counter); + + Assert.Contains("public int get()", code, StringComparison.Ordinal, code); + Assert.DoesNotContain("readonly int get(", code, StringComparison.Ordinal, code); + Assert.Contains("// readonly: get does not modify its instance", code, StringComparison.Ordinal, code); + } + + /// + /// Tests that an instance member of a struct is still spelled readonly, and a static one is + /// not. + /// + [TestMethod] + public void ReadOnlyStructMethod_IsSpelledReadonly() + { + ClassDeclaration point = new("Point") { Kind = TypeDeclarationKind.Struct }; + point.Members.Add(new FieldDeclaration("x", new TypeReference("int"))); + FunctionDeclaration getX = new("getX") { ReturnType = "int", IsReadOnly = true }; + getX.Body.Add(new ReturnStatement(new VariableReference("x"))); + point.Members.Add(getX); + FunctionDeclaration origin = new("origin") { ReturnType = "int", IsReadOnly = true, IsStatic = true }; + origin.Body.Add(new ReturnStatement(Literal.Number(0))); + point.Members.Add(origin); + + string code = new CSharpGenerator().Generate(point); + + Assert.Contains("public readonly int getX()", code, StringComparison.Ordinal, code); + Assert.Contains("public static int origin()", code, StringComparison.Ordinal, code); + Assert.DoesNotContain("readonly:", code, StringComparison.Ordinal, code); + } + + /// + /// Tests that an interface member is neither readonly nor written abstract, which an + /// interface member with no body already is. + /// + [TestMethod] + public void InterfaceMember_HasNeitherReadonlyNorAbstract() + { + string code = new CSharpGenerator().Generate(CompiledExemplar.Shape()); + + Assert.Contains("public double area();", code, StringComparison.Ordinal, code); + Assert.Contains("public void draw(double scale);", code, StringComparison.Ordinal, code); + Assert.DoesNotContain("abstract", code, StringComparison.Ordinal, code); + Assert.DoesNotContain("readonly double", code, StringComparison.Ordinal, code); + } + + /// + /// Tests that a class with an abstract member is itself abstract. + /// + [TestMethod] + public void ClassWithAbstractMember_IsAbstract() + { + ClassDeclaration animal = new("Animal"); + animal.Members.Add(new FunctionDeclaration("speak") { ReturnType = "string", IsAbstract = true }); + + string code = new CSharpGenerator().Generate(animal); + + Assert.Contains("public abstract class Animal", code, StringComparison.Ordinal, code); + Assert.Contains("public abstract string speak();", code, StringComparison.Ordinal, code); + } + + /// + /// Tests that a class with no abstract member is not marked abstract. + /// + [TestMethod] + public void ClassWithoutAbstractMember_IsNotAbstract() + { + ClassDeclaration animal = new("Animal"); + animal.Members.Add(new FunctionDeclaration("speak") { ReturnType = "string", IsVirtual = true }); + + string code = new CSharpGenerator().Generate(animal); + + Assert.Contains("public class Animal", code, StringComparison.Ordinal, code); + Assert.DoesNotContain("abstract", code, StringComparison.Ordinal, code); + } + + /// + /// Tests that a class's destructor is a finalizer with no access modifier. + /// + [TestMethod] + public void ClassDestructor_HasNoAccessModifier() + { + ClassDeclaration vec = new("Vec"); + vec.Members.Add(new FunctionDeclaration("Vec") { Kind = FunctionKind.Destructor }); + + string code = new CSharpGenerator().Generate(vec); + + Assert.Contains("~Vec()", code, StringComparison.Ordinal, code); + Assert.DoesNotContain("public ~Vec", code, StringComparison.Ordinal, code); + } + + /// + /// Tests that a struct's destructor is written down rather than declared. + /// + [TestMethod] + public void StructDestructor_IsANote() + { + ClassDeclaration vec = new("Vec") { Kind = TypeDeclarationKind.Struct }; + vec.Members.Add(new FunctionDeclaration("Vec") { Kind = FunctionKind.Destructor }); + + string code = new CSharpGenerator().Generate(vec); + + Assert.Contains("// ~Vec is a destructor, which a C# struct cannot declare.", code, StringComparison.Ordinal, code); + Assert.DoesNotContain("~Vec()", code, StringComparison.Ordinal, code); + } +} diff --git a/Coder/Languages/CSharpGenerator.cs b/Coder/Languages/CSharpGenerator.cs index 14087d8..825b39f 100644 --- a/Coder/Languages/CSharpGenerator.cs +++ b/Coder/Languages/CSharpGenerator.cs @@ -200,13 +200,14 @@ private void GenerateClass(ClassDeclaration classDecl, CodeBlocker code) _ => "class", }; - // In the order C# takes them: access, then the promise the type makes about itself, then - // the permission to declare the rest of it elsewhere, then what kind of type it is. A - // record is written before the keyword rather than instead of it, which is what makes - // `record struct` reachable. + // In the order C# takes them: access, then whether it can be instantiated, then the promise + // the type makes about itself, then the permission to declare the rest of it elsewhere, then + // what kind of type it is. A record is written before the keyword rather than instead of it, + // which is what makes `record struct` reachable. string[] modifiers = [ SpellVisibility(classDecl.Visibility) ?? DefaultVisibility, + .. IsAbstractClass(classDecl) ? (string[])["abstract"] : [], .. isReadOnlyStruct ? (string[])["readonly"] : [], .. classDecl.IsPartial ? (string[])["partial"] : [], .. classDecl.IsRecord ? (string[])["record"] : [], @@ -241,7 +242,7 @@ .. classDecl.Interfaces.Select(MapToCSType), { if (member is FunctionDeclaration method) { - GenerateFunction(method, code, classDecl.Name); + GenerateFunction(method, code, classDecl); } else { @@ -666,7 +667,7 @@ private void GenerateFunction(FunctionDeclaration function, CodeBlocker code) => /// /// The declaration to emit. /// The writer to emit into. - /// The name of the type it belongs to, when it belongs to one. + /// The type it belongs to, when it belongs to one. /// /// C# expresses a defaulted or deleted member by not declaring it: the compiler supplies the one /// and the absence of the other is what makes a call fail to compile. So neither is emitted, and @@ -678,22 +679,39 @@ private void GenerateFunction(FunctionDeclaration function, CodeBlocker code) => /// = default means there. /// /// - private void GenerateFunction(FunctionDeclaration function, CodeBlocker code, string? enclosingType) + private void GenerateFunction(FunctionDeclaration function, CodeBlocker code, ClassDeclaration? enclosingType) { WritePreamble(function, code, enclosingType is null ? PreambleSite.Function : PreambleSite.Method); + string name = SpellFunctionName(function, enclosingType?.Name); + if (function.Definition != FunctionDefinition.Provided && !(function.Kind == FunctionKind.Constructor && function.Definition == FunctionDefinition.Defaulted)) { string state = function.Definition == FunctionDefinition.Defaulted ? "supplied by the language" : "deleted"; - WriteInexpressible(code, $"{SpellFunctionName(function, enclosingType)} is {state}, which C# expresses by not declaring it."); + WriteInexpressible(code, $"{name} is {state}, which C# expresses by not declaring it."); + return; + } + + // A struct cannot have a finalizer (CS0575), so the destructor is written down rather than + // declared, the way a member C# expresses by not declaring it is. + if (function.Kind == FunctionKind.Destructor && enclosingType?.Kind == TypeDeclarationKind.Struct) + { + WriteInexpressible(code, $"{name} is a destructor, which a C# struct cannot declare."); return; } + // readonly is a modifier only an instance member of a struct may carry (CS0106). Anywhere else + // the member still makes the claim, so it is written down the way the type's own claim is. + if (function.IsReadOnly && !function.IsStatic && !IsReadOnlySpelled(function, enclosingType)) + { + WriteInexpressible(code, $"readonly: {name} does not modify its instance"); + } + WriteFunctionAttributes(function, code); - WriteFunctionModifiers(function, code); + WriteFunctionModifiers(function, code, enclosingType); - code.Write(SpellFunctionName(function, enclosingType)); + code.Write(name); code.Write(SpellTypeParameters(function.TypeParameters)); code.Write("("); @@ -775,11 +793,16 @@ private static void WriteFunctionAttributes(FunctionDeclaration function, CodeBl /// /// The declaration being emitted. /// The writer to emit into. - private static void WriteFunctionModifiers(FunctionDeclaration function, CodeBlocker code) + /// The type it belongs to, when it belongs to one. + private static void WriteFunctionModifiers(FunctionDeclaration function, CodeBlocker code, ClassDeclaration? enclosingType) { // A function nobody has given a visibility to is public: an inaccessible method is not what - // someone who wrote no modifier meant. - code.Write($"{SpellVisibility(function.Visibility) ?? DefaultVisibility} "); + // someone who wrote no modifier meant. A finalizer is the exception, because it takes no + // access modifier at all (CS0106): only the runtime calls it. + if (function.Kind != FunctionKind.Destructor) + { + code.Write($"{SpellVisibility(function.Visibility) ?? DefaultVisibility} "); + } if (function.IsStatic || function.Kind is FunctionKind.Operator or FunctionKind.ConversionOperator) { @@ -787,16 +810,20 @@ private static void WriteFunctionModifiers(FunctionDeclaration function, CodeBlo code.Write("static "); } + // An interface member with no body is abstract already, and says so by having none. if (function.IsAbstract) { - code.Write("abstract "); + if (enclosingType?.Kind != TypeDeclarationKind.Interface) + { + code.Write("abstract "); + } } else if (function.IsVirtual) { code.Write("virtual "); } - if (function.IsReadOnly) + if (IsReadOnlySpelled(function, enclosingType)) { code.Write("readonly "); } @@ -808,6 +835,33 @@ private static void WriteFunctionModifiers(FunctionDeclaration function, CodeBlo } } + /// + /// Determines whether a declaration's is spelled as + /// C#'s readonly. + /// + /// The declaration being emitted. + /// The type it belongs to, when it belongs to one. + /// True when the declaration is an instance member of a struct that claims it. + /// + /// The claim is C++'s trailing const, which any member function may make. C# lets only an + /// instance member of a struct make it, so everywhere else readonly fails with CS0106. + /// + private static bool IsReadOnlySpelled(FunctionDeclaration function, ClassDeclaration? enclosingType) => + function.IsReadOnly && !function.IsStatic && enclosingType?.Kind == TypeDeclarationKind.Struct; + + /// + /// Determines whether a class is written abstract. + /// + /// The declaration being emitted. + /// True when it is a class with a member that has no definition. + /// + /// The AST says abstract of a member, as C++ does, and a C++ class with a pure virtual member is + /// abstract without saying so. C# requires the class to say it (CS0513). + /// + private static bool IsAbstractClass(ClassDeclaration classDecl) => + classDecl.Kind == TypeDeclarationKind.Class + && classDecl.Members.OfType().Any(member => member.IsAbstract); + /// /// Spells the name a declaration is written under. /// From 7fd3070b226d7c0243d7a0cf83dff962b01f393f Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 14 Oct 2026 03:49:44 +0000 Subject: [PATCH 2/3] Move a C# member's notes out of GenerateFunction Sonar flagged GenerateFunction's cognitive complexity (S3776, 19 of 15) after the destructor and readonly notes were added to it. They now live in WriteMemberNotes; the output is unchanged. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_018FP1DMG4WYMABYwTfyb2Xd --- Coder/Languages/CSharpGenerator.cs | 40 ++++++++++++++++++++++-------- 1 file changed, 29 insertions(+), 11 deletions(-) diff --git a/Coder/Languages/CSharpGenerator.cs b/Coder/Languages/CSharpGenerator.cs index 825b39f..57ee94f 100644 --- a/Coder/Languages/CSharpGenerator.cs +++ b/Coder/Languages/CSharpGenerator.cs @@ -693,21 +693,11 @@ private void GenerateFunction(FunctionDeclaration function, CodeBlocker code, Cl return; } - // A struct cannot have a finalizer (CS0575), so the destructor is written down rather than - // declared, the way a member C# expresses by not declaring it is. - if (function.Kind == FunctionKind.Destructor && enclosingType?.Kind == TypeDeclarationKind.Struct) + if (!WriteMemberNotes(function, name, enclosingType, code)) { - WriteInexpressible(code, $"{name} is a destructor, which a C# struct cannot declare."); return; } - // readonly is a modifier only an instance member of a struct may carry (CS0106). Anywhere else - // the member still makes the claim, so it is written down the way the type's own claim is. - if (function.IsReadOnly && !function.IsStatic && !IsReadOnlySpelled(function, enclosingType)) - { - WriteInexpressible(code, $"readonly: {name} does not modify its instance"); - } - WriteFunctionAttributes(function, code); WriteFunctionModifiers(function, code, enclosingType); @@ -765,6 +755,34 @@ private void GenerateFunction(FunctionDeclaration function, CodeBlocker code, Cl } } + /// + /// Writes down what a member asks for that C# cannot say where the member is. + /// + /// The declaration being emitted. + /// The name it is written under. + /// The type it belongs to, when it belongs to one. + /// The writer to emit into. + /// False when the note stands in for the whole declaration, so nothing else is written. + private bool WriteMemberNotes(FunctionDeclaration function, string name, ClassDeclaration? enclosingType, CodeBlocker code) + { + // A struct cannot have a finalizer (CS0575), so the destructor is written down rather than + // declared, the way a member C# expresses by not declaring it is. + if (function.Kind == FunctionKind.Destructor && enclosingType?.Kind == TypeDeclarationKind.Struct) + { + WriteInexpressible(code, $"{name} is a destructor, which a C# struct cannot declare."); + return false; + } + + // readonly is a modifier only an instance member of a struct may carry (CS0106). Anywhere else + // the member still makes the claim, so it is written down the way the type's own claim is. + if (function.IsReadOnly && !function.IsStatic && !IsReadOnlySpelled(function, enclosingType)) + { + WriteInexpressible(code, $"readonly: {name} does not modify its instance"); + } + + return true; + } + /// /// Writes the attributes a declaration earns. /// From a5d9eea73af3f858a71d5ce6a2ad354fb164bd19 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 14 Oct 2026 04:08:54 +0000 Subject: [PATCH 3/3] Move a C# constructor's initialisers out of GenerateFunction Sonar still scored GenerateFunction at 16 of 15 (S3776) after the member notes moved out. The initialiser assignments now live in WriteInitialisers; the output is unchanged. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_018FP1DMG4WYMABYwTfyb2Xd --- Coder/Languages/CSharpGenerator.cs | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/Coder/Languages/CSharpGenerator.cs b/Coder/Languages/CSharpGenerator.cs index 57ee94f..04e03f3 100644 --- a/Coder/Languages/CSharpGenerator.cs +++ b/Coder/Languages/CSharpGenerator.cs @@ -729,7 +729,21 @@ private void GenerateFunction(FunctionDeclaration function, CodeBlocker code, Cl WriteConstraintClauses(function.TypeParameters, code); using Scope body = new(code); + WriteInitialisers(function, code); + foreach (AstNode statement in function.Body) + { + GenerateInternal(statement, code); + } + } + + /// + /// Writes a constructor's member initialisers as the assignments that open its body. + /// + /// The declaration being emitted. + /// The writer to emit into. + private void WriteInitialisers(FunctionDeclaration function, CodeBlocker code) + { // C# assigns where C++ initialises. Written before the body's own statements and in the order // declared, which is what the initialiser means where there is no initialiser list. One with no // value value-initialises the member, as C++'s `x()` does, and `default` is C#'s spelling. @@ -748,11 +762,6 @@ private void GenerateFunction(FunctionDeclaration function, CodeBlocker code, Cl EndStatement(code); } - - foreach (AstNode statement in function.Body) - { - GenerateInternal(statement, code); - } } ///