From e59ae417e6385bb1d61febdf22c2564fdc63dc87 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 12 Sep 2026 09:22:43 +0000 Subject: [PATCH] Address the SonarCloud findings from #57 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The quality gate passed on #57 and it merged before these landed, so all 24 findings are in code that is now on main. One of them is a real defect rather than a style note. S2699, reported as a blocker, was right: WhatThePaletteCreates_IsReadyToConnect reads every slot of every new node and asserts nothing about what comes back. It also has no guard against its own filter matching nothing, so it would pass just as happily if the palette stopped offering these nodes entirely — which is the regression it exists to catch. It now asserts that all three templates are found, and that every operand already sitting in a slot is the placeholder the rest of the library understands rather than a null it does not. S1192 asked for constants where a literal repeats. AstSchema already keeps one for the arguments slot's name and the new slots now have theirs; the palette's category names and the serializer's expectedType key follow the same convention their own files already set. The MSTest analyzers asked for the assertions that say what they mean: HasCount over AreEqual on a count, IsEmpty over AreEqual against zero, AreSequenceEqual over CollectionAssert, and Contains over IsTrue around a predicate. All four are already used elsewhere in this suite, so these match the surrounding code rather than introducing a second style. MSTEST0046 is left as it was, and that is the one deliberate exception. It prefers Assert.Contains over StringAssert.Contains, but this suite calls StringAssert.Contains in 188 places and has no bare Assert.Contains anywhere. Two call sites written the other way would read as a mistake rather than as an improvement. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01QGCMUrT3jBgmANHNHcPmBf --- Coder.Graph/AstNodeCatalog.cs | 50 ++++++++++------- Coder.Graph/AstSchema.cs | 55 ++++++++++++------- Coder.Test/Ast/CallExpressionTests.cs | 6 +- Coder.Test/Ast/ExpressionStatementTests.cs | 4 +- .../Graph/CallAndConditionalSlotsTests.cs | 55 +++++++++++++------ Coder/Serialization/YamlSerializer.cs | 15 +++-- 6 files changed, 117 insertions(+), 68 deletions(-) diff --git a/Coder.Graph/AstNodeCatalog.cs b/Coder.Graph/AstNodeCatalog.cs index fcb7500..0ef779a 100644 --- a/Coder.Graph/AstNodeCatalog.cs +++ b/Coder.Graph/AstNodeCatalog.cs @@ -39,47 +39,59 @@ public sealed record AstNodeTemplate(string Category, string Label, Func public static class AstNodeCatalog { + /// The category a declaration is listed under. + private const string Declarations = "Declarations"; + + /// The category a statement is listed under. + private const string Statements = "Statements"; + + /// The category an expression is listed under. + private const string Expressions = "Expressions"; + + /// The category a literal is listed under. + private const string Literals = "Literals"; + /// /// Gets every node the palette offers, grouped and ordered for display. /// public static IReadOnlyList Templates { get; } = [ - new("Declarations", "Class", () => new ClassDeclaration("NewClass")), - new("Declarations", "Function", () => new FunctionDeclaration("newFunction") { ReturnType = "void" }), - new("Declarations", "Parameter", () => new Parameter("value", "int")), - new("Declarations", "Variable", () => new VariableDeclaration("value", "int")), - new("Declarations", "Constant", () => new VariableDeclaration("VALUE", "int", Literal.Number(0)) { IsConstant = true }), - new("Declarations", "Entry point", () => new EntryPoint()), - - new("Statements", "Return", () => new ReturnStatement()), - new("Statements", "Expression", () => new ExpressionStatement()), + new(Declarations, "Class", () => new ClassDeclaration("NewClass")), + new(Declarations, "Function", () => new FunctionDeclaration("newFunction") { ReturnType = "void" }), + new(Declarations, "Parameter", () => new Parameter("value", "int")), + new(Declarations, "Variable", () => new VariableDeclaration("value", "int")), + new(Declarations, "Constant", () => new VariableDeclaration("VALUE", "int", Literal.Number(0)) { IsConstant = true }), + new(Declarations, "Entry point", () => new EntryPoint()), + + new(Statements, "Return", () => new ReturnStatement()), + new(Statements, "Expression", () => new ExpressionStatement()), .. Enum.GetValues().Select(op => new AstNodeTemplate( - "Statements", + Statements, Spell(op), () => new AssignmentStatement(new VariableReference("target"), AstSchema.Unfilled(), op), "Assignment")), .. Enum.GetValues().Select(op => new AstNodeTemplate( - "Expressions", + Expressions, Spell(op), () => new BinaryExpression(AstSchema.Unfilled(), op, AstSchema.Unfilled()), "Binary")), .. Enum.GetValues().Select(op => new AstNodeTemplate( - "Expressions", + Expressions, Spell(op), () => new UnaryExpression(op, AstSchema.Unfilled()), "Unary")), - new("Expressions", "Variable reference", () => new VariableReference("value")), - new("Expressions", "Call", () => new CallExpression("function")), - new("Expressions", "Conditional", () => new ConditionalExpression( + new(Expressions, "Variable reference", () => new VariableReference("value")), + new(Expressions, "Call", () => new CallExpression("function")), + new(Expressions, "Conditional", () => new ConditionalExpression( AstSchema.Unfilled(), AstSchema.Unfilled(), AstSchema.Unfilled())), - new("Literals", "Text", () => Literal.Text("text")), - new("Literals", "Number", () => Literal.Number(0)), - new("Literals", "Decimal", () => Literal.DecimalValue(0)), - new("Literals", "Boolean", () => Literal.Bool(true)), + new(Literals, "Text", () => Literal.Text("text")), + new(Literals, "Number", () => Literal.Number(0)), + new(Literals, "Decimal", () => Literal.DecimalValue(0)), + new(Literals, "Boolean", () => Literal.Bool(true)), ]; /// diff --git a/Coder.Graph/AstSchema.cs b/Coder.Graph/AstSchema.cs index 1c1232c..7f02872 100644 --- a/Coder.Graph/AstSchema.cs +++ b/Coder.Graph/AstSchema.cs @@ -38,12 +38,27 @@ public static class AstSchema /// The name of the slot an expression's arguments sit in. private const string ArgumentsSlotName = "Arguments"; + /// The name of the slot a call's receiver sits in. + private const string ReceiverSlotName = "Receiver"; + + /// The name of the slot a conditional's condition sits in. + private const string ConditionSlotName = "Condition"; + + /// The name of the slot a conditional's chosen-when-true value sits in. + private const string WhenTrueSlotName = "WhenTrue"; + + /// The name of the slot a conditional's chosen-when-false value sits in. + private const string WhenFalseSlotName = "WhenFalse"; + + /// What a caption calls a declaration that has not been named yet. + private const string Unnamed = ""; + private static readonly AstSlot ArgumentsSlot = new(ArgumentsSlotName, AstSlotCardinality.Many, AstSlotKind.Expression); private static readonly AstSlot EnumMembersSlot = new("Members", AstSlotCardinality.Many, AstSlotKind.EnumMember); - private static readonly AstSlot ReceiverSlot = new("Receiver", AstSlotCardinality.One, AstSlotKind.Expression); - private static readonly AstSlot ConditionSlot = new("Condition", AstSlotCardinality.One, AstSlotKind.Expression); - private static readonly AstSlot WhenTrueSlot = new("WhenTrue", AstSlotCardinality.One, AstSlotKind.Expression); - private static readonly AstSlot WhenFalseSlot = new("WhenFalse", AstSlotCardinality.One, AstSlotKind.Expression); + private static readonly AstSlot ReceiverSlot = new(ReceiverSlotName, AstSlotCardinality.One, AstSlotKind.Expression); + private static readonly AstSlot ConditionSlot = new(ConditionSlotName, AstSlotCardinality.One, AstSlotKind.Expression); + private static readonly AstSlot WhenTrueSlot = new(WhenTrueSlotName, AstSlotCardinality.One, AstSlotKind.Expression); + private static readonly AstSlot WhenFalseSlot = new(WhenFalseSlotName, AstSlotCardinality.One, AstSlotKind.Expression); /// /// Lists the slots a node exposes, in the order the editor should draw them. @@ -95,10 +110,10 @@ public static IReadOnlyList ChildrenOf(AstNode node, AstSlot slot) (MemberInitialiser initialiser, "Value") => initialiser.Value, (AssignmentStatement assignment, "Target") => assignment.Target, (AssignmentStatement assignment, "Value") => assignment.Value, - (CallExpression callExpr, "Receiver") => callExpr.Receiver, - (ConditionalExpression conditional, "Condition") => conditional.Condition, - (ConditionalExpression conditional, "WhenTrue") => conditional.WhenTrue, - (ConditionalExpression conditional, "WhenFalse") => conditional.WhenFalse, + (CallExpression callExpr, ReceiverSlotName) => callExpr.Receiver, + (ConditionalExpression conditional, ConditionSlotName) => conditional.Condition, + (ConditionalExpression conditional, WhenTrueSlotName) => conditional.WhenTrue, + (ConditionalExpression conditional, WhenFalseSlotName) => conditional.WhenFalse, (ExpressionStatement statement, "Expression") => statement.Expression, _ => null, }; @@ -188,19 +203,19 @@ private static bool TryAttachOperand(AstNode parent, AstSlot slot, AstNode child unary.Operand = operandExpr; return true; - case (CallExpression callExpr, "Receiver") when child is Expression receiverExpr: + case (CallExpression callExpr, ReceiverSlotName) when child is Expression receiverExpr: callExpr.Receiver = receiverExpr; return true; - case (ConditionalExpression conditional, "Condition") when child is Expression conditionExpr: + case (ConditionalExpression conditional, ConditionSlotName) when child is Expression conditionExpr: conditional.Condition = conditionExpr; return true; - case (ConditionalExpression conditional, "WhenTrue") when child is Expression whenTrueExpr: + case (ConditionalExpression conditional, WhenTrueSlotName) when child is Expression whenTrueExpr: conditional.WhenTrue = whenTrueExpr; return true; - case (ConditionalExpression conditional, "WhenFalse") when child is Expression whenFalseExpr: + case (ConditionalExpression conditional, WhenFalseSlotName) when child is Expression whenFalseExpr: conditional.WhenFalse = whenFalseExpr; return true; @@ -467,20 +482,20 @@ public static bool TryDetachAt(AstNode parent, AstSlot slot, int index) // A receiver is genuinely optional -- a call with none is a free function rather than an // unfinished member call -- so detaching one clears it instead of leaving a placeholder. - case (CallExpression callExpr, "Receiver"): + case (CallExpression callExpr, ReceiverSlotName): bool hadReceiver = callExpr.Receiver is not null; callExpr.Receiver = null; return hadReceiver; - case (ConditionalExpression conditional, "Condition"): + case (ConditionalExpression conditional, ConditionSlotName): conditional.Condition = Unfilled(); return true; - case (ConditionalExpression conditional, "WhenTrue"): + case (ConditionalExpression conditional, WhenTrueSlotName): conditional.WhenTrue = Unfilled(); return true; - case (ConditionalExpression conditional, "WhenFalse"): + case (ConditionalExpression conditional, WhenFalseSlotName): conditional.WhenFalse = Unfilled(); return true; @@ -589,12 +604,12 @@ public static string Describe(AstNode node) return node switch { - ClassDeclaration classDecl => $"class {classDecl.Name ?? ""}", - FunctionDeclaration function => $"function {function.Name ?? ""}", + ClassDeclaration classDecl => $"class {classDecl.Name ?? Unnamed}", + FunctionDeclaration function => $"function {function.Name ?? Unnamed}", EntryPoint => "entry point", - Parameter parameter => $"param {parameter.Name ?? ""}", + Parameter parameter => $"param {parameter.Name ?? Unnamed}", ReturnStatement => "return", - CallExpression callExpr => $"call {(callExpr.Callee.Length == 0 ? "" : callExpr.Callee)}", + CallExpression callExpr => $"call {(callExpr.Callee.Length == 0 ? Unnamed : callExpr.Callee)}", ConditionalExpression => "conditional", ExpressionStatement => "expression", BinaryExpression binary => $"binary {SpellOrName(binary.Operator)}", diff --git a/Coder.Test/Ast/CallExpressionTests.cs b/Coder.Test/Ast/CallExpressionTests.cs index c5cce22..816d336 100644 --- a/Coder.Test/Ast/CallExpressionTests.cs +++ b/Coder.Test/Ast/CallExpressionTests.cs @@ -26,7 +26,7 @@ public void CallExpression_ShouldCreateCorrectStructure() Assert.AreEqual("sqrt", call.Callee); Assert.IsNull(call.Receiver); - Assert.AreEqual(1, call.Arguments.Count); + Assert.HasCount(1, call.Arguments); } /// @@ -75,7 +75,7 @@ public void CallExpression_Clone_IsDeep() Assert.IsNotNull(clone.Receiver); Assert.AreNotSame(original.Receiver, clone.Receiver); Assert.AreEqual("point", ((VariableReference)clone.Receiver).Name); - Assert.AreEqual(2, clone.Arguments.Count); + Assert.HasCount(2, clone.Arguments); Assert.AreNotSame(original.Arguments[0], clone.Arguments[0]); } @@ -210,7 +210,7 @@ public void CallExpression_RoundTripsThroughYaml() Assert.AreEqual("void", roundTripped.ExpectedType); Assert.IsInstanceOfType(roundTripped.Receiver); Assert.AreEqual("point", ((VariableReference)roundTripped.Receiver!).Name); - Assert.AreEqual(2, roundTripped.Arguments.Count); + Assert.HasCount(2, roundTripped.Arguments); Assert.AreEqual("point.translate(dx, 3)", new CSharpGenerator().Generate(roundTripped)); } diff --git a/Coder.Test/Ast/ExpressionStatementTests.cs b/Coder.Test/Ast/ExpressionStatementTests.cs index 131a346..f6d91a4 100644 --- a/Coder.Test/Ast/ExpressionStatementTests.cs +++ b/Coder.Test/Ast/ExpressionStatementTests.cs @@ -99,8 +99,8 @@ public void VoidCall_ReachesAFunctionBody() string generated = new CSharpGenerator().Generate(function); - StringAssert.Contains(generated, "assert(ready);"); - StringAssert.Contains(generated, "items.clear();"); + StringAssert.Contains(generated, "assert(ready);", StringComparison.Ordinal); + StringAssert.Contains(generated, "items.clear();", StringComparison.Ordinal); } /// diff --git a/Coder.Test/Graph/CallAndConditionalSlotsTests.cs b/Coder.Test/Graph/CallAndConditionalSlotsTests.cs index 85c8216..b293161 100644 --- a/Coder.Test/Graph/CallAndConditionalSlotsTests.cs +++ b/Coder.Test/Graph/CallAndConditionalSlotsTests.cs @@ -37,17 +37,17 @@ private static AstSlot Slot(AstNode node, string name) => [TestMethod] public void SlotsOf_DescribesTheNewNodes() { - CollectionAssert.AreEqual( + Assert.AreSequenceEqual( CallSlots, - AstSchema.SlotsOf(new CallExpression("f")).Select(slot => slot.Name).ToArray()); + AstSchema.SlotsOf(new CallExpression("f")).Select(slot => slot.Name)); - CollectionAssert.AreEqual( + Assert.AreSequenceEqual( ConditionalSlots, - AstSchema.SlotsOf(NewConditional()).Select(slot => slot.Name).ToArray()); + AstSchema.SlotsOf(NewConditional()).Select(slot => slot.Name)); - CollectionAssert.AreEqual( + Assert.AreSequenceEqual( ExpressionStatementSlots, - AstSchema.SlotsOf(new ExpressionStatement()).Select(slot => slot.Name).ToArray()); + AstSchema.SlotsOf(new ExpressionStatement()).Select(slot => slot.Name)); } /// @@ -59,7 +59,7 @@ public void ChildrenOf_ReportsAnAbsentReceiverAsEmpty() { CallExpression call = new("sqrt"); - Assert.AreEqual(0, AstSchema.ChildrenOf(call, Slot(call, "Receiver")).Count); + Assert.IsEmpty(AstSchema.ChildrenOf(call, Slot(call, "Receiver"))); } /// @@ -76,7 +76,7 @@ public void TryAttach_FillsACall() Assert.IsTrue(AstSchema.TryAttach(call, Slot(call, "Arguments"), new VariableReference("dy"))); Assert.AreSame(receiver, call.Receiver); - Assert.AreEqual(2, call.Arguments.Count); + Assert.HasCount(2, call.Arguments); Assert.AreEqual("dx", ((VariableReference)call.Arguments[0]).Name); Assert.AreEqual("dy", ((VariableReference)call.Arguments[1]).Name); } @@ -95,7 +95,7 @@ public void TryAttachAt_SwapsAnArgumentInPlace() Assert.IsTrue(AstSchema.TryAttachAt(call, Slot(call, "Arguments"), 0, new VariableReference("z"))); - Assert.AreEqual(2, call.Arguments.Count); + Assert.HasCount(2, call.Arguments); Assert.AreEqual("z", ((VariableReference)call.Arguments[0]).Name); Assert.AreEqual("b", ((VariableReference)call.Arguments[1]).Name); } @@ -132,7 +132,7 @@ public void TryDetachAt_RemovesAnArgument() Assert.IsTrue(AstSchema.TryDetachAt(call, Slot(call, "Arguments"), 0)); - Assert.AreEqual(1, call.Arguments.Count); + Assert.HasCount(1, call.Arguments); Assert.AreEqual("b", ((VariableReference)call.Arguments[0]).Name); } @@ -187,7 +187,7 @@ public void AFunctionBody_TakesAnExpressionStatement() Assert.IsTrue(AstSchema.TryAttach(function, body, new ExpressionStatement(new CallExpression("reset")))); - Assert.AreEqual(1, function.Body.Count); + Assert.HasCount(1, function.Body); } /// @@ -239,27 +239,46 @@ public void AnEmptyCallee_IsRefused() [TestMethod] public void ThePalette_OffersTheNewNodes() { - Assert.IsTrue(AstNodeCatalog.Templates.Any(template => template.Create() is CallExpression), "call"); - Assert.IsTrue(AstNodeCatalog.Templates.Any(template => template.Create() is ConditionalExpression), "conditional"); - Assert.IsTrue(AstNodeCatalog.Templates.Any(template => template.Create() is ExpressionStatement), "expression statement"); + Type[] offered = [.. AstNodeCatalog.Templates.Select(template => template.Create().GetType())]; + + Assert.Contains(typeof(CallExpression), offered); + Assert.Contains(typeof(ConditionalExpression), offered); + Assert.Contains(typeof(ExpressionStatement), offered); } /// /// Tests that what the palette creates is valid on its own, which is what lets a freshly created /// node be connected rather than filled in first. /// + /// + /// "Valid on its own" means every slot can be read and every operand already in one is the + /// placeholder produces, never a null the rest of the library + /// does not understand. A call's receiver is the one slot that starts empty rather than + /// placeholdered, because a call with no receiver is a free function rather than an unfinished + /// member call. + /// [TestMethod] public void WhatThePaletteCreates_IsReadyToConnect() { - foreach (AstNodeTemplate template in AstNodeCatalog.Templates - .Where(template => template.Create() is CallExpression or ConditionalExpression or ExpressionStatement)) + AstNodeTemplate[] templates = [.. AstNodeCatalog.Templates + .Where(template => template.Create() is CallExpression or ConditionalExpression or ExpressionStatement)]; + + // Asserted rather than assumed: a filter that matched nothing would make the loop below pass + // without checking anything, which is exactly what this test exists to rule out. + Assert.HasCount(3, templates); + + foreach (AstNodeTemplate template in templates) { AstNode node = template.Create(); foreach (AstSlot slot in AstSchema.SlotsOf(node)) { - // Reading a slot must not throw, whether or not anything is in it yet. - AstSchema.ChildrenOf(node, slot); + foreach (AstNode child in AstSchema.ChildrenOf(node, slot)) + { + Assert.IsTrue( + AstSchema.IsUnfilled(child), + $"{AstSchema.Describe(node)}'s {slot.Name} should start unfilled, not as {AstSchema.Describe(child)}"); + } } } } diff --git a/Coder/Serialization/YamlSerializer.cs b/Coder/Serialization/YamlSerializer.cs index 55dfca4..2b26267 100644 --- a/Coder/Serialization/YamlSerializer.cs +++ b/Coder/Serialization/YamlSerializer.cs @@ -320,6 +320,9 @@ private static void SerializeNamespaceDeclaration(NamespaceDeclaration namespace /// The key a node's single value is written under. private const string ValueKey = "value"; + /// The key an expression's expected type is written under. + private const string ExpectedTypeKey = "expectedType"; + /// The key a node's members are written under. private const string MembersKey = "members"; @@ -398,7 +401,7 @@ private static void SerializeCallExpression(CallExpression callExpr, Dictionary< if (callExpr.ExpectedType != null) { - nodeData["expectedType"] = callExpr.ExpectedType; + nodeData[ExpectedTypeKey] = callExpr.ExpectedType; } } @@ -418,7 +421,7 @@ private static void SerializeConditionalExpression(ConditionalExpression conditi if (conditional.ExpectedType != null) { - nodeData["expectedType"] = conditional.ExpectedType; + nodeData[ExpectedTypeKey] = conditional.ExpectedType; } } @@ -698,7 +701,7 @@ private static void SerializeBinaryExpression(BinaryExpression binaryExpr, Dicti if (binaryExpr.ExpectedType != null) { - nodeData["expectedType"] = binaryExpr.ExpectedType; + nodeData[ExpectedTypeKey] = binaryExpr.ExpectedType; } } @@ -712,7 +715,7 @@ private static void SerializeUnaryExpression(UnaryExpression unaryExpr, Dictiona if (unaryExpr.ExpectedType != null) { - nodeData["expectedType"] = unaryExpr.ExpectedType; + nodeData[ExpectedTypeKey] = unaryExpr.ExpectedType; } } @@ -725,7 +728,7 @@ private static void SerializeLiteralExpression(LiteralExpression literal, if (literal.ExpectedType != null) { - nodeData["expectedType"] = literal.ExpectedType; + nodeData[ExpectedTypeKey] = literal.ExpectedType; } } @@ -735,7 +738,7 @@ private static void SerializeVariableReference(VariableReference varRef, Diction if (varRef.ExpectedType != null) { - nodeData["expectedType"] = varRef.ExpectedType; + nodeData[ExpectedTypeKey] = varRef.ExpectedType; } }