Skip to content

Reading AbsoluteFilePath.AbsoluteDirectoryPath breaks the instance's equality and hash code #265

Description

@matt-edmondson

What happens

On ktsu.Semantics.Paths 5.4.2, reading AbsoluteFilePath.AbsoluteDirectoryPath mutates the instance it is read from. Afterwards it no longer compares equal to an identical instance, and its hash code has changed — while its text is untouched.

AbsoluteFilePath a = "/tmp/x/y.schema.json".As<AbsoluteFilePath>();
AbsoluteFilePath b = "/tmp/x/y.schema.json".As<AbsoluteFilePath>();

a == b;                  // True
a.Equals(b);             // True
a.GetHashCode() == b.GetHashCode();   // True

_ = a.AbsoluteDirectoryPath;          // a read, nothing else

a == b;                  // False   ❌
a.Equals(b);             // False   ❌
b.Equals(a);             // False   ❌
a.GetHashCode() == b.GetHashCode();   // False   ❌

(string)a == (string)b;  // still True — the value never changed

FileNameWithoutExtension does the same. FileName, FileExtension and FullFileExtension do not.

Root cause

AbsoluteFilePath is a record class, and it carries two lazily-populated memoization fields:

AbsoluteFilePath._cachedDirectoryPath             = <null>     → [AbsoluteDirectoryPath]
AbsoluteFilePath._cachedFileNameWithoutExtension  = <null>
SemanticString`1.<WeakString>k__BackingField      = /tmp/x/y.schema.json   (unchanged)

The compiler-generated Equals/GetHashCode for a record include every instance field, so the memoization fields are part of the equality contract. Populating a cache on first read therefore changes equality and the hash code. That also explains exactly which properties are affected: the two that have a _cached… field behind them, and no others.

Verified by reflecting over the private fields before and after the read (both instances shown above; only the touched one gains a non-null cache).

Why it matters

The value is immutable and its text never changes, so callers reasonably treat these as value types and use them as dictionary keys, in sets, and in assertions. All three break in ways that are very hard to attribute:

  • An AbsoluteFilePath used as a dictionary or HashSet key becomes unfindable once anything reads its directory — the hash bucket it was stored under no longer matches.
  • Equality silently depends on access history rather than value, which is the opposite of what a semantic string is for.
  • The failure is invisible in any message: Assert.AreEqual prints expected and actual as character-for-character identical strings and still fails.

How it surfaced

ktsu-dev/Schema#200 proposed replacing Path.GetDirectoryName(schemaFilePath) with schemaFilePath.AbsoluteDirectoryPath inside Schema.SetSourceFile. That looks like a pure readability win. But SchemaEditor.SaveToCurrentPath does:

CurrentSchema.SetSourceFile(CurrentSchemaPath);   // reads .AbsoluteDirectoryPath
Options.RecordRecentFile(CurrentSchemaPath);      // stores the same instance

so anchoring the schema corrupted equality for a path the schema does not own, and FileBrowserTests.SavingRecordsTheChosenPathAsRecentlyUsed began failing with identical-looking expected and actual values. Details and the full measurement are in ktsu-dev/Schema#208, where the workaround was to keep using Path.GetDirectoryName.

AsAbsolute(AbsoluteDirectoryPath) was measured alongside and is clean — it mutates neither its receiver nor its argument.

Suggested fix

Keep the memoization but take it out of the equality contract. A record's generated members can't be told to ignore a field, so one of:

  • Override Equals(TDerived?) and GetHashCode() on the path types so they defer to WeakString only. SemanticString<T> already defines equality that way; the derived records widen it by accident rather than by intent.
  • Move the caches out of instance fields (for example a ConditionalWeakTable keyed on the instance, or recompute — both properties are cheap string slicing).
  • Drop the caches entirely, if they were not measured to be worth it.

Whichever route, it would be worth a test that a value stays equal to an identical one, and keeps its hash code, after every derived property has been read — that would have caught both fields at once, and will catch the next one added.

Environment

  • ktsu.Semantics.Paths 5.4.2
  • .NET SDK 10.0.401, net10.0, Linux x64

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

No labels
No labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions