[Trosper3]

Summary

Sets that contain other sets could miss lookups, depending only on the order their elements were written in. {{1,2}}.Contains({2,1}) returned False, and {{1,2},{2,1}} kept both copies. This fixes that, plus a related equality bug, and adds a test project so these stay fixed.

Stacked on #31 (rbs report). Until #31 merges, this PR also shows its commits.

Merge order

#11, #13, #15 and #16 are stacked. Merge them into main in this order:

0. ci: run the Redux Build System (rbs) report, soft for now #31: rbs CI report and dotnet format (the new base)
1. fix(collections): hash sets independently of element order #11 (closes Equal sets hash differently, so sets of sets give wrong answers #9): set hashing; adds the test project the others use (this PR)
2. fix(grammar): accept set edges in unorderedcross again #13 (closes fix(grammar): unorderedcross rejects every edge since d7e982e #12): unorderedcross edges
3. fix(grammar): make is int, is set and is list check their type #15 (closes is int and is set constraints don't check their type #10): is int / is set / is list type checks
4. feat(parser): read instance strings in one pass, with clear errors #16 (closes Ignore whitespace that isn't part of a value #2, Clear parse errors #4, Parsing is slow on large input and crashes the process on deep nesting #7): one-pass instance reader and clear errors
5. feat(collections): add multisets #17 (closes Add multisets #1): multisets. Draft, and optional for the next release: no Redux definition uses it until Redux opts in.

After each merge, the next PR shows only its own commits. If one is squash-merged, I'll rebase the next one onto main before it's reviewed.

Releasing for Redux: publish a new version only after all four are merged. Redux's grammar fixes (ReduxISU/Redux#628, ReduxISU/Redux#629, ReduxISU/Redux#630) work with 0.1.2 and can land any time before the upgrade; only ReduxISU/Redux#630's test changes go in the same Redux PR that upgrades the SPADE package. Tracked in ReduxISU/Redux#627.

What changed

• UtilCollection.GetHashCode: sets now combine their elements' hash codes with an order-independent sum instead of hashing ToString(), which follows insertion order. Lists still hash their elements in order, and values hash their text.
• UtilCollection.Equals: now checks that both sides are the same kind (value, list or set) before comparing. Before, an empty set {} compared equal to an empty list () and to any plain value such as x. Equality and hashing have to agree, so this goes in the same change.
• New test project tests/SPADE.Tests (xUnit, .NET 10), with tests covering reordered sets, sets of sets and tuples, Contains, Count, Union and Intersect, and inequality across kinds. Before the fix, 12 of them fail.
• Collections freeze once hashed. A HashSet keeps each element under the hash it had when it went in, and UtilCollection is mutable. So changing a collection, or anything nested in it, after it was added to a set left it under a stale hash: Contains missed it and duplicates slipped in, with no error. Now:
  • The first GetHashCode() freezes the collection. Its elements freeze too, all the way down, because the hash is built from theirs.
  • After that, Add and the indexer setter throw InvalidOperationException, so the mistake is loud instead of silent.
  • The hash is computed once and cached.
  • Freeze() freezes a collection and everything in it on purpose, and IsFrozen() reports it. Frozen collections are safe to read from several threads; Documentation/threads.md explains what is safe to share.
  • The List/HashSet constructors copy their argument, and ToList() returns a copy, so nothing outside can change a frozen collection.
  • Building a collection fully and then adding it to a set works exactly as before. 17 tests cover this, and Documentation/utilcollection.md describes it.
• DiscreteParser.csproj excludes tests/ so the library doesn't compile the test files.
• Documentation/development.md explains how to run the tests.
• Adds the test project to SPADE.slnx and coverlet.collector, so the rbs report measures coverage.
• Tests target .NET 10; ci.yml and publish.yml install only the .NET 10 SDK. The library stays on net6.0.

Testing

• dotnet test tests/SPADE.Tests: 34 passed.
• Freeze guard against Redux: packed the top of the stack (feat(grammar): add a number type for decimal values #28) with and without the guard, and ran Redux's full suite (11,971 tests) against each. Both runs fail the same 16 tests, which are 0.2.0 behaviour changes such as error-text snapshots. No test fails only with the guard, so Redux never changes a collection after hashing it, at least on the paths its tests exercise.
• Ran Redux's full test suite (ReduxISU/Redux, 1,571 tests) with this change applied on top of the code in the published 0.1.2 package: 1,567 passed, 0 failed, 4 skipped, the same as with the package itself.

Worth knowing before the next release

The NuGet package 0.1.2 was published on 2025-05-01, so it doesn't include the three commits after 5745eb7. One of those, d7e982e, makes every unorderedcross membership check fail for normal edges like {a,b}: the new line checks whether each element of the edge is a list, rather than whether the edge itself is. Built from current main, with or without this PR, 236 of Redux's tests fail on that. That's tracked separately in #12, since it isn't related to hashing.

Closes #9

[norkish]

Looks great. I suggest we go with .NET 10 to keep consistent with other Redux repos. But let me know if you see a good reason to do otherwise.

[norkish]

Is there a reason you didn't go with .NET 10 since that's what the rest of Redux is using?

[norkish]

Could we make this net10.0?

Suggested change

	<TargetFramework>net8.0</TargetFramework>
	<TargetFramework>net10.0</TargetFramework>

[Trosper3]

Shoot! Great catch.

[norkish]

Honestly, it's pure laziness. I couldn't run the tests because I only had .NET 10 installed and asking you to change it was easier than installing .NET 8 :). But I did test it with .NET 10 and all the tests passed.

