Write a header's definitions so it can be included from more than one translation unit [patch]

CLAUDE.md

@@ -63,7 +63,12 @@ source in seven target languages. The solution uses:
   `VariableDeclaration`: a field with no initialiser is value-initialised, so a default-constructed
   instance is the one the declaration described, and a local with none is ordinary. `SourceFile`'s
   `Imports` are the one part of the AST that does not translate — a C++ include path, a C# namespace
-  and a Python module are different kinds of thing — so a file is built for a language.
+  and a Python module are different kinds of thing — so a file is built for a language. A file
+  marked `IsHeader` is included by every translation unit that uses it, so what it defines is
+  written to be defined in each of them: C++ writes free functions and namespace-scope variables
+  `inline`, and C writes functions (lowered methods included) `static inline` and mutable variables
+  `static`, one copy per translation unit, since C has no inline variable
+  (`CFamilyGenerator.WritingHeader`).
 - `Coder/Ast/FunctionKind.cs`, `FunctionDefinition.cs` — what a function declares (method,
   constructor, destructor, operator, conversion) and where its behaviour comes from (provided,
   defaulted, deleted). A declaration with no statements is otherwise ambiguous between a function

Coder.Test/Languages/HeaderLinkageTests.cs

@@ -0,0 +1,234 @@
+// 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;
+
+/// <summary>
+/// Tests that a generated C or C++ header can be included by more than one translation unit of a
+/// program.
+/// </summary>
+/// <remarks>
+/// <c>#pragma once</c> keeps a header to once per translation unit and no further, so a free
+/// function, a function a method is lowered to, or a mutable variable defined in it with external
+/// linkage is defined once in each unit, and the program fails to link with "multiple definition"
+/// (issue #423). C++ now writes them <c>inline</c>; C writes a function <c>static inline</c> and a
+/// variable <c>static</c>, one copy per unit, since C has no inline variable.
+/// </remarks>
+[TestClass]
+public class HeaderLinkageTests
+{
+	private static readonly string[] CCompilers = ["cc", "gcc", "clang"];
+
+	private static readonly string[] CppCompilers = ["c++", "g++", "clang++"];
+
+	/// <summary>
+	/// Tests that C writes a header's functions <c>static inline</c> and its mutable variables
+	/// <c>static</c>.
+	/// </summary>
+	[TestMethod]
+	public void C_HeaderDefinitions_HaveInternalLinkage()
+	{
+		string header = new CGenerator().Generate(Header());
+
+		StringAssert.Contains(header, "static int limit = 5;", StringComparison.Ordinal);
+		StringAssert.Contains(header, "static inline int twice(int x)", StringComparison.Ordinal);
+		StringAssert.Contains(header, "static inline int P_get(P* self)", StringComparison.Ordinal);
+		StringAssert.Contains(header, "static inline int P_make(void)", StringComparison.Ordinal);
+		StringAssert.Contains(header, "static int scale = 8;", StringComparison.Ordinal);
+		StringAssert.Contains(header, "static inline int half(int x)", StringComparison.Ordinal);
+	}
+
+	/// <summary>
+	/// Tests that C++ writes a header's free functions and namespace-scope variables <c>inline</c>,
+	/// and leaves a class's members as they were.
+	/// </summary>
+	[TestMethod]
+	public void Cpp_HeaderDefinitions_AreInline()
+	{
+		string header = new CppGenerator().Generate(Header());
+
+		StringAssert.Contains(header, "inline int limit = 5;", StringComparison.Ordinal);
+		StringAssert.Contains(header, "inline int twice(int x)", StringComparison.Ordinal);
+		StringAssert.Contains(header, "inline int scale = 8;", StringComparison.Ordinal);
+		StringAssert.Contains(header, "inline int half(int x)", StringComparison.Ordinal);
+		StringAssert.Contains(header, "    int get()", StringComparison.Ordinal);
+		StringAssert.Contains(header, "    static int make()", StringComparison.Ordinal);
+	}
+
+	/// <summary>
+	/// Tests that a file that is not a header is written as it always was.
+	/// </summary>
+	[TestMethod]
+	public void ASourceFileThatIsNotAHeader_IsUnchanged()
+	{
+		SourceFile file = Header();
+		file.IsHeader = false;
+
+		string c = new CGenerator().Generate(file);
+		string cpp = new CppGenerator().Generate(file);
+
+		Assert.DoesNotContain("inline", c, StringComparison.Ordinal, c);
+		Assert.DoesNotContain("static int limit", c, StringComparison.Ordinal, c);
+		Assert.DoesNotContain("static int scale", c, StringComparison.Ordinal, c);
+		StringAssert.Contains(c, "int twice(int x)", StringComparison.Ordinal);
+		StringAssert.Contains(cpp, "int limit = 5;", StringComparison.Ordinal);
+		Assert.DoesNotContain("inline int", cpp, StringComparison.Ordinal, cpp);
+	}
+
+	/// <summary>
+	/// Tests that a C header included from two translation units compiles, links and runs.
+	/// </summary>
+	[TestMethod]
+	public void C_HeaderIncludedTwice_Links()
+	{
+		string? compiler = ToolchainHarness.FindOnPath("--version", CCompilers);
+		if (compiler is null)
+		{
+			Assert.Inconclusive("No C compiler on the path, so nothing was compiled.");
+			return;
+		}
+
+		const string first = """
+			#include <stdio.h>
+			#include "lib.h"
+
+			int other(void);
+
+			int main(void)
+			{
+				P p = { 1 };
+				printf("%d %d %d %d %d %d\n", limit, twice(3), P_get(&p), P_make(), half(scale), other());
+				return 0;
+			}
+
+			""";
+
+		const string second = """
+			#include "lib.h"
+
+			int other(void)
+			{
+				P p = { 2 };
+				return twice(limit) + P_get(&p) + P_make() + half(scale) + P_count;
+			}
+
+			""";
+
+		LinkTwoUnits(compiler, new CGenerator().Generate(Header()), "lib.h", ("a.c", first), ("b.c", second), "-std=c11 -Wall -Wextra -pedantic-errors");
+	}
+
+	/// <summary>
+	/// Tests that a C++ header included from two translation units compiles, links and runs.
+	/// </summary>
+	[TestMethod]
+	public void Cpp_HeaderIncludedTwice_Links()
+	{
+		string? compiler = ToolchainHarness.FindOnPath("--version", CppCompilers);
+		if (compiler is null)
+		{
+			Assert.Inconclusive("No C++ compiler on the path, so nothing was compiled.");
+			return;
+		}
+
+		const string first = """
+			#include <cstdio>
+			#include "lib.h"
+
+			int other();
+
+			int main()
+			{
+				P p{};
+				std::printf("%d %d %d %d %d %d\n", limit, twice(3), p.get(), P::make(), geo::half(geo::scale), other());
+				return 0;
+			}
+
+			""";
+
+		const string second = """
+			#include "lib.h"
+
+			int other()
+			{
+				P p{};
+				return twice(limit) + p.get() + P::make() + geo::half(geo::scale) + P::count;
+			}
+
+			""";
+
+		LinkTwoUnits(compiler, new CppGenerator().Generate(Header()), "lib.h", ("a.cpp", first), ("b.cpp", second), "-std=c++17 -Wall -Wextra -pedantic-errors");
+	}
+
+	/// <summary>
+	/// The issue's reproduction, widened to every kind of definition a header can hold: a mutable
+	/// variable and a function at file scope, a struct with an instance method, a static method and a
+	/// static field, and a namespace holding a variable and a function.
+	/// </summary>
+	/// <returns>The header.</returns>
+	private static SourceFile Header()
+	{
+		SourceFile file = new("lib") { IsHeader = true };
+		file.Members.Add(new FieldDeclaration("limit", "int") { InitialValue = new LiteralExpression<int>(5) });
+		file.Members.Add(Function("twice", BinaryOperator.Multiply));
+
+		ClassDeclaration point = new("P") { Kind = TypeDeclarationKind.Struct };
+		point.Members.Add(new FieldDeclaration("v", "int") { Visibility = Visibility.Public });
+		point.Members.Add(new FieldDeclaration("count", "int") { IsStatic = true, Visibility = Visibility.Public, InitialValue = new LiteralExpression<int>(0) });
+		FunctionDeclaration get = new("get") { ReturnType = "int", Visibility = Visibility.Public };
+		get.Body.Add(new ReturnStatement(new LiteralExpression<int>(3)));
+		point.Members.Add(get);
+		FunctionDeclaration make = new("make") { ReturnType = "int", Visibility = Visibility.Public, IsStatic = true };
+		make.Body.Add(new ReturnStatement(new LiteralExpression<int>(4)));
+		point.Members.Add(make);
+		file.Members.Add(point);
+
+		NamespaceDeclaration geo = new("geo");
+		geo.Members.Add(new FieldDeclaration("scale", "int") { InitialValue = new LiteralExpression<int>(8) });
+		geo.Members.Add(Function("half", BinaryOperator.Divide));
+		file.Members.Add(geo);
+		return file;
+	}
+
+	/// <summary>
+	/// A function of one <c>int</c> that applies an operator to it and two.
+	/// </summary>
+	/// <param name="name">The function's name.</param>
+	/// <param name="op">The operator.</param>
+	/// <returns>The function.</returns>
+	private static FunctionDeclaration Function(string name, BinaryOperator op)
+	{
+		FunctionDeclaration function = new(name) { ReturnType = "int" };
+		function.Parameters.Add(new Parameter("x", "int"));
+		function.Body.Add(new ReturnStatement(new BinaryExpression(new VariableReference("x"), op, new LiteralExpression<int>(2))));
+		return function;
+	}
+
+	/// <summary>
+	/// Compiles two translation units that both include a header, links them, and runs the result.
+	/// </summary>
+	/// <param name="compiler">The compiler driver.</param>
+	/// <param name="header">The generated header.</param>
+	/// <param name="headerName">The name the units include it by.</param>
+	/// <param name="first">The unit holding <c>main</c>.</param>
+	/// <param name="second">The other unit.</param>
+	/// <param name="flags">The dialect and warning flags.</param>
+	private static void LinkTwoUnits(string compiler, string header, string headerName, (string Name, string Source) first, (string Name, string Source) second, string flags)
+	{
+		ToolchainHarness.InTemporaryDirectory(directory =>
+		{
+			File.WriteAllText(Path.Combine(directory, headerName), header);
+			File.WriteAllText(Path.Combine(directory, first.Name), first.Source);
+			File.WriteAllText(Path.Combine(directory, second.Name), second.Source);
+
+			(int exitCode, string output) = ToolchainHarness.Run(compiler, $"{flags} {first.Name} {second.Name} -o linked", directory);
+			Assert.AreEqual(0, exitCode, $"{compiler} could not build the two units:{Environment.NewLine}{header}{Environment.NewLine}{output}");
+
+			(int ran, string printed) = ToolchainHarness.Run(Path.Combine(directory, "linked"), string.Empty, directory);
+			Assert.AreEqual(0, ran, printed);
+			Assert.AreEqual("5 6 3 4 4 21", printed.Trim(), header);
+		});
+	}
+}

