Make Type.Compare.Plugin use matching rather than splitTyConApp_maybe

isovector / type-sets

type level sets

BSD 3-Clause "New" or "Revised" License

67 stars 9 forks source link

Make Type.Compare.Plugin use matching rather than splitTyConApp_maybe #10

Closed mgsloan closed 4 years ago

mgsloan commented 5 years ago

This fixes #9. I also think this is an improvement in general:

Makes it clear the cases that aren't handled
If GHC's Type changes, we'll get errors / warnings
Probably faster
The string building version didn't have any indication of nesting. I didn't find a concrete case where this is a problem, but it seemed iffy

mgsloan commented 5 years ago

Also, it's worth noting that if non-deterministic (different each compile), non-lexicographic ordering is acceptable, then we could use nonDetCmpType straight from GHC: https://github.com/ghc/ghc/blob/3c9161621c6e467f53c5d4649a3c54b3cb40fbf9/compiler/types/Type.hs#L2158

Even though I just wrote this code, I'm leaning towards that approach.

mgsloan commented 5 years ago

Another issue here is that it panics when it encounters impredicative types. I'd like to synthesize a TypeError (Text ...) for this case, but looking up the identifiers is done in TcPluginM and MagicTyFam doesn't provide access to it.

mgsloan commented 5 years ago

I recall that work has been done to make GHC more deterministic, perhaps the name nonDet... is now a misnomer, worth poking around with it.

isovector commented 5 years ago

nonDet... is indeed non-deterministic; it compares based on uniques. But this is definitely not what we want --- it means in essence we don't have coherence, which means you could never serialize a structure that depends on CmpType and expect it to work on the other side.

isovector commented 5 years ago

Thanks for the PR! I'll take a close look at it sometime today!

isovector commented 5 years ago

This LGTM. Just to confirm, this will produce different hashes from the previous version, right?

mgsloan commented 5 years ago

This doesn't produce hashes at all, and instead structurally compares the types. I believe the comparison behavior is mostly the same as before, the test-suite did not need to be updated. The main difference that comes to mind is that function constructors are always LT data constructors, whereas before it was using the name of the function tycon.

isovector commented 4 years ago

Does this pass CI now that #12 is merged? I'm happy to merge it if so.

mgsloan commented 4 years ago

@isovector Looks like it does!

mgsloan commented 4 years ago

@isovector Ping!

isovector commented 4 years ago

Sorry for the delay on this --- I've been traveling for the last few days. I'm happy to merge this.

mgsloan commented 4 years ago

No problem, thanks for merging!

On Mon, Nov 18, 2019 at 7:55 PM Sandy Maguire notifications@github.com wrote:

Merged #10 https://github.com/isovector/type-sets/pull/10 into master.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/isovector/type-sets/pull/10?email_source=notifications&email_token=AAAFQKRQQAPRS5T2IRVFBFDQUNIQZA5CNFSM4JIJNY7KYY3PNVWWK3TUL52HS4DFWZEXG43VMVCXMZLOORHG65DJMZUWGYLUNFXW5KTDN5WW2ZLOORPWSZGOU55J2SQ#event-2809830730, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAFQKUIINS5ST6GGVPGUMTQUNIQZANCNFSM4JIJNY7A .