Check an expression body's expression before writing its class, and refuse a comment-only one [patch]

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<InvalidOperationException>(() => 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<InvalidOperationException>(() => property.WriteTo(codeBlocker));
+		Assert.ThrowsExactly<InvalidOperationException>(() => 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<InvalidOperationException>(() => 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<object[]> 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<InvalidOperationException>(() => 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<InvalidOperationException>(() => codeBlocker.AddSourceFile(file));
+		Assert.AreEqual(string.Empty, codeBlocker.ToString());
+	}
+
 	[TestMethod]
 	public void AnInvalidMemberOfANestedClassLeavesTheOuterClassUnwritten()
 	{

CodeBlocker/Templates/AccessorTemplate.cs

@@ -146,7 +146,8 @@ internal void WriteTo(CodeBlocker codeBlocker, string keyword, string[]? express
 	/// </exception>
 	/// <remarks>
 	/// This never runs <see cref="BodyFactory"/>. A factory that runs and writes no expression is
-	/// caught by <see cref="RenderExpression"/>, which the writers call before writing anything.
+	/// caught by <see cref="RenderExpression"/>, which the writers, and a property's own validation,
+	/// call before writing anything.
 	/// </remarks>
 	internal void Validate()
 	{

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)
 	}
 
 	/// <inheritdoc/>
-	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);
 		}
 	}
 

CodeBlocker/Templates/OperatorTemplate.cs

@@ -80,7 +80,7 @@ public override void WriteTo(CodeBlocker codeBlocker)
 	}
 
 	/// <inheritdoc/>
-	internal override void Validate() => _ = Signature;
+	internal override void Validate(CodeBlocker codeBlocker) => _ = Signature;
 
 	private string Signature => Kind switch
 	{

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)
 	}
 
 	/// <inheritdoc/>
-	internal override void Validate()
+	/// <remarks>
+	/// 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.
+	/// </remarks>
+	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);
+	}
+
+	/// <summary>
+	/// 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.
+	/// </summary>
+	private void ValidateDeclaration()
 	{
 		if (ExpressionBodyFactory is null && Getter is null && Setter is null)
 		{

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<C
 		Ensure.NotNull(classes);
 
 		List<ClassTemplate> classList = [.. classes];
-		ValidateAll(classList);
+		ValidateAll(codeBlocker, classList);
 
 		foreach (ClassTemplate classTemplate in classList)
 		{
@@ -112,12 +112,12 @@ public static CodeBlocker AddClasses(this CodeBlocker codeBlocker, IEnumerable<C
 		return codeBlocker;
 	}
 
-	private static void ValidateAll(IEnumerable<ClassTemplate> classes)
+	private static void ValidateAll(CodeBlocker codeBlocker, IEnumerable<ClassTemplate> classes)
 	{
 		foreach (ClassTemplate classTemplate in classes)
 		{
 			Ensure.NotNull(classTemplate);
-			classTemplate.Validate();
+			classTemplate.Validate(codeBlocker);
 		}
 	}
 }

CodeBlocker/Templates/TemplateBase.cs

@@ -88,11 +88,16 @@ public virtual void WriteTo(CodeBlocker codeBlocker)
 	/// <summary>
 	/// Throws if <see cref="WriteTo"/> would refuse this template, without writing anything.
 	/// </summary>
+	/// <param name="codeBlocker">
+	/// The <see cref="CodeBlocker"/> 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.
+	/// </param>
 	/// <remarks>
 	/// 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.
 	/// </remarks>
-	internal virtual void Validate()
+	internal virtual void Validate(CodeBlocker codeBlocker)
 	{
 	}
 

CodeBlocker/Templates/TemplateRendering.cs

@@ -332,17 +332,21 @@ internal static void WriteBody(CodeBlocker codeBlocker, Action<CodeBlocker>? bod
 	/// <param name="owner">Names the declaration in the exception message, for example <c>Property 'P'</c>.</param>
 	/// <returns>The expression's lines, for <see cref="WriteExpressionBody"/>.</returns>
 	/// <exception cref="InvalidOperationException">
-	/// The callback wrote nothing but blank lines. An expression body has no valid empty form: it
-	/// would render as <c>=&gt; ;</c>, 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 <c>=&gt; ;</c>, which does not
+	/// compile.
 	/// </exception>
 	internal static string[] RenderExpression(CodeBlocker codeBlocker, Action<CodeBlocker>? 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;