[Trosper3]

Added the freeze-on-hash guard (dbc816b): once a collection is hashed (for example, added to a set), it and everything inside it can't be changed, so a set can't end up holding an element under a stale hash. See "What changed" for details; Redux's full suite behaves identically with and without it. Merged up through the stack; #17 adds the same guard for multisets.

[Trosper3]

Made the cached hash thread-safe: frozen is now volatile, so a thread that sees a collection as frozen also sees its hash. A frozen collection can't change, so it's safe to share and read from several threads; building one still isn't, the same as List/HashSet. Documented in utilcollection.md, with a test that races threads to hash a fresh nested set. Merged up through the stack.

[Trosper3]

@norkish: following up on the mutability concern before you review. Here's what changed in this PR since you last looked, and I'd like to check it matches what you had in mind.

The problem. UtilCollection is mutable, and a HashSet files each element under the hash it had when it was added. If a set (or anything nested in it) was changed after being added, it sat under a stale hash: Contains and equality lookups missed it and duplicates slipped in, with no error.

What this PR does now:

• Freeze on first hash. The first GetHashCode() call freezes a collection. A collection's hash is built from its elements' hashes, so everything nested inside freezes too. After that, Add and the indexer setter throw InvalidOperationException ("…build it fully before adding it to a set"), so the mistake fails loudly instead of silently giving wrong answers.
• Freeze on purpose. Freeze() freezes a collection and everything inside it, and returns it. IsFrozen() reports it. This is for code that wants to lock a parsed instance before sharing it, even if it is never put in a set.
• No aliasing. The List<UtilCollection> / HashSet<UtilCollection> constructors copy their argument, and ToList() returns a copy. Code outside the collection can't change a frozen one through a shared reference.
• Cached hash. A frozen collection computes its hash once, which also speeds up lookups in large nested instances.
• Thread-safe reads. The cache uses a volatile flag, so frozen collections can be shared and read from several threads at once. Building a collection is still single-threaded, the same as List/HashSet. A new Documentation/threads.md says what is safe to share and what isn't (one StringParser per thread while parsing), with a parse-once, freeze, then share example. This matters for the high-performance computing work starting on Redux.
• Multisets (feat(collections): add multisets #17) freeze the same way.

What stays the same: building a collection fully and then adding it to a set works exactly as before. Collections that are never hashed or frozen can still be changed.

What changes in behaviour:

• Anything inside a set is frozen. For example, parsing {(1,2)} freezes the inner (1,2).
• ToList() on a list returns a copy instead of the internal list.

Testing:

• 17 new tests in this PR, plus 4 for multisets in feat(collections): add multisets #17. They cover nested freezing, dictionary keys, Freeze(), the copying constructors, and threads hashing the same collection at once.
• I ran Redux's full suite (11,971 tests) against the top of the stack with and without the freeze. Both runs show the same 16 failures, all expected 0.2.0 error-text changes. Nothing fails only with the freeze, so Redux never changes a collection after hashing it.

The longer-term fix is fully immutable collections: builders, and no Add. I'd do that in a later release, since it's a breaking change for Redux. Does this cover what you were after?

[Trosper3]

Filed the longer-term fix mentioned above as #42 (fully immutable collections: builders, no Add). It's a potential change for the 0.4.0 milestone, not part of this PR or 0.2.0.

[norkish]

This looks good and reading over the changes it seems sufficiently thorough to address the stale hashing issue. Nice work. Also worth highlighting here that rbs tests and formatting have been enforced in this PR, as well.

[norkish]

The PR grew to include essentially 5 main additions:

• Fixing a bug where identical sets that differed in order were not recognized as identical
• Fixing a bug (discovered during review process) to avoid stale hashes if collections changed (e.g., through added elements) by freezing collections
• Adding the testing suite
• Adding rbs
• Updating formatting of the entire repo to conform with required formatting

Despite there being potentially a scope crawl issue with bundling this all in one PR, this needs to move forward and I don't readily see any persisting issues.