Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CaseOfCase UPLC transformation tests. #5960

Merged
merged 5 commits into from
May 8, 2024

Conversation

Unisay
Copy link
Contributor

@Unisay Unisay commented May 6, 2024

  • Chores: extract caseOfCase tests from Simplifier module into a dedicated test module.
  • Chores: a bit of re-formatting (1st commit)
  • A test case that demonstrates how CaseOfCase UPLC transformation changes program evaluation result in a presence of Error. (2nd commit)
Untyped Plutus Core
  CaseOfCase
    CaseOfCase/1:                                  OK
    CaseOfCase/2:                                  OK
    CaseOfCase/3:                                  OK
    Transformation doesn't evaluate error eagerly: FAIL
      untyped-plutus-core/test/Transform/CaseOfCase/Test.hs:92:
      expected: EvaluationSuccess (Constant () (Some (ValueOf DefaultUniUnit ())))
       but got: EvaluationFailure
      Use -p '/CaseOfCase/&&/Transformation doesn'\''t evaluate error eagerly/' to rerun this test only.
  • Fix for the CaseOfCase UPLC transformation to make the test above pass.

Closes #5958

@Unisay Unisay added Don't look here yet No Changelog Required Add this to skip the Changelog Check labels May 6, 2024
@Unisay Unisay self-assigned this May 6, 2024
@Unisay Unisay marked this pull request as ready for review May 7, 2024 13:57
Copy link
Contributor

@effectfully effectfully left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of nitpicking, sorry.

Thank you for confirming that there's a bug in case-of-case indeed.

I'm not convinced by the solution, because case-of-case is supposed to be an optimization and if we're adding force-delay that aren't required in many cases, then are we even making things better?

I'm OK with merging the PR though, a not-too-optimizing optimization is certainly better than a broken one. We can always think of a better solution later.

@effectfully
Copy link
Contributor

BTW, if anybody is confused why we even bother optimizing ifThenElse b (constr <...>) (constr <...>), then here's where we produce such expressions:

Screenshot from 2024-05-07 18-12-16

@Unisay Unisay force-pushed the yura/5958-case-of-case-error branch from 9aa9b46 to f3d65a7 Compare May 8, 2024 10:50
@Unisay
Copy link
Contributor Author

Unisay commented May 8, 2024

  • Rebased on master.
  • Addressed review comments.
  • Isolated caseOfCase transformation in tests.

@ana-pantilie
Copy link
Contributor

ana-pantilie commented May 8, 2024

BTW, if anybody is confused why we even bother optimizing ifThenElse b (constr <...>) (constr <...>), then here's where we produce such expressions:

Screenshot from 2024-05-07 18-12-16

@effectfully why are those functions defined the way they are? Nvm, I see it's because you need to convert from bools to builtin bools. This builtin machinery is so awkward in so many places...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One budget golden file change? When there were like 20 in the original PR? I don't get it, gonna take a look.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Unisay or feel free to take a look yourself why we're not seeing more changes here.


goldenVsSimplified :: String -> Term Name PLC.DefaultUni PLC.DefaultFun () -> TestTree
goldenVsSimplified name =
goldenVsString name ("untyped-plutus-core/test/Transform/CaseOfCase/" ++ name ++ ".uplc.golden")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use /, use </> or joinPath. Those handle weird corner cases correctly and are cross-platform. Also we have the TestNested machinery for doing these things, but I see that it wasn't in these tests originally.

@effectfully
Copy link
Contributor

@ana-pantilie

Nvm, I see it's because you need to convert from bools to builtin bools. This builtin machinery is so awkward in so many places...

This is very much by design, originally the builtins machinery would return a regular Plutus bool (Scott-encoded back then), but folks wanted me to remove that feature and return built-in bools instead, so I did that in like 2019.

Although now that you mention it, we'll hopefully soon be able to return SOPs from builtins, sounds like we need to come full circle and add builtins returning SOP bools directly.

In any case, for you it's obvious that we should return SOPs and not doing so seems awkward. But for, say, Aiken people it's obvious that we should return constants instead, because they prefer to pretend that SOPs don't exist. It's all subjective.

@effectfully
Copy link
Contributor

I've reviewed this once again, I don't think any of my comments are blockers, so let's merge it and do follow-ups:

  1. why did only one budget test changed? That's quite weird
  2. should we avoid generating force-delay when branches are pure?

@zliu41 please still review this PR.

@effectfully effectfully merged commit b6e82b6 into master May 8, 2024
7 checks passed
@effectfully effectfully deleted the yura/5958-case-of-case-error branch May 8, 2024 22:10
@effectfully
Copy link
Contributor

Oh, and great work, thank you @Unisay!

@effectfully
Copy link
Contributor

@Unisay just made an issue for you, not urgent. I decided not to dive into it for now, so hopefully you could.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
No Changelog Required Add this to skip the Changelog Check
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Verify that CaseOfCase UPLC transformation doesn't make expression stricter than needed
3 participants