unison icon indicating copy to clipboard operation
unison copied to clipboard

Ignored empty assignment in `abilities.Each.repeat`

Open runarorama opened this issue 1 year ago • 4 comments

Original code added to codebase:

abilities.Each.repeat : Nat ->{Each} ()
abilities.Each.repeat n =
  _ = lazily (Stream.range 0 n)
  ()

Pretty-prints as:

abilities.Each.repeat : Nat ->{Each} ()
abilities.Each.repeat n =
  lazily (Stream.range 0 n)
  ()

which doesn't compile

runarorama avatar Feb 28 '24 18:02 runarorama

Strangely, if I print abilities.Each.repeat it fails as Rúnar says, but if I load the definition in, make it unique (by changing the start of the range for example) then re-add it and print that it works fine; so I'm guessing something has changed in the parser since this term was added to fix it 🤔 ;

The version in base (which fails) has the variable stored as User "_"-2750003 whereas new parses result in only User "_"; that appears to be the only change in the AST.

ChrisPenner avatar Jul 16 '24 17:07 ChrisPenner

Yeah, we used to accept code that didn't explicitly ignore a return value. In such ABTs, we'd generate a variable like User "_"-30174 which presumably started out life as User "_" before getting bumped tens of thousands of times during type checking.

We (obviously) should print the leading _ = because the variable is named "_" and the type of the ignored value isn't ().

mitchellwrosen avatar Jul 17 '24 00:07 mitchellwrosen

So, what is the bug to fix here?

  1. Should we be trying to identify this case and print it in a way that will a) parse and b) have the correct structure going forward?
  2. Should we just fix the definition in base and not do anything else?
  3. Is there possibly still a bug here when identifiers do get incremented somewhere, and we need to find a new way to replicate it?

sellout avatar Aug 20 '24 21:08 sellout

@sellout my thoughts would be:

Definitely worth it to just fix this definition in base for now (maybe save the old one on a branch somewhere for posterity?)

After that it'd probably be worth looking into spots where we're auto-generating a User var, preferably we wouldn't ever generate a user var since that's obviously a misnomer; but I'm not sure it's so easy to fix 🙃

ChrisPenner avatar Aug 23 '24 20:08 ChrisPenner