Coder/Languages/CFamilyGenerator.cs

@@ -66,6 +66,18 @@ protected sealed override void GenerateFunctionDeclaration(FunctionDeclaration f
 	/// </remarks>
 	protected abstract string SpellType(TypeReference type);
 
+	/// <summary>
+	/// Gets a value indicating whether the file being generated is a header.
+	/// </summary>
+	/// <remarks>
+	/// A header is included by every translation unit that uses it, and <c>#pragma once</c> keeps it
+	/// to once per unit and no further: a definition with external linkage in it is defined once in
+	/// each unit and the program does not link. So what a header defines is written so that a
+	/// definition in every unit is allowed, as <c>inline</c> in C++ and as <c>static</c> or
+	/// <c>static inline</c> in C, which has no inline variable (#423).
+	/// </remarks>
+	protected bool WritingHeader => GenerationRoot is SourceFile { IsHeader: true };
+
 	/// <summary>
 	/// Gets the file extension (without the dot) used for a header in this language.
 	/// </summary>

Coder/Languages/CGenerator.cs

@@ -177,7 +177,7 @@
 	/// The <c>&lt;math.h&gt;</c> macros, which is why a file holding one includes it.
 	/// </remarks>
 	protected override string SpellNonFiniteDouble(double value) =>
 		double.IsNaN(value) ? "NAN" : value > 0 ? "INFINITY" : "-INFINITY";
 
 	/// <inheritdoc/>
 	protected override IEnumerable<string> RequiredImports(SourceFile file)
@@ -254,9 +254,20 @@
 		// C has no generics, so what a function is written over is written down, as for a type.
 		WriteTypeParametersDown(funcDecl.TypeParameters, code);
 
-		// Internal linkage is the only privacy C has, and it is what a private function wants: the
-		// declaration is visible to this translation unit and to nothing else.
-		if (funcDecl.Visibility == Visibility.Private)
+		// A declaration with no definition is a prototype, which is all C has to offer for either of
+		// them: an abstract declaration is one an implementation must supply, and a defaulted one is
+		// one the language would have supplied had it been C++.
+		bool isPrototype = funcDecl.IsAbstract || funcDecl.Definition == FunctionDefinition.Defaulted;
+
+		// A definition in a header is made in every translation unit that includes it, and only
+		// `static inline` lets each of them have one without the program defining it twice (#423).
+		// Otherwise internal linkage is the only privacy C has, and it is what a private function
+		// wants: the declaration is visible to this translation unit and to nothing else.
+		if (WritingHeader && !isPrototype)
+		{
+			code.Write("static inline ");
+		}
+		else if (funcDecl.Visibility == Visibility.Private)
 		{
 			code.Write("static ");
 		}
@@ -267,10 +278,7 @@
 		code.Write(SpellFunctionName(funcDecl, enclosingType));
 		WriteParameterList(ParametersOf(funcDecl, enclosingType), code);
 
-		// A declaration with no definition is a prototype, which is all C has to offer for either of
-		// them: an abstract declaration is one an implementation must supply, and a defaulted one is
-		// one the language would have supplied had it been C++.
-		if (funcDecl.IsAbstract || funcDecl.Definition == FunctionDefinition.Defaulted)
+		if (isPrototype)
 		{
 			code.WriteLine(";");
 			return;
@@ -595,7 +603,7 @@
 	/// outright; the placeholder also gives a constructor's <c>{0}</c> something to zero.
 	/// </para>
 	/// </remarks>
 	protected override void GenerateClassDeclaration(ClassDeclaration classDecl, CodeBlocker code)
 	{
 		Ensure.NotNull(classDecl);
 		Ensure.NotNull(code);
@@ -1150,7 +1158,10 @@
 
 		TypeReference type = field.Type ?? new TypeReference(UnknownTypeName);
 
-		if (field.IsConstant || field.IsStatic || field.Visibility == Visibility.Private)
+		// C has no inline variable, so a mutable one in a header is static too: every translation
+		// unit that includes the header has its own copy, which is the one way the header can define
+		// it without the program defining it twice (#423).
+		if (field.IsConstant || field.IsStatic || field.Visibility == Visibility.Private || WritingHeader)
 		{
 			code.Write("static ");
 		}

Coder/Languages/CppGenerator.cs

@@ -169,7 +169,7 @@
 	/// </remarks>
 	protected override string SpellNonFiniteDouble(double value) =>
 		double.IsNaN(value) ? "std::numeric_limits<double>::quiet_NaN()"
 		: value > 0 ? "std::numeric_limits<double>::infinity()"
 		: "-std::numeric_limits<double>::infinity()";
 
 	/// <inheritdoc/>
@@ -179,7 +179,7 @@
 	/// </remarks>
 	protected override string SpellNonFiniteSingle(float value) =>
 		float.IsNaN(value) ? "std::numeric_limits<float>::quiet_NaN()"
 		: value > 0 ? "std::numeric_limits<float>::infinity()"
 		: "-std::numeric_limits<float>::infinity()";
 
 	/// <inheritdoc/>
@@ -263,7 +263,7 @@
 	/// elided or duplicated, which is a stronger promise than the AST is in a position to make.
 	/// </para>
 	/// </remarks>
 	protected override void GenerateFunction(FunctionDeclaration funcDecl, CodeBlocker code, string? enclosingType)
 	{
 		Ensure.NotNull(funcDecl);
 		Ensure.NotNull(code);
@@ -298,6 +298,11 @@
 			code.Write("static ");
 		}
 
+		if (NeedsInline(funcDecl, enclosingType))
+		{
+			code.Write("inline ");
+		}
+
 		// An abstract declaration is virtual whether or not it was asked to be: there is nothing else
 		// `= 0` could mean.
 		if (funcDecl.IsVirtual || funcDecl.IsAbstract)
@@ -366,6 +371,24 @@
 		}
 	}
 
+	/// <summary>
+	/// Reports whether a function has to say <c>inline</c> to be defined in a header.
+	/// </summary>
+	/// <param name="funcDecl">The declaration being emitted.</param>
+	/// <param name="enclosingType">The type it is a member of, or null for a free function.</param>
+	/// <returns>True for a free function defined in a header that is not inline already.</returns>
+	/// <remarks>
+	/// A function defined in a class is inline already, and so are a friend defined there and a
+	/// <c>constexpr</c> one; anything else defined in a header is defined again in every translation
+	/// unit that includes it, and the program does not link (#423).
+	/// </remarks>
+	private bool NeedsInline(FunctionDeclaration funcDecl, string? enclosingType) =>
+		WritingHeader
+		&& enclosingType is null
+		&& !funcDecl.IsFriend
+		&& !funcDecl.IsCompileTimeEvaluable
+		&& !funcDecl.IsAbstract;
+
 	/// <summary>
 	/// Writes what the type's members start at, between a constructor's signature and its body.
 	/// </summary>
@@ -478,7 +501,7 @@
 	/// describing a scope.
 	/// </para>
 	/// </remarks>
 	protected override void GenerateClassDeclaration(ClassDeclaration classDecl, CodeBlocker code)
 	{
 		Ensure.NotNull(classDecl);
 		Ensure.NotNull(code);
@@ -1038,7 +1061,10 @@
 			return insideType > 0 ? "inline static " : "static ";
 		}
 
-		return string.Empty;
+		// A mutable variable at namespace scope has external linkage, so a header defining one
+		// defines it again in every translation unit that includes it. C++17's inline variable is
+		// one object however many units define it (#423).
+		return insideType == 0 && WritingHeader ? "inline " : string.Empty;
 	}
 
 	/// <summary>