All names are printed with their unique values by default.

[Unisay]

• A test demonstrating that without a unique suffix a name doesn't survive printing/parsing.
• Update printer and parser to support unique suffixes, e.g. foo#42.

Closes #4808

[Unisay]

This example demonstrates that without unique suffixes variables f and z were shadowed multiple times.

[Unisay]

Which a and b are these? Without unique suffixes its not clear.

[effectfully]

I'd prefer to use ` instead of # and replace our current usage of ` with @, I think that would make dumped UPLC much more readable, but maybe it's just me.

We need proper escaping of delimiters, but I don't think it should block this PR, I think it's best to keep this one minimal (it's only 714 files changed after all) and implement escaping separately.

[effectfully]

Hm, there's an issue here, we can have names that contain arbitrary symbols when the name is wrapped in `. I think we have to implement escaping, unless you've already done that (I just started reviewing the PR). This can be especially troubling given that # can be a part of a GHC Core and even surface Haskell name.

[effectfully]

That syntax works with fourmolu for you?

[Unisay]

Yes, this is controlled by the function-arrows option in fourmolu:

[effectfully]

Yeah, this looks wrong to me, although I'm not entirely sure. # is a legitimate part of a name, so we have to have escaping in order for #-a-part-of-a-name not to get confused with #-the-delimiter.

Although looking at

isQuotedIdentifierChar :: Char -> Bool
isQuotedIdentifierChar c =
  (isAlpha c || isDigit c || isPunctuation c || isSymbol c)
    && isAscii c
    && c /= '`'

seems like we already have an escaping bug, except I suppose GHC isn't going to produce ` as a part of a name unlike #.

I happen to have some code somewhere for escaping, I'll dig it out.

[effectfully]

I think it's worth keeping this part of the Note and everything below it, except we should remove NameWithType and TyNameWithKind. All the reasoning about us avoiding a quadratic number of instances by insisting that each pretty-printing config must have a PrettyConfigName entry still applies.

[effectfully]

I feel like # is too noisy, I think ` would be perfect here, compare this

r#3 -> (a#0 -> b#1 -> interlist#2 b#1 a#0 -> r#3) -> r#3)

and this

r`3 -> (a`0 -> b`1 -> interlist`2 b`1 a`0 -> r`3) -> r`3)

Now we of course already use ` for quoted identifiers, but we could use something else, I think @ might work, given that it's basic Haddock syntax and GHC uses the symbol for type applications which we render differently, so we won't clash with GHC here (still need escaping for non-GHC-produced code, but that's a given).

[Unisay]

Indeed, ` is lighter which could be more aesthetically pleasing, depending on your taste. I wouldn't mind us using `, however these reasons make me think its not the best option:

1. More often than not back-ticks are used in pairs; When singular back-ticks happen to match they can hurt readability. Consider expected`0 risk`100 -- it reads as if 0 risk is spliced inside expected100
2. As you've mentioned: we already use back-ticks for escaping.

