refactor: Constraint::Plus stores an ExtensionSet, which is a BTreeSet

[acl-cqc]

• Convert ExtensionSet from HashSet to BTreeSe => trivially derive Hash
• Constraint::Plus stores an ExtensionSet, rather than a single Extension => gen_union_constraint avoids long chains of metas
• Inline gen_union_constraint (no longer recursive)

[codecov]

Codecov Report

Merging #636 (d26797f) into main (b3062e9) will decrease coverage by 0.01%.
The diff coverage is 92.30%.

@@            Coverage Diff             @@
##             main     #636      +/-   ##
==========================================
- Coverage   78.31%   78.31%   -0.01%     
==========================================
  Files          65       65              
  Lines       11537    11532       -5     
  Branches    11537    11532       -5     
==========================================
- Hits         9035     9031       -4     
+ Misses       1862     1861       -1     
  Partials      640      640              

Files	Coverage Δ
src/std_extensions/arithmetic/conversions.rs	75.36% <100.00%> (ø)
src/extension/infer.rs	86.20% <96.55%> (-0.07%)	⬇️
src/extension.rs	74.50% <75.00%> (ø)

... and 1 file with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

[aborgna-q]

the Hash implementation here deliberately goes against the instruction of how to use Hash

It doesn't. The requirement of Hash is that $k_1 = k_2 \implies \texttt{hash}(k_1) = \texttt{hash}(k_2)$.
The HashSet equality is order-invariant, so hashing should be too.

[aborgna-q]

Ah, I see what you mean. You are forced to introduce a hand-picked hasher.

You could sort the ExtensionIds instead, or just use a BTreeSet.

[acl-cqc]

Found it again:

This must not be used in any code which creates hashes, such as in an implementation of Hash. The way to create a combined hash of multiple values is to call Hash::hash multiple times using the same Hasher, not to call this method repeatedly and combine the results.

[acl-cqc]

Actually a BTreeSet might not be a bad way to go. IIUC this might even enable making some ExtensionSet methods const....I'll have a go

[acl-cqc]

Actually a BTreeSet might not be a bad way to go. IIUC this might even enable making some ExtensionSet methods const....I'll have a go

Ok, I thought I remembered a BTreeSet::singleton const method, but it is not so. However, converted to BTreeSet, that was painless, and Hash is ready-provided :-) :-)

[acl-cqc]

AFAICS this is reducing code coverage by shortening and simplifying the tested part of the codebase, therefore the untested part of the codebase (not changed by this PR) is now a greater proportion....?

[aborgna-q]

Yep, that's why the patch test passes, but project fails. Just ignore it.

The diff coverage is 92.30%.

[acl-cqc]

The alternative would be to store this with the empty ExtensionSet, and then we would only be storing one kind of Constraint (so, a struct, not an enum - might be syntactically easier to work with!). Then we'd be testing ExtensionSet::is_empty instead of matching on Constraint::Plus vs ::Equals...