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

More reliable short-cicruiting behavior for || and && operators. #5863

Merged
merged 5 commits into from
Apr 2, 2024

Conversation

Unisay
Copy link
Contributor

@Unisay Unisay commented Mar 28, 2024

Before the fix:

  ShortCircuiting operators
    (&&) short-circuits with GHC optimisations:    OK
    (||) short-circuits with GHC optimisations:    OK
    (&&) short-circuits without GHC optimisations: FAIL
      test/ShortCircuit/Spec.hs:50:
      An error has occurred:  User error:
      The machine terminated because of an error, either from a built-in function or from an explicit use of 'error'.
      Use -p '/ShortCircuiting/&&/(&&) short-circuits without GHC optimisations/' to rerun this test only.
    (||) short-circuits without GHC optimisations: FAIL
      test/ShortCircuit/Spec.hs:50:
      An error has occurred:  User error:
      The machine terminated because of an error, either from a built-in function or from an explicit use of 'error'.
      Use -p '/ShortCircuiting/&&/(||) short-circuits without GHC optimisations/' to rerun this test only.

2 out of 4 tests failed (0.01s)

After the fix:

  ShortCircuiting operators
    (&&) short-circuits with GHC optimisations:    OK
    (||) short-circuits with GHC optimisations:    OK
    (&&) short-circuits without GHC optimisations: OK
    (||) short-circuits without GHC optimisations: OK

All 4 tests passed (0.01s)

The fix is implemented as a code rewrite when compiling an expression:
a && b is rewritten as if a then b else False
a || b is rewritten as if a then True else b

Based on this un-merged PR.

Closes #4114

@Unisay Unisay force-pushed the yura/PLT-9667/short-circuit-and-or branch 5 times, most recently from 13d5855 to 96920bf Compare March 28, 2024 12:55
@Unisay Unisay force-pushed the yura/PLT-9667/short-circuit-and-or branch from 96920bf to 65b2d8f Compare April 2, 2024 12:22
@Unisay Unisay requested review from michaelpj and zliu41 April 2, 2024 12:29
@Unisay Unisay force-pushed the yura/PLT-9667/short-circuit-and-or branch from 65b2d8f to 3d3c478 Compare April 2, 2024 12:40
@Unisay Unisay marked this pull request as ready for review April 2, 2024 12:52
Copy link
Contributor

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

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

I don't really like special-casing certain functions in the compiler. But I think I am outvoted on this 🤷

@Unisay Unisay enabled auto-merge (squash) April 2, 2024 16:39
@Unisay Unisay merged commit 77e308a into master Apr 2, 2024
5 of 6 checks passed
@Unisay Unisay deleted the yura/PLT-9667/short-circuit-and-or branch April 2, 2024 18:05
Copy link
Member

@zliu41 zliu41 left a comment

Choose a reason for hiding this comment

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

@Unisay A few more comments.

PlutusTx.Bool.&& and || should now be marked NOINLINE, since we don't need their unfoldings.

I don't really like special-casing certain functions in the compiler.

@michaelpj Yeah but if anything should be special-cased, it's && and ||, since they are special in almost all other strict languages.

@zliu41
Copy link
Member

zliu41 commented Apr 3, 2024

There's a Note [Short-circuit evaluation for && and ||] which is now out of date.

The Note should be rewritten to explain that saturated applications of && and || always short-circuit, but unsaturated applications won't.

@zliu41
Copy link
Member

zliu41 commented Apr 3, 2024

I wonder if we should raise an error if || and && is used unsaturated. Or at least a warning, but I don't know that the Plutus Tx compiler has the ability to emit warnings.

v0d1ch pushed a commit to v0d1ch/plutus that referenced this pull request Dec 6, 2024
…ntersectMBO#5863)

* chore: fix hlint warnings

* Test case that demonstrates lack of short-curicuiting under no GHC optimisations.

* Compile (&&), (||) as if .. then .. else

* Changelog entry

* Comment about Short-curcuiting && || tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

overzZzealous: Non-lazy Semantics of || and && in Plutus Core
3 participants