Between ` and @ I chose the latter.

Lets also consider these characters:

ghci> import PlutusCore.Name.Unique
ghci> isIdentifierChar '~'
False
ghci> isIdentifierChar '!'
False
ghci> isIdentifierChar '-'
False
ghci> isIdentifierChar '^'
False
ghci> isIdentifierChar '|'
False

[Unisay]

Demo:

r-3 -> (a-0 -> b-1 -> interlist-2 b-1 a-0 -> r-3) -> r-3)

r^3 -> (a^0 -> b^1 -> interlist^2 b^1 a^0 -> r^3) -> r^3)

r~3 -> (a~0 -> b~1 -> interlist~2 b~1 a~0 -> r~3) -> r~3)

r!3 -> (a!0 -> b!1 -> interlist!2 b!1 a!0 -> r!3) -> r!3)

r|3 -> (a|0 -> b|1 -> interlist|2 b|1 a|0 -> r|3) -> r|3)

r@3 -> (a@0 -> b@1 -> interlist@2 b@1 a@0 -> r@3) -> r@3)

[Unisay]

Personally, I like ! most of all above.
Next best option is - IMO.

[effectfully]

More often than not back-ticks are used in pairs; When singular back-ticks happen to match they can hurt readability. Consider expected`0 risk`100 -- it reads as if 0 risk is spliced inside expected100

Yeah, good point.

I just took a long look at the demo. I'm OK with any of those, but - seems very nice as well as ! indeed (except I'd perhaps prefer the former over the latter).

[Unisay]

👍🏼 I went with - for Unique indexes and ! for De Bruijn (instead of # for all)

[effectfully]

I made a strategic decision not to review this test, sorry.

[Unisay]

I wish I could have made a strategic decision not to have to modify that test.
(The problem with it is that is quite fragile in the face of changes in a way we render names.)

[effectfully]

Thank you a ton for this PR, you're doing God's work.

[effectfully]

LGTM, the only remaining parts are mine:

1. create an issue for escaping special characters
2. restore the parts of the removed Note that are still relevant

I'll do these tomorrow, for now let's ask others to review.

[effectfully]

👍

[effectfully]

I think we probably don't need that here, it was needed to adjust the default and now we want the default behavior. Anyways, not important.

[zliu41]

I don't agree that we should print uniques in the golden files by default.

Most of the time, the uniques don't matter. They not only add noise and make things harder to read, but also, sometimes when you make a small change, it can change all the uniques in the golden file, which makes the actual changes extremely hard to find.

I've encountered a few tests that can benefit from adding uniques to the output, but I think we should only add uniques for those tests.

[bezirg]

I also disagree that it should be on by default. Don't we have some pretty debug mode for this kind of stuff? Then all the golden files could just use the debug pretty mode.

[effectfully]

There are two different things here:

1. whether the default config should prescribe to print the uniques
2. whether the golden test machinery should print them by default

I very strongly believe that when it comes to 1, the current master is straight out buggy. Pretty-printing a program and parsing it back shouldn't give you a semantically different program, that's just ridiculous. If we want to, we may pretty-print only those uniques that disambiguate names while ignoring others, but I'm not a fan of that: GHC adds "uniques" by default when it produces Core and I've never found that to be of any annoyance and I've spent quite a lot of time staring at GHC Core. So not fixing master in that regard is a non-option in my opinion, the current situation actively confuses our users (the original issue to fix the semantics changing pretty-printing exists because Runtime Verification ran into it, got very confused and, when I explained what was going on, very unhappy).

As for the second option, I feel less strongly, but I do believe that we should dump uniques into golden files by default. Your golden test is too large to have uniques? Well, then it's too large to be read anyway. Worst practices should be hard and it's not like using the non-default config is that hard. So I do believe that we should generally disincentivize creation of large golden files and if we can at the same time get correct scoping, that's a double win.

[bezirg]

I very strongly believe that when it comes to 1, the current master is straight out buggy. Pretty-printing a program and parsing it back shouldn't give you a semantically different program, that's just ridiculous.

I see now your point and I think also have ran into this before. But when does this exactly happen? Is it when there are free variables in the program or when we have to introduce fresh variables during the compilation?

[effectfully]

I see now your point and I think also have ran into this before. But when does this exactly happen? Is it when there are free variables in the program or when we have to introduce fresh variables during the compilation?

\x_0 x_1 -> x_0 being pretty-printed becomes \x x -> x, which has a different meaning. So basically any name shadowing is dangerous, which is ridiculous.

[Unisay]

I agree with:

• Default pretty printing config should produce a representation that always parses back (roundtrip);
• There might be some cases where we explicitly opt-out of printing indexes, it should be possible;
• The usage of golden files should be minimised and having to make an extra step to opt-out of indexes could have a positive effect in this dimension (aka "Worst practices should be hard")
• The "fragility" concern is real: if only indexes have changed but program didn't, then we don't want to see such change in the diff. @effectfully suggested that rename pass addresses this.

[zliu41]

I agree should fix 1. For 2, I really disagree - uniques in anything but the tiniest golden files are quite annoying. Added a let binding? Everything afterwards will have different uniques. Added a rename in one of the passes? Congratulations, every single unique in every single golden file will now be different. What benefit do we get for such trouble?

[Unisay]

I am in the process of re-working this PR in order to have a cake and eat it too:

• Unique indexes should be printed by default;
• Golden tests explicitly opt-in to a simple representation that doesn't print unique indexes;

This entails quite some boring work: the change isn't hard but its a lot files to visit and decide. Stay tuned.

[zliu41]

Looks like this file now has an additional pair of parentheses (it's hard to tell whether that's the only difference, but seems so?), which is redundant

[zliu41]

There are a number of golden files that now have additional redundant parentheses which appears to be what makes the line count so large. See the comment above for an example.

[effectfully]

now have additional redundant parentheses which appears to be what makes the line count so large.

I promised to take a look at it and got caught up with other stuff. Gonna do it now.

[effectfully]

@Unisay you've changed the behavior of prettyPirReadable in PlutusIR.Core.Instance.Pretty.Readable by turning botPrettyConfigReadable into topPrettyConfigReadable. So making it

-- | Pretty-print something with the @PrettyConfigReadable@ config.
prettyPirReadable :: PrettyPir a => a -> Doc ann
prettyPirReadable = prettyBy (botPrettyConfigReadable prettyConfigName def)

-- | Pretty-print something with the @PrettyConfigReadableSimple@ config.
prettyPirReadableSimple :: PrettyPir a => a -> Doc ann
prettyPirReadableSimple = prettyBy (botPrettyConfigReadable prettyConfigNameSimple def)

will solve the extra parens issue. BTW, we don't have two sets of parens there, one of them are there because a let-expression is applied to a let-expression, i.e. (let x = a in q) (let y = b in r), i.e. those parens are as expected. Only took me a few thousand pulled out hairs to figure out, phew.