From 6b2e02a8513881e841c8eec8d8064f599f20fed5 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 22 Sep 2026 02:29:41 +0000 Subject: [PATCH] Keep a path's equality and hash code stable when a derived property is read [patch] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The four concrete path records memoize their derived properties into private instance fields. A record's generated Equals and GetHashCode compare every instance field, so those caches were part of the equality contract: populating one on first read moved the instance out of its own hash bucket and stopped it comparing equal to an identical value, while its text stayed character-for-character the same. A path used as a dictionary or HashSet key therefore became unfindable as soon as anything read its directory, and the failure was near-impossible to attribute — Assert.AreEqual printed expected and actual as identical strings and still failed. The reported case was AbsoluteFilePath.AbsoluteDirectoryPath and FileNameWithoutExtension. It is wider than that: AbsoluteDirectoryPath caches Parent, Name, Depth and IsRoot, and RelativeDirectoryPath caches Parent, Name and Depth, so reading any of those broke the same way. All eleven cache fields across the four types are covered here. Each type now declares Equals and GetHashCode that defer to the base, which compares WeakString and the equality contract only. The caches stay; they are simply no longer part of identity. Sealing these members once on SemanticString is not available — CS8870 forbids sealing in a non-sealed record, and a base-level override does not stop a derived record synthesizing its own — so the invariant is pinned by a test instead. PathCacheEqualityTests covers both halves: the behaviour, per type and per cached property, plus dictionary and set lookup; and the structural rule, that any concrete path type declaring instance fields must declare its own equality members. The structural check reads CompilerGeneratedAttribute with inherit: false, since a record's synthesized members are declared members of the type and a hand-written GetHashCode would otherwise inherit the attribute from the base record's generated one. Nine of the ten tests fail without the fix; the tenth is a control asserting that differing paths stay unequal. Fixes #265 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01LAZsKzczZ5oCJcRH9HSnEa --- .../Implementations/AbsoluteDirectoryPath.cs | 18 +- .../Implementations/AbsoluteFilePath.cs | 18 +- .../Implementations/RelativeDirectoryPath.cs | 18 +- .../Implementations/RelativeFilePath.cs | 18 +- .../Paths/PathCacheEqualityTests.cs | 260 ++++++++++++++++++ 5 files changed, 328 insertions(+), 4 deletions(-) create mode 100644 Semantics.Test/Paths/PathCacheEqualityTests.cs diff --git a/Semantics.Paths/Implementations/AbsoluteDirectoryPath.cs b/Semantics.Paths/Implementations/AbsoluteDirectoryPath.cs index 809cea57..313c6c1e 100644 --- a/Semantics.Paths/Implementations/AbsoluteDirectoryPath.cs +++ b/Semantics.Paths/Implementations/AbsoluteDirectoryPath.cs @@ -10,7 +10,23 @@ namespace ktsu.Semantics.Paths; [IsAbsolutePath] public sealed record AbsoluteDirectoryPath : SemanticDirectoryPath, IAbsoluteDirectoryPath { - // Cache for expensive parent directory computation + /// + /// Determines whether this path and are the same path. + /// + /// The path to compare with, or . + /// if both are absolute directory paths with the same text; otherwise, . + /// + /// Declared explicitly, rather than left to the record's generated equality, because that would compare the + /// private memoization fields below as well — so reading a derived property would change both equality and the + /// hash code while leaving the text untouched, moving the instance out of its own hash bucket. A path's identity + /// is its text. See . + /// + public bool Equals(AbsoluteDirectoryPath? other) => base.Equals(other); + + /// + public override int GetHashCode() => base.GetHashCode(); + + // Cache for expensive parent directory computation. Excluded from equality by the members above. private AbsoluteDirectoryPath? _cachedParent; /// diff --git a/Semantics.Paths/Implementations/AbsoluteFilePath.cs b/Semantics.Paths/Implementations/AbsoluteFilePath.cs index 67e7e3c8..f0dd29bd 100644 --- a/Semantics.Paths/Implementations/AbsoluteFilePath.cs +++ b/Semantics.Paths/Implementations/AbsoluteFilePath.cs @@ -8,7 +8,23 @@ namespace ktsu.Semantics.Paths; [IsAbsolutePath] public sealed record AbsoluteFilePath : SemanticFilePath, IAbsoluteFilePath { - // Cache for expensive directory path computation + /// + /// Determines whether this path and are the same path. + /// + /// The path to compare with, or . + /// if both are absolute file paths with the same text; otherwise, . + /// + /// Declared explicitly, rather than left to the record's generated equality, because that would compare the + /// private memoization fields below as well — so reading a derived property would change both equality and the + /// hash code while leaving the text untouched, moving the instance out of its own hash bucket. A path's identity + /// is its text. See . + /// + public bool Equals(AbsoluteFilePath? other) => base.Equals(other); + + /// + public override int GetHashCode() => base.GetHashCode(); + + // Cache for expensive directory path computation. Excluded from equality by the members above. private AbsoluteDirectoryPath? _cachedDirectoryPath; /// diff --git a/Semantics.Paths/Implementations/RelativeDirectoryPath.cs b/Semantics.Paths/Implementations/RelativeDirectoryPath.cs index 76172fb6..664b2d2b 100644 --- a/Semantics.Paths/Implementations/RelativeDirectoryPath.cs +++ b/Semantics.Paths/Implementations/RelativeDirectoryPath.cs @@ -14,7 +14,23 @@ namespace ktsu.Semantics.Paths; [IsRelativePath] public sealed record RelativeDirectoryPath : SemanticDirectoryPath, IRelativeDirectoryPath { - // Cache for expensive parent directory computation + /// + /// Determines whether this path and are the same path. + /// + /// The path to compare with, or . + /// if both are relative directory paths with the same text; otherwise, . + /// + /// Declared explicitly, rather than left to the record's generated equality, because that would compare the + /// private memoization fields below as well — so reading a derived property would change both equality and the + /// hash code while leaving the text untouched, moving the instance out of its own hash bucket. A path's identity + /// is its text. See . + /// + public bool Equals(RelativeDirectoryPath? other) => base.Equals(other); + + /// + public override int GetHashCode() => base.GetHashCode(); + + // Cache for expensive parent directory computation. Excluded from equality by the members above. private RelativeDirectoryPath? _cachedParent; /// diff --git a/Semantics.Paths/Implementations/RelativeFilePath.cs b/Semantics.Paths/Implementations/RelativeFilePath.cs index a359705f..d972f8be 100644 --- a/Semantics.Paths/Implementations/RelativeFilePath.cs +++ b/Semantics.Paths/Implementations/RelativeFilePath.cs @@ -8,7 +8,23 @@ namespace ktsu.Semantics.Paths; [IsRelativePath] public sealed record RelativeFilePath : SemanticFilePath, IRelativeFilePath { - // Cache for expensive directory path computation + /// + /// Determines whether this path and are the same path. + /// + /// The path to compare with, or . + /// if both are relative file paths with the same text; otherwise, . + /// + /// Declared explicitly, rather than left to the record's generated equality, because that would compare the + /// private memoization fields below as well — so reading a derived property would change both equality and the + /// hash code while leaving the text untouched, moving the instance out of its own hash bucket. A path's identity + /// is its text. See . + /// + public bool Equals(RelativeFilePath? other) => base.Equals(other); + + /// + public override int GetHashCode() => base.GetHashCode(); + + // Cache for expensive directory path computation. Excluded from equality by the members above. private RelativeDirectoryPath? _cachedDirectoryPath; /// diff --git a/Semantics.Test/Paths/PathCacheEqualityTests.cs b/Semantics.Test/Paths/PathCacheEqualityTests.cs new file mode 100644 index 00000000..95165980 --- /dev/null +++ b/Semantics.Test/Paths/PathCacheEqualityTests.cs @@ -0,0 +1,260 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.Semantics.Test.Paths; + +using System; +using System.Collections.Generic; +using System.Linq; +using System.Reflection; +using System.Runtime.CompilerServices; +using ktsu.Semantics.Paths; +using ktsu.Semantics.Strings; +using Microsoft.VisualStudio.TestTools.UnitTesting; + +/// +/// A path's identity is its text. Reading a derived property must not change whether the instance +/// compares equal to an identical one, nor what it hashes to. +/// +/// +/// The path types memoize their derived properties in private instance fields. A record's generated +/// Equals and GetHashCode compare every instance field, so populating a cache on +/// first read used to move the instance out of its own hash bucket and stop it comparing equal to an +/// identical value — while its text stayed character-for-character the same. See +/// . +/// +[TestClass] +public class PathCacheEqualityTests +{ + /// + /// Asserts that reading leaves equal to + /// , in every direction and by hash code. + /// + /// The path type under test. + /// The instance the property is read from. + /// An instance built from the same text, never read from. + /// Reads the derived property under test. + /// The property being read, for the failure message. + private static void AssertStableAfterReading(T subject, T twin, Action read, string propertyName) + where T : SemanticString + { + Assert.AreEqual(twin, subject, $"{typeof(T).Name} was not equal to its twin before reading {propertyName}"); + + read(subject); + + Assert.AreEqual(twin, subject, $"reading {typeof(T).Name}.{propertyName} broke Equals(twin, subject)"); + Assert.AreEqual(subject, twin, $"reading {typeof(T).Name}.{propertyName} broke Equals(subject, twin)"); + Assert.AreEqual( + twin.GetHashCode(), + subject.GetHashCode(), + $"reading {typeof(T).Name}.{propertyName} changed the hash code"); + } + + [TestMethod] + public void ReadingAbsoluteDirectoryPathLeavesTheFilePathEqualAndHashable() + { + string text = TestPaths.Absolute("tmp", "x", "y.schema.json"); + AbsoluteFilePath subject = AbsoluteFilePath.Create(text); + AbsoluteFilePath twin = AbsoluteFilePath.Create(text); + + AssertStableAfterReading(subject, twin, path => _ = path.AbsoluteDirectoryPath, nameof(AbsoluteFilePath.AbsoluteDirectoryPath)); + } + + [TestMethod] + public void ReadingFileNameWithoutExtensionLeavesTheAbsoluteFilePathEqualAndHashable() + { + string text = TestPaths.Absolute("tmp", "x", "y.schema.json"); + AbsoluteFilePath subject = AbsoluteFilePath.Create(text); + AbsoluteFilePath twin = AbsoluteFilePath.Create(text); + + AssertStableAfterReading(subject, twin, path => _ = path.FileNameWithoutExtension, nameof(AbsoluteFilePath.FileNameWithoutExtension)); + } + + [TestMethod] + public void ReadingEveryDerivedPropertyLeavesTheAbsoluteFilePathEqualAndHashable() + { + string text = TestPaths.Absolute("tmp", "x", "y.schema.json"); + AbsoluteFilePath subject = AbsoluteFilePath.Create(text); + AbsoluteFilePath twin = AbsoluteFilePath.Create(text); + + AssertStableAfterReading( + subject, + twin, + path => + { + _ = path.AbsoluteDirectoryPath; + _ = path.FileNameWithoutExtension; + _ = path.FileName; + _ = path.FileExtension; + }, + "every derived property"); + } + + [TestMethod] + public void ReadingEveryDerivedPropertyLeavesTheRelativeFilePathEqualAndHashable() + { + string text = TestPaths.Relative("x", "y.schema.json"); + RelativeFilePath subject = RelativeFilePath.Create(text); + RelativeFilePath twin = RelativeFilePath.Create(text); + + AssertStableAfterReading( + subject, + twin, + path => + { + _ = path.RelativeDirectoryPath; + _ = path.FileNameWithoutExtension; + _ = path.FileName; + _ = path.FileExtension; + }, + "every derived property"); + } + + [TestMethod] + public void ReadingEveryDerivedPropertyLeavesTheAbsoluteDirectoryPathEqualAndHashable() + { + string text = TestPaths.Absolute("tmp", "x", "src"); + AbsoluteDirectoryPath subject = AbsoluteDirectoryPath.Create(text); + AbsoluteDirectoryPath twin = AbsoluteDirectoryPath.Create(text); + + AssertStableAfterReading( + subject, + twin, + path => + { + _ = path.Parent; + _ = path.Name; + _ = path.Depth; + _ = path.IsRoot; + }, + "every derived property"); + } + + [TestMethod] + public void ReadingEveryDerivedPropertyLeavesTheRelativeDirectoryPathEqualAndHashable() + { + string text = TestPaths.Relative("projects", "app", "src"); + RelativeDirectoryPath subject = RelativeDirectoryPath.Create(text); + RelativeDirectoryPath twin = RelativeDirectoryPath.Create(text); + + AssertStableAfterReading( + subject, + twin, + path => + { + _ = path.Parent; + _ = path.Name; + _ = path.Depth; + }, + "every derived property"); + } + + [TestMethod] + public void APathStaysFindableAsADictionaryKeyAfterItsDirectoryIsRead() + { + string text = TestPaths.Absolute("tmp", "x", "y.schema.json"); + AbsoluteFilePath key = AbsoluteFilePath.Create(text); + Dictionary recentFiles = new() { [key] = "recent" }; + + _ = key.AbsoluteDirectoryPath; + + Assert.IsTrue( + recentFiles.ContainsKey(key), + "the stored key could not be found in the dictionary it was stored in, after its directory was read"); + Assert.IsTrue( + recentFiles.ContainsKey(AbsoluteFilePath.Create(text)), + "an equal key could not be found in the dictionary after the stored key's directory was read"); + } + + [TestMethod] + public void APathStaysFindableInASetAfterItsDirectoryIsRead() + { + string text = TestPaths.Absolute("tmp", "x", "y.schema.json"); + AbsoluteFilePath member = AbsoluteFilePath.Create(text); + HashSet seen = [member]; + + _ = member.AbsoluteDirectoryPath; + + Assert.IsTrue(seen.Contains(member), "the set no longer contains the member it was built from"); + Assert.IsFalse( + seen.Add(AbsoluteFilePath.Create(text)), + "an equal path was added to the set a second time after the first one's directory was read"); + } + + [TestMethod] + public void DifferentPathsStayUnequal() + { + AbsoluteFilePath first = AbsoluteFilePath.Create(TestPaths.Absolute("tmp", "x", "y.schema.json")); + AbsoluteFilePath second = AbsoluteFilePath.Create(TestPaths.Absolute("tmp", "x", "z.schema.json")); + + _ = first.AbsoluteDirectoryPath; + _ = second.AbsoluteDirectoryPath; + + Assert.AreNotEqual(first, second); + } + + /// + /// Determines whether was written by hand rather than synthesized for the record. + /// + /// The candidate equality member, or if the type declares none. + /// if the method exists and is not compiler-generated. + /// + /// A record's generated Equals and GetHashCode are declared members of the type, so their presence + /// alone proves nothing — the distinction is . It must be read with + /// inherit: false: a hand-written GetHashCode overrides the base record's generated one and would + /// otherwise inherit its attribute, making every type look compiler-generated. + /// + private static bool IsHandWritten(MethodInfo? method) => + method is not null && !method.IsDefined(typeof(CompilerGeneratedAttribute), inherit: false); + + /// + /// The structural rule behind the behavioural tests above, so a path type added later cannot + /// reintroduce the defect by declaring a cache field and inheriting the generated equality. + /// + [TestMethod] + public void EveryPathTypeWithInstanceFieldsDeclaresItsOwnEqualityMembers() + { + IEnumerable pathTypes = typeof(AbsoluteFilePath).Assembly + .GetTypes() + .Where(type => type is { IsClass: true, IsAbstract: false, IsPublic: true }) + .Where(typeof(ISemanticString).IsAssignableFrom); + + List offenders = []; + + foreach (Type type in pathTypes) + { + FieldInfo[] instanceFields = type.GetFields(BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.DeclaredOnly); + if (instanceFields.Length == 0) + { + continue; + } + + bool declaresGetHashCode = IsHandWritten(type.GetMethod( + nameof(GetHashCode), + BindingFlags.Instance | BindingFlags.Public | BindingFlags.DeclaredOnly, + binder: null, + types: Type.EmptyTypes, + modifiers: null)); + + bool declaresEquals = IsHandWritten(type.GetMethod( + nameof(Equals), + BindingFlags.Instance | BindingFlags.Public | BindingFlags.DeclaredOnly, + binder: null, + types: [type], + modifiers: null)); + + if (!declaresGetHashCode || !declaresEquals) + { + string fields = string.Join(", ", instanceFields.Select(field => field.Name)); + offenders.Add($"{type.Name} declares instance field(s) {fields} but no {(declaresEquals ? "" : $"Equals({type.Name}?)")}{(declaresEquals || declaresGetHashCode ? "" : " and no ")}{(declaresGetHashCode ? "" : "GetHashCode()")}"); + } + } + + Assert.AreEqual( + 0, + offenders.Count, + "A record's generated equality includes every instance field, so a type that memoizes into one " + + "must declare Equals and GetHashCode that defer to WeakString only:" + + Environment.NewLine + + string.Join(Environment.NewLine, offenders)); + } +}