From 00b600438e7c1179b98702aaf71bae7e5cfa4fb5 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 5 Nov 2026 10:27:34 +0000 Subject: [PATCH] Check an expression body's expression before writing its class, and refuse a comment-only one [patch] A property whose expression body, or expression accessor, wrote no expression was only refused when the property itself was written, inside its class's body. The class had already been written up to that member, and the scope's dispose closed it, leaving a well-formed class that silently lacked the property and every member after it (#183). Validate now takes the CodeBlocker it is checking for, and a property's validation renders its expressions, so ClassTemplate, AddClasses and AddSourceFile refuse it before writing anything. The empty-expression check also only looked for blank lines, so a body that wrote only a line comment or only preprocessor directives rendered as "=> ;" (CS1525) (#184). It now requires a line with code, through the existing HasCode helper. Fixes ktsu-dev/CodeBlocker#183 Fixes ktsu-dev/CodeBlocker#184 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017vfJW1zLzNveHEn69h7Vn3 --- CodeBlocker.Test/TemplateTests.cs | 101 ++++++++++++++++++++ CodeBlocker/Templates/AccessorTemplate.cs | 3 +- CodeBlocker/Templates/ClassTemplate.cs | 8 +- CodeBlocker/Templates/OperatorTemplate.cs | 2 +- CodeBlocker/Templates/PropertyTemplate.cs | 28 +++++- CodeBlocker/Templates/SourceFileTemplate.cs | 8 +- CodeBlocker/Templates/TemplateBase.cs | 7 +- CodeBlocker/Templates/TemplateRendering.cs | 14 ++- 8 files changed, 153 insertions(+), 18 deletions(-) diff --git a/CodeBlocker.Test/TemplateTests.cs b/CodeBlocker.Test/TemplateTests.cs index 3fd8237..35f284a 100644 --- a/CodeBlocker.Test/TemplateTests.cs +++ b/CodeBlocker.Test/TemplateTests.cs @@ -787,6 +787,62 @@ public void AStandaloneExpressionAccessorThatWritesNoExpressionWritesNothing() Assert.AreEqual("int a;\n", codeBlocker.ToString()); } + private static void WritePlaceholderComment(CodeBlocker codeBlocker) => codeBlocker.WriteLine("// placeholder"); + + private static void WriteEmptyDebugConditional(CodeBlocker codeBlocker) + { + codeBlocker.WriteLine("#if DEBUG"); + codeBlocker.WriteLine("#endif"); + } + + [TestMethod] + public void AnExpressionBodyOfOnlyACommentIsRejectedBeforeAnythingIsWritten() + { + // Used to render "int P => ;// placeholder" (CS1525). + using CodeBlocker codeBlocker = CodeBlocker.Create(CodeBlocker.DefaultIndentString, NewLines.Lf); + codeBlocker.WriteLine("int a;"); + + PropertyTemplate property = new() { Type = "int", Name = "P", ExpressionBodyFactory = WritePlaceholderComment }; + + Assert.ThrowsExactly(() => property.WriteTo(codeBlocker)); + Assert.AreEqual("int a;\n", codeBlocker.ToString()); + } + + [TestMethod] + public void AnExpressionAccessorOfOnlyACommentIsRejectedBeforeAnythingIsWritten() + { + // Used to render "get => ;// placeholder" (CS1525). + using CodeBlocker codeBlocker = CodeBlocker.Create(CodeBlocker.DefaultIndentString, NewLines.Lf); + codeBlocker.WriteLine("int a;"); + + PropertyTemplate property = new() { Type = "int", Name = "P", Getter = AccessorTemplate.Expression(WritePlaceholderComment) }; + + Assert.ThrowsExactly(() => property.WriteTo(codeBlocker)); + Assert.ThrowsExactly(() => AccessorTemplate.Expression(WritePlaceholderComment).WriteTo(codeBlocker, "get")); + Assert.AreEqual("int a;\n", codeBlocker.ToString()); + } + + [TestMethod] + public void AnExpressionBodyOfOnlyDirectivesIsRejectedBeforeAnythingIsWritten() + { + // Used to render "int P =>", then the directives, then a lone ";" (CS1525). + using CodeBlocker codeBlocker = CodeBlocker.Create(CodeBlocker.DefaultIndentString, NewLines.Lf); + + PropertyTemplate property = new() { Type = "int", Name = "P", ExpressionBodyFactory = WriteEmptyDebugConditional }; + + Assert.ThrowsExactly(() => property.WriteTo(codeBlocker)); + Assert.AreEqual(string.Empty, codeBlocker.ToString()); + } + + [TestMethod] + public void AnExpressionBodyWithCodeAroundItsCommentsAndDirectivesIsStillWritten() + { + Assert.AreEqual("public int X => x; // c\n", RenderExpressionBody(codeBlocker => codeBlocker.Write("x // c"))); + Assert.AreEqual( + "int P =>\n#if DEBUG\n1\n#else\n2\n#endif\n;\n", + Render(new PropertyTemplate { Type = "int", Name = "P", ExpressionBodyFactory = WriteDebugConditional })); + } + #endregion #region MethodTemplate @@ -1200,6 +1256,51 @@ public void AClassWithAnInvalidMemberWritesNothing() Assert.AreEqual(string.Empty, codeBlocker.ToString()); } + public static IEnumerable PropertiesWhoseExpressionWritesNothing => + [ + ["empty expression body", new PropertyTemplate { Type = "int", Name = "P", ExpressionBodyFactory = _ => { } }], + ["empty expression accessor", new PropertyTemplate { Type = "int", Name = "P", Getter = AccessorTemplate.Expression(_ => { }) }], + ["comment-only expression body", new PropertyTemplate { Type = "int", Name = "P", ExpressionBodyFactory = WritePlaceholderComment }], + ["comment-only expression accessor", new PropertyTemplate { Type = "int", Name = "P", Getter = AccessorTemplate.Expression(WritePlaceholderComment) }], + ]; + + [TestMethod] + [DynamicData(nameof(PropertiesWhoseExpressionWritesNothing))] + public void AClassWithAPropertyWhoseExpressionWritesNothingWritesNothing(string description, PropertyTemplate property) + { + // The empty expression used to be found only when the property was written, inside the + // class's body, so the class was closed without it and every member after it (#183). + ClassTemplate type = new() + { + Name = "C", + Members = { new FieldTemplate { Type = "int", Name = "f" }, property, new FieldTemplate { Type = "int", Name = "g" } }, + }; + + using CodeBlocker codeBlocker = CodeBlocker.Create(CodeBlocker.DefaultIndentString, NewLines.Lf); + + Assert.ThrowsExactly(() => codeBlocker.AddClass(type), description); + Assert.AreEqual(string.Empty, codeBlocker.ToString(), description); + } + + [TestMethod] + public void ASourceFileWithAnEmptyExpressionBodyWritesNothing() + { + SourceFileTemplate file = new() + { + Namespace = "Contoso", + Classes = + { + new ClassTemplate { Name = "Good" }, + new ClassTemplate { Name = "Bad", Members = { new PropertyTemplate { Type = "int", Name = "P", ExpressionBodyFactory = _ => { } } } }, + }, + }; + + using CodeBlocker codeBlocker = CodeBlocker.Create(CodeBlocker.DefaultIndentString, NewLines.Lf); + + Assert.ThrowsExactly(() => codeBlocker.AddSourceFile(file)); + Assert.AreEqual(string.Empty, codeBlocker.ToString()); + } + [TestMethod] public void AnInvalidMemberOfANestedClassLeavesTheOuterClassUnwritten() { diff --git a/CodeBlocker/Templates/AccessorTemplate.cs b/CodeBlocker/Templates/AccessorTemplate.cs index 553308b..bc9d143 100644 --- a/CodeBlocker/Templates/AccessorTemplate.cs +++ b/CodeBlocker/Templates/AccessorTemplate.cs @@ -146,7 +146,8 @@ internal void WriteTo(CodeBlocker codeBlocker, string keyword, string[]? express /// /// /// This never runs . A factory that runs and writes no expression is - /// caught by , which the writers call before writing anything. + /// caught by , which the writers, and a property's own validation, + /// call before writing anything. /// internal void Validate() { diff --git a/CodeBlocker/Templates/ClassTemplate.cs b/CodeBlocker/Templates/ClassTemplate.cs index 9304811..a8f670a 100644 --- a/CodeBlocker/Templates/ClassTemplate.cs +++ b/CodeBlocker/Templates/ClassTemplate.cs @@ -73,7 +73,7 @@ public override void WriteTo(CodeBlocker codeBlocker) // Everything is checked before the doc comment, attributes and modifiers are written, so a // failing check cannot leave an unfinished declaration behind, nor a closed body that is // silently missing the failing member and everything after it. - Validate(); + Validate(codeBlocker); string keyword = TypeKindKeywords.For(Kind); base.WriteTo(codeBlocker); @@ -102,18 +102,18 @@ public override void WriteTo(CodeBlocker codeBlocker) } /// - internal override void Validate() + internal override void Validate(CodeBlocker codeBlocker) { _ = TypeKindKeywords.For(Kind); foreach (MemberTemplate member in Members) { - member.Validate(); + member.Validate(codeBlocker); } foreach (ClassTemplate nestedClass in NestedClasses) { - nestedClass.Validate(); + nestedClass.Validate(codeBlocker); } } diff --git a/CodeBlocker/Templates/OperatorTemplate.cs b/CodeBlocker/Templates/OperatorTemplate.cs index 4c237b3..1aeccc2 100644 --- a/CodeBlocker/Templates/OperatorTemplate.cs +++ b/CodeBlocker/Templates/OperatorTemplate.cs @@ -80,7 +80,7 @@ public override void WriteTo(CodeBlocker codeBlocker) } /// - internal override void Validate() => _ = Signature; + internal override void Validate(CodeBlocker codeBlocker) => _ = Signature; private string Signature => Kind switch { diff --git a/CodeBlocker/Templates/PropertyTemplate.cs b/CodeBlocker/Templates/PropertyTemplate.cs index 471ec0a..5bc45c3 100644 --- a/CodeBlocker/Templates/PropertyTemplate.cs +++ b/CodeBlocker/Templates/PropertyTemplate.cs @@ -57,7 +57,7 @@ public override void WriteTo(CodeBlocker codeBlocker) { Ensure.NotNull(codeBlocker); - Validate(); + ValidateDeclaration(); if (ExpressionBodyFactory is not null) { @@ -85,7 +85,31 @@ public override void WriteTo(CodeBlocker codeBlocker) } /// - internal override void Validate() + /// + /// The expression body, or an expression accessor's body, is rendered to check that it writes an + /// expression, so a class holding this property refuses it before writing its own header rather + /// than after the members ahead of it. That runs the callback once more when the property is + /// then written. + /// + internal override void Validate(CodeBlocker codeBlocker) + { + ValidateDeclaration(); + + if (ExpressionBodyFactory is not null) + { + _ = TemplateRendering.RenderExpression(codeBlocker, ExpressionBodyFactory, $"Property '{Name}'"); + return; + } + + _ = Getter?.RenderExpression(codeBlocker, "get"); + _ = Setter?.RenderExpression(codeBlocker, SetterKeyword); + } + + /// + /// Throws when the property cannot be written whatever its callbacks write: when it has no + /// accessors and no expression body, or an accessor that cannot be written. + /// + private void ValidateDeclaration() { if (ExpressionBodyFactory is null && Getter is null && Setter is null) { diff --git a/CodeBlocker/Templates/SourceFileTemplate.cs b/CodeBlocker/Templates/SourceFileTemplate.cs index 0ed8015..db6f904 100644 --- a/CodeBlocker/Templates/SourceFileTemplate.cs +++ b/CodeBlocker/Templates/SourceFileTemplate.cs @@ -59,7 +59,7 @@ public static CodeBlocker AddSourceFile(this CodeBlocker codeBlocker, SourceFile // Checked before the header is written, so a type that would be refused leaves nothing // behind rather than a file holding only the types before it. - ValidateAll(template.Classes); + ValidateAll(codeBlocker, template.Classes); codeBlocker.AddDocumentation(template.Documentation); codeBlocker.AddComments(template.Comments); @@ -101,7 +101,7 @@ public static CodeBlocker AddClasses(this CodeBlocker codeBlocker, IEnumerable classList = [.. classes]; - ValidateAll(classList); + ValidateAll(codeBlocker, classList); foreach (ClassTemplate classTemplate in classList) { @@ -112,12 +112,12 @@ public static CodeBlocker AddClasses(this CodeBlocker codeBlocker, IEnumerable classes) + private static void ValidateAll(CodeBlocker codeBlocker, IEnumerable classes) { foreach (ClassTemplate classTemplate in classes) { Ensure.NotNull(classTemplate); - classTemplate.Validate(); + classTemplate.Validate(codeBlocker); } } } diff --git a/CodeBlocker/Templates/TemplateBase.cs b/CodeBlocker/Templates/TemplateBase.cs index 1abb91b..41915f8 100644 --- a/CodeBlocker/Templates/TemplateBase.cs +++ b/CodeBlocker/Templates/TemplateBase.cs @@ -88,11 +88,16 @@ public virtual void WriteTo(CodeBlocker codeBlocker) /// /// Throws if would refuse this template, without writing anything. /// + /// + /// The the template will be written to. Nothing is written to it: it + /// supplies the indent and line terminator a callback is rendered with, for a check that can + /// only be made by running the callback. + /// /// /// A container calls this on everything it holds before writing any of it, so a template that /// fails its check cannot leave a container written without it, and the ones after it. /// - internal virtual void Validate() + internal virtual void Validate(CodeBlocker codeBlocker) { } diff --git a/CodeBlocker/Templates/TemplateRendering.cs b/CodeBlocker/Templates/TemplateRendering.cs index f993ece..ee2b7c9 100644 --- a/CodeBlocker/Templates/TemplateRendering.cs +++ b/CodeBlocker/Templates/TemplateRendering.cs @@ -332,17 +332,21 @@ internal static void WriteBody(CodeBlocker codeBlocker, Action? bod /// Names the declaration in the exception message, for example Property 'P'. /// The expression's lines, for . /// - /// The callback wrote nothing but blank lines. An expression body has no valid empty form: it - /// would render as => ;, which does not compile. + /// The callback wrote nothing but blank lines, line comments and preprocessor directives. An + /// expression body has no valid empty form: it would render as => ;, which does not + /// compile. /// internal static string[] RenderExpression(CodeBlocker codeBlocker, Action? expressionFactory, string owner) { string[] lines = SplitLines(codeBlocker, RenderFragment(codeBlocker, expressionFactory)); - if (lines.All(string.IsNullOrWhiteSpace)) + // A comment or directive is no more an expression than a blank line is: a placeholder + // comment would otherwise take the semicolon in front of it and render as "=> ;// ...". + if (!HasCode(lines)) { throw new InvalidOperationException( - $"{owner} has an expression body whose callback wrote no expression, which would render as " + - "\"=> ;\". Write an expression, or use a block body or an automatic accessor instead."); + $"{owner} has an expression body whose callback wrote no expression, only blank lines, " + + "comments or directives, which would render as \"=> ;\". Write an expression, or use a " + + "block body or an automatic accessor instead."); } return lines;