Refuse an expression body that writes no expression, before writing [patch]

CodeBlocker.Test/TemplateTests.cs

@@ -490,17 +490,44 @@ public void AnExpressionBodyStartingWithABlankLineKeepsItsFirstLineOnTheDeclarat
 	}
 
 	[TestMethod]
-	public void AnEmptyExpressionBodyStillTerminatesTheDeclaration()
+	public void AnEmptyExpressionBodyIsRejectedBeforeAnythingIsWritten()
 	{
+		// It used to render "public int X => ;", which does not compile, and raise nothing.
 		PropertyTemplate property = new()
 		{
 			Type = "int",
 			Name = "X",
 			Keywords = { "public" },
+			Documentation = new DocComment { Summary = "s" },
 			ExpressionBodyFactory = _ => { },
 		};
 
-		Assert.AreEqual("public int X => ;\n", Render(property));
+		using CodeBlocker codeBlocker = CodeBlocker.Create(CodeBlocker.DefaultIndentString, NewLines.Lf);
+		codeBlocker.WriteLine("int a;");
+
+		Assert.ThrowsExactly<InvalidOperationException>(() => property.WriteTo(codeBlocker));
+		Assert.AreEqual("int a;\n", codeBlocker.ToString());
+	}
+
+	[TestMethod]
+	public void AnExpressionBodyOfOnlyBlankLinesIsRejectedBeforeAnythingIsWritten()
+	{
+		PropertyTemplate property = new()
+		{
+			Type = "int",
+			Name = "X",
+			ExpressionBodyFactory = codeBlocker =>
+			{
+				codeBlocker.NewLine();
+				codeBlocker.WriteLine("  ");
+			},
+		};
+
+		using CodeBlocker codeBlocker = CodeBlocker.Create(CodeBlocker.DefaultIndentString, NewLines.Lf);
+		codeBlocker.WriteLine("int a;");
+
+		Assert.ThrowsExactly<InvalidOperationException>(() => property.WriteTo(codeBlocker));
+		Assert.AreEqual("int a;\n", codeBlocker.ToString());
 	}
 
 	private static string RenderExpressionBody(Action<CodeBlocker> expressionFactory) =>
@@ -733,6 +760,33 @@ public void AnExpressionAccessorWithNoBodyIsRejectedBeforeAnythingIsWritten()
 		Assert.AreEqual("int a;\n", codeBlocker.ToString());
 	}
 
+	[TestMethod]
+	public void AnExpressionAccessorThatWritesNoExpressionIsRejectedBeforeAnythingIsWritten()
+	{
+		PropertyTemplate property = new()
+		{
+			Type = "int",
+			Name = "P",
+			Getter = AccessorTemplate.Expression(_ => { }),
+		};
+
+		using CodeBlocker codeBlocker = CodeBlocker.Create(CodeBlocker.DefaultIndentString, NewLines.Lf);
+		codeBlocker.WriteLine("int a;");
+
+		Assert.ThrowsExactly<InvalidOperationException>(() => property.WriteTo(codeBlocker));
+		Assert.AreEqual("int a;\n", codeBlocker.ToString());
+	}
+
+	[TestMethod]
+	public void AStandaloneExpressionAccessorThatWritesNoExpressionWritesNothing()
+	{
+		using CodeBlocker codeBlocker = CodeBlocker.Create(CodeBlocker.DefaultIndentString, NewLines.Lf);
+		codeBlocker.WriteLine("int a;");
+
+		Assert.ThrowsExactly<InvalidOperationException>(() => AccessorTemplate.Expression(_ => { }).WriteTo(codeBlocker, "get"));
+		Assert.AreEqual("int a;\n", codeBlocker.ToString());
+	}
+
 	#endregion
 
 	#region MethodTemplate

CodeBlocker/Templates/AccessorTemplate.cs

@@ -79,13 +79,37 @@ public static AccessorTemplate Block(Action<CodeBlocker> body) =>
 	/// <exception cref="ArgumentNullException"><paramref name="codeBlocker"/> is <see langword="null"/>.</exception>
 	/// <exception cref="InvalidOperationException">
 	/// <see cref="Kind"/> is not a known kind, or is <see cref="AccessorKind.Expression"/> with no
