`update` issue with sum type, adding a new type constructor

[kylegoetz]

I have a sum type ResourceType = Light | Device | ...... and a lot of types and terms referencing it.

When I update this by adding a new constructor to the end (... | Event) and update, I get a very long list of things added to my scratch file to update for compilation. Some of these get hashes instead of names in the scratch file.

For example,

api.SmartScene.PostBody.toJson = cases
  PostBody#ckkft7ui#0
    (Name.Name n)
    oimage
    oappdata
    (ResourceTuple id type')
    slots
    oduration
    oaction -> ...

@aryairani asked me to add this to the issue tracker.

One should be able to reproduce this error by

1. pulling @kylegoetz/huenison/streamtest from Share (once I've resolved why push errors out with @kylegoetz/huenison does not exist on https://api.unison-lang.org)
2. edit ResourceType
3. add | Event2
4. update

The first thing that shows up in the scratch file should be

unique type api.BehaviorInstance.GetBody
  = GetBody#lbung6rm#0
      ResourceId
      ResourceId
      Boolean
      (Optional Json)
      Json
      [(Text, ResourceId, ResourceType)]
      BehaviorInstanceStatus
      Text
      Name
      (Optional ResourceIdV1)

with the constructor hash.

[aryairani]

@mitchellwrosen Did #4500 fix this issue for upgrade, but we still need the same fix for update?
I was able to reproduce by following the steps listed above.

[aryairani]

the resulting scratch file is here: https://gist.github.com/aryairani/4fc55aa7baca67ac8eebf3b03d987095

[mitchellwrosen]

@aryairani I can look into it - the issue for upgrade was in code unique to upgrade, but update apparently has something similar going on

[mitchellwrosen]

We've made some progress understanding this one.

It seems the bug is somewhere around prettyUnisonFile, which was originally written for structured find-and-replace, and reused in both update and upgrade

The function accepts a PPE, but only as a fallback, and uses the unison file itself (via UF.toNames) as the primary source of names.

However, UF.toNames seems to expect that the unison file's decls' names are structured like this:

"Maybe" =>
  "Maybe.Nothing" => ...
  "Maybe.Just" => ...

which is to say, the constructor names are provided in their entirety. UF.toNames just takes the names of constructors directly and makes a Names object out of them.

However, the unison file passed to prettyUnisonFile (at least in the update path, possibly others) has decls that look like this:

"Maybe" =>
  "Nothing" => ...
  "Just" => ...

that is, with the constructors' full names (e.g. "Maybe.Just") implicitly defined as the concatenation of the decl name and the constructor name, with a dot in between.

[mitchellwrosen]

For example, say prettyUnisonFile is used to pretty-print a file that looks like:

unique type Foo = Hey Nat
unique type Bar = Hey Nat

I suppose there are arguably two possible in-memory representations A and B of the names here, though I think only B is currently valid, as we don't have a requirement that constructors' names all share a prefix with the decl (though we really ought to have such a requirement, because constructors that aren't named such can't be rendered...):

A

"Foo" => ["Hey"]
"Bar" => ["Hey"]

B

"Foo" => ["Foo.Hey"]
"Bar" => ["Bar.Hey"]

If a unison file structured like A is passed into prettyUnisonFile, then the constructors will be considered in conflict (as they share the name "Hey") and will be printed hash-qualified, which is wrong.

[mitchellwrosen]

I think a likely explanation is that unison files like A are malformed. I found where we're stripping decl name prefixes:

unison/unison-cli/src/Unison/Codebase/Editor/HandleInput/Update2.hs

Line 365 in 966d8e4

case traverse (fmap Name.toVar . Name.stripNamePrefix name) array of

@aryairani do you recall why?

[aryairani]

I think a likely explanation is that unison files like A are malformed. I found where we're stripping decl name prefixes:

unison/unison-cli/src/Unison/Codebase/Editor/HandleInput/Update2.hs

Line 365 in 966d8e4

case traverse (fmap Name.toVar . Name.stripNamePrefix name) array of

@aryairani do you recall why?

Could just have been an oversight, thinking that A was correct; or maybe A is correct?

If we have

unique type Foo = X

I'm assuming we want the namespace to have type Foo and constructor Foo.X.

If we have

unique type Foo.Bar = X

I'm assuming we want the namespace to have type Foo.Bar and constructor Foo.Bar.X (not Foo.Bar.Foo.Bar.X).

I was thinking that calling the constructor X was correct, but now with the discrepancy I'm not sure.

[mitchellwrosen]

If A is correct, then UF.toNames is the buggy bit.

But A doesn't seem correct because:

I think only B is currently valid, as we don't have a requirement that constructors' names all share a prefix with the decl

[mitchellwrosen]

Yeah, it does seem that the parser parses the constructor name of unique type Foo = X as "Foo.X" (B), not "X" (A).

[aryairani]

I guess it's up to us to choose the convention, we just have to make every place match. And if there are some spots we don't want to touch (e.g. the existing parser; not saying that's the case) then that constrains us.

Yeah, it does seem that the parser parses the constructor name of unique type Foo = X as "Foo.X" (B), not "X" (A).

Ah. What does it parse for unique type Foo.Bar = X?

[mitchellwrosen]

What does it parse for unique type Foo.Bar = X?

"Foo.Bar.X"