Don't evaluate away builtins where the result might be unserializable

[michaelpj]

See the note. The test case does evaluate the uncompress application until you add the guard, so this is working as desired. We can see if it fixes Kenneth's problem.

Pre-submit checklist:

• Branch
  • Tests are provided (if possible)
  • Commit sequence broadly makes sense
  • Key commits have useful messages
  • Changelog fragments have been written (if appropriate)
  • Relevant tickets are mentioned in commit messages
  • Formatting, PNG optimization, etc. are updated
• PR
  • (For external contributions) Corresponding issue exists and is linked in the description
  • Targeting master unless this is a cherry-pick backport
  • Self-reviewed the diff
  • Useful pull request description
  • Reviewer requested

[bezirg]

This is a solution indeed. After reading your PR, I started wondering if going with option 3 (using a rewrite rule) is a better solution.

The way this evaluateBuiltins bottom-up transformation is that it will go deep and cancel the saturated uncompress builtin. The problem is that when the transformation will go up, it will not apply an evaluate builtin which is worthwile and would not lead to serialisazation problems. Anyway it is difficult to explain so here is some test you could try:

(Bls12_381_G1_equal (bls_uncompress "bytestring") (bls_uncompress "bytestring")).

This should ideally turn into (con bool true), but based on what I am reading in this PR, I suspect that it will not reduce

[kwxm]

(Bls12_381_G1_equal (bls_uncompress "bytestring") (bls_uncompress "bytestring")).

Just for information, uncompress will usually fail on a random bytestring. You can get a valid point on the curve by using hashToGroup followed by compress though. One valid compressed point is

(con bytestring # 97f1d3a73197d7942695638c4fa9ac0fc3688c4f9774b905a14e3a3f171bac586c55e83ff97a1aeffb3af00adb22c6bb)

in PLC, or

"\x97\xf1\xd3\xa7\x31\x97\xd7\x94\x26\x95\x63\x8c\x4f\xa9\xac\x0f\xc3\x68\x8c\x4f\x97\x74\xb9\x05\xa1\x4e\x3a\x3f\x17\x1b\xac\x58\x6c\x55\xe8\x3f\xf9\x7a\x1a\xef\xfb\x3a\xf0\x0a\xdb\x22\xc6\xbb"

with OverloadedStrings in Haskell.

[michaelpj]

The problem is that when the transformation will go up, it will not apply an evaluate builtin which is worthwile and would not lead to serialisazation problems.

Yep, that's absolutely a downside. I'm not so worried about that.

After reading your PR, I started wondering if going with option 3 (using a rewrite rule) is a better solution.

I think it's generally a bit awkward to have a pass that undoes what a different pass does. Not impossible though! One reason I like the current approach is that I think it also naturally extends to other types we might have in future that have this problem.

On the other hand, I do think that running a later pass is nice in that the earlier passes don't have to know about the serializability problem. If we had many passes that could produce new constants then what we're doing here would be quite annoying.

So I'm unsure.

[bezirg]

I am ok to go with this more principal approach, but I would appreciate a comment that 'a separate later pass to undo the evaluation would optimize more'. Also add please my example as a golden test with a comment!

[kwxm]

I've tried this on my branch and it enables me to get rid of all the workarounds and write what I'd expect.

[kwxm]

This solves the original problem and the downsides don't seem too problematic. Maybe we could revisit it if we think of a better approach later (although that's a good way to bury the issue in the backlog for months/years).

[kwxm]

You could tediously spell out all the other cases here so that if we add more types to the universe we'd be forced to look at this.

[michaelpj]

I could, but I'm not sure it's worth it. This is a weird special case...

[kwxm]

Do we care about -ize versus -ise? Probably not: I think we're pretty inconsistent elsewhere. You could go for broke and call them "unzerializable" though.

[michaelpj]

We're massively inconsistent. I'm inconsistent in my own writing. I have no idea which is which, I just use them randomly.

[kwxm]

Maybe do these for G1 too?

[kwxm]

(Bls12_381_G1_equal (bls_uncompress "bytestring") (bls_uncompress "bytestring")).

This should ideally turn into (con bool true), but based on what I am reading in this PR, I suspect that it will not reduce

I think you're right, but I'd suggest that if you've got a term involving these things that can be statically reduced then you've done something wrong because you probably shouldn't be doing that computation on the chain anyway.

[michaelpj]

Ah, I remembered the other reason not to do a later pass: it can't handle the Ml-result type. There isn't AFAIK a canonical way to serialise those at all, so we can't replace them with an equivalent builtin application, we just need to not produce any.

[kwxm]

There isn't AFAIK a canonical way to serialise those at all

That's right. See this for example.

[michaelpj]

I am ok to go with this more principal approach, but I would appreciate a comment that 'a separate later pass to undo the evaluation would optimize more'. Also add please my example as a golden test with a comment!

Added.