Fix records to print with fieldnames

[hojberg]

Overview

Ensure records are printed with fieldnames.

This fixes #2260

Implementation notes

We were attempting to determine a type being a records, by looking
number of accessors and seeing if they matched a freshly generated
number of them. This was not working as we too eagerly removed type
prefixed names thus causing a mismatch.

When a suffixied PPE was used to determining if a type was a record it would often (not always) fail because it checked to see if the type name was a prefix of the accessor field name:

Intended: is List a prefix of List.map -> True
Failure case when map is a unique name in the codebase: is List a prefix of map -> False.

We fixed this by not using a suffixified PPE for the fieldname generation, lookup and comparion.

Test coverage

Added a transcript that covers records.

[hojberg]

Hmm seems to have an issue with a couple of transcripts.

[hojberg]

Seems like structural typing isn't including fields in the hash perhaps? Such that structural type X = One Nat Nat and structural type Y = {a: Nat, b: Nat} actually are the same type?

[pchiusano]

I'm not sure this fix is going to do it for all cases, since the PPE that's passed in is going to be returning suffixed names. The suffixed name could actually be set or modify if this type is the only record type in the namespace.

A better fix might be to pass in a separate PPE that's build from non-suffixed names, and then use that for the logic here. I think it will be more robust then. You can call Unison.PrettyPrintEnv.Names (fromNames) rather than fromSuffixedNames.

[runarorama]

I don't see it. The only things in this names map are the accessors generated within this function. The passed-in PPE is only used for looking up the names for those. I must be missing something.

[pchiusano]

The PPE that's passed in could report that the name for one of the record accessors is actually just modify (because just modify is a unique suffix in the namespace).

Another case I thought of is that one of the record accessors could have a name of base.v12.foo.bar.Blah.modify (because the namespace also has a base.v11.foo.bar.Blah type). I don't think this PR (or what's in trunk) will correctly handle this case.

[pchiusano]

I think this is okay as a temp improvement for some cases but I left a comment on how I think it could be done more robustly.

[pchiusano]

@hojberg yeah fields are not part of the type's hash - it's just a shorthand for producing some helper functions that operate on the type, not part of the type's identity.

[hojberg]

@pchiusano without thinking deeply about that, I think it would be nice if they were.

Regardless, for right now, this seems to have a bug where it tries to print a non-record type that structurally matches as a record:

Transcripts caught this:

structural type X = Two Nat Nat

Being printed as

structural type X = { #1cc12n45ii : Nat, #lomoj4v9oi : Nat }

[aryairani]

I could be wrong, but I think this is begging for a redesign of how field names are represented, that doesn't require synthesizing code and computing hashes for comparison against known names in the printer.

[pchiusano]

Kind of agree with @aryairani - I'd say the right thing would be to have proper record types. They could hash differently and we could represent the fields explicitly in the data decl itself. This is a bigger change though and I'd say out of scope for this PR.

I think as long as record declarations are just syntax sugar for writing a few helper functions, the basic approach being taken here is okay, the logic needs to be made more robust though.

[hojberg]

@runarorama do you have some time next week to work on this with me? I think I need some pointers on how to get these transcripts over the line.

[hojberg]

Paired with @pchiusano and @rlmark and this seems like it's in a good place now 🎉