-	/// <see cref="BodyFactory"/>. Nothing is written.
+	/// <see cref="BodyFactory"/> or with one that writes no expression. Nothing is written.
 	/// </exception>
 	public void WriteTo(CodeBlocker codeBlocker, string keyword)
 	{
 		Ensure.NotNull(codeBlocker);
 		Validate();
 
+		WriteTo(codeBlocker, keyword, RenderExpression(codeBlocker, keyword));
+	}
+
+	/// <summary>
+	/// Renders the expression of an <see cref="AccessorKind.Expression"/> accessor, so an empty one
+	/// is rejected before the property it belongs to writes anything.
+	/// </summary>
+	/// <param name="codeBlocker">The <see cref="CodeBlocker"/> the accessor will be written to.</param>
+	/// <param name="keyword">The accessor keyword, for the exception message.</param>
+	/// <returns>The expression's lines, or <see langword="null"/> for any other kind.</returns>
+	/// <exception cref="InvalidOperationException">The <see cref="BodyFactory"/> wrote no expression.</exception>
+	internal string[]? RenderExpression(CodeBlocker codeBlocker, string keyword) =>
+		Kind == AccessorKind.Expression
+			? TemplateRendering.RenderExpression(codeBlocker, BodyFactory, $"The '{keyword}' accessor")
+			: null;
+
+	/// <summary>
+	/// Writes the accessor, using an expression already rendered by <see cref="RenderExpression"/>.
+	/// </summary>
+	/// <param name="codeBlocker">The <see cref="CodeBlocker"/> to write to.</param>
+	/// <param name="keyword">The accessor keyword: <c>get</c>, <c>set</c> or <c>init</c>.</param>
+	/// <param name="expression">The rendered expression, for an <see cref="AccessorKind.Expression"/> accessor.</param>
+	internal void WriteTo(CodeBlocker codeBlocker, string keyword, string[]? expression)
+	{
 		string prefix = string.IsNullOrEmpty(Modifier) ? keyword : $"{Modifier} {keyword}";
 
 		switch (Kind)
@@ -96,7 +120,7 @@ public void WriteTo(CodeBlocker codeBlocker, string keyword)
 
 			case AccessorKind.Expression:
 				codeBlocker.Write(prefix);
-				TemplateRendering.WriteExpressionBody(codeBlocker, BodyFactory);
+				TemplateRendering.WriteExpressionBody(codeBlocker, expression ?? RenderExpression(codeBlocker, keyword)!);
 				break;
 
 			case AccessorKind.Block:
@@ -120,6 +144,10 @@ public void WriteTo(CodeBlocker codeBlocker, string keyword)
 	/// <see cref="Kind"/> is not a known kind, or is <see cref="AccessorKind.Expression"/> with no
 	/// <see cref="BodyFactory"/>, which would render as <c>get =&gt; ;</c>.
 	/// </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.
+	/// </remarks>
 	internal void Validate()
 	{
 		if (Kind is not (AccessorKind.Auto or AccessorKind.Expression or AccessorKind.Block))

CodeBlocker/Templates/PropertyTemplate.cs

@@ -28,7 +28,8 @@ public class PropertyTemplate : MemberTemplate
 	/// </summary>
 	/// <remarks>
 	/// An expression body replaces the accessor list, so <see cref="Getter"/> and
-	/// <see cref="Setter"/> are ignored when this is set.
+	/// <see cref="Setter"/> are ignored when this is set. A callback that writes no expression, or
+	/// only blank lines, is rejected: <c>=&gt; ;</c> does not compile.
 	/// </remarks>
 	public Action<CodeBlocker>? ExpressionBodyFactory { get; set; }
 
@@ -48,28 +49,39 @@ public class PropertyTemplate : MemberTemplate
 	/// <exception cref="InvalidOperationException">
 	/// <see cref="Getter"/>, <see cref="Setter"/> and <see cref="ExpressionBodyFactory"/> are all
 	/// <see langword="null"/>; or, with no expression body, an accessor's kind is unknown, or is
-	/// <see cref="AccessorKind.Expression"/> with no <see cref="AccessorTemplate.BodyFactory"/>.
+	/// <see cref="AccessorKind.Expression"/> with no <see cref="AccessorTemplate.BodyFactory"/>; or
+	/// the expression body, or an expression accessor's body, writes no expression. Nothing is
+	/// written in any of these cases.
 	/// </exception>
 	public override void WriteTo(CodeBlocker codeBlocker)
 	{
 		Ensure.NotNull(codeBlocker);
 
 		Validate();
-		base.WriteTo(codeBlocker);
 
 		if (ExpressionBodyFactory is not null)
 		{
-			TemplateRendering.WriteExpressionBody(codeBlocker, ExpressionBodyFactory);
+			// Rendered before the declaration is written: an empty expression is only found by
+			// running the callback, and "Type Name =>" would otherwise be left behind.
+			string[] expression = TemplateRendering.RenderExpression(codeBlocker, ExpressionBodyFactory, $"Property '{Name}'");
+			base.WriteTo(codeBlocker);
+			TemplateRendering.WriteExpressionBody(codeBlocker, expression);
 			return;
 		}
 
+		// Likewise rendered up front, for an expression accessor's body.
+		string[]? getterExpression = Getter?.RenderExpression(codeBlocker, "get");
+		string[]? setterExpression = Setter?.RenderExpression(codeBlocker, SetterKeyword);
+
+		base.WriteTo(codeBlocker);
+
 		if (CanUseShorthand)
 		{
 			WriteShorthand(codeBlocker);
 			return;
 		}
 
-		WriteAccessorList(codeBlocker);
+		WriteAccessorList(codeBlocker, getterExpression, setterExpression);
 	}
 
 	/// <inheritdoc/>
@@ -128,14 +140,14 @@ private void WriteShorthand(CodeBlocker codeBlocker)
 		codeBlocker.WriteLine(HasDefaultValue ? ";" : string.Empty);
 	}
 
-	private void WriteAccessorList(CodeBlocker codeBlocker)
+	private void WriteAccessorList(CodeBlocker codeBlocker, string[]? getterExpression, string[]? setterExpression)
 	{
 		codeBlocker.WriteLine();
 		codeBlocker.WriteLine("{");
 		codeBlocker.Indent();
 
-		Getter?.WriteTo(codeBlocker, "get");
-		Setter?.WriteTo(codeBlocker, SetterKeyword);
+		Getter?.WriteTo(codeBlocker, "get", getterExpression);
+		Setter?.WriteTo(codeBlocker, SetterKeyword, setterExpression);
 
 		codeBlocker.Outdent();
 

CodeBlocker/Templates/TemplateRendering.cs

@@ -322,13 +322,38 @@ internal static void WriteBody(CodeBlocker codeBlocker, Action<CodeBlocker>? bod
 	}
 
 	/// <summary>
-	/// Writes an expression body — the arrow, the expression and its terminating semicolon — and
-	/// ends the line.
+	/// Renders an expression body's expression into its lines, so it can be checked before anything
+	/// of the declaration it belongs to is written.
 	/// </summary>
-	/// <param name="codeBlocker">The <see cref="CodeBlocker"/> to write to.</param>
+	/// <param name="codeBlocker">The <see cref="CodeBlocker"/> the expression will be written to.</param>
 	/// <param name="expressionFactory">
 	/// The callback that writes the expression, without <c>=&gt;</c> or a semicolon.
 	/// </param>
+	/// <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.
+	/// </exception>
+	internal static string[] RenderExpression(CodeBlocker codeBlocker, Action<CodeBlocker>? expressionFactory, string owner)
+	{
+		string[] lines = SplitLines(codeBlocker, RenderFragment(codeBlocker, expressionFactory));
+		if (lines.All(string.IsNullOrWhiteSpace))
+		{
+			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.");
+		}
+
+		return lines;
+	}
+
+	/// <summary>
+	/// Writes an expression body — the arrow, the expression and its terminating semicolon — and
+	/// ends the line.
+	/// </summary>
+	/// <param name="codeBlocker">The <see cref="CodeBlocker"/> to write to.</param>
+	/// <param name="lines">The expression, as <see cref="RenderExpression"/> rendered it.</param>
 	/// <remarks>
 	/// The first line of the expression stays on the declaration line, after the arrow, and every
 	/// following line is spliced at the current indent, so a multi-line switch expression, collection
@@ -337,10 +362,8 @@ internal static void WriteBody(CodeBlocker codeBlocker, Action<CodeBlocker>? bod
 	/// of its own. A line comment there keeps the semicolon in front of it, and a line holding only a
 	/// comment is not code, so the semicolon never ends up inside a comment.
 	/// </remarks>
-	internal static void WriteExpressionBody(CodeBlocker codeBlocker, Action<CodeBlocker>? expressionFactory)
+	internal static void WriteExpressionBody(CodeBlocker codeBlocker, string[] lines)
 	{
-		string[] lines = SplitLines(codeBlocker, RenderFragment(codeBlocker, expressionFactory));
-
 		int last = lines.Length - 1;
 		while (last >= 0 && lines[last].Length == 0)
 		{
@@ -355,12 +378,6 @@ internal static void WriteExpressionBody(CodeBlocker codeBlocker, Action<CodeBlo
 			first++;
 		}
 
-		if (last < 0)
-		{
-			codeBlocker.WriteLine(" => ;");
-			return;
-		}
-
 		// A directive must be the first thing on its line (CS1040), so one that opens the
 		// expression goes on the line after the arrow rather than on the declaration line.
 		if (IsDirective(lines[first]))