Skip to content

Conservatively fold matcher-like builtins in EvaluateBuiltins#7680

Draft
dancewithheart wants to merge 2 commits intoIntersectMBO:masterfrom
dancewithheart:fix/evaluate-builtins-matcher-like-unsound-6167
Draft

Conservatively fold matcher-like builtins in EvaluateBuiltins#7680
dancewithheart wants to merge 2 commits intoIntersectMBO:masterfrom
dancewithheart:fix/evaluate-builtins-matcher-like-unsound-6167

Conversation

@dancewithheart
Copy link

Fixes #6167 by

  • Detect matcher-like builtins via biMatcherLike
  • Allow folding only when all branches are value-like
  • Otherwise, leave the term unchanged

This preserves existing optimizations (e.g. ifThenElse True 1 2) while avoiding the unsound case e.g. when the discarded branch fails:

[ { (builtin ifThenElse) (con integer) }
    (con bool True)
    (con integer 1)
    [ [ (builtin divideInteger) (con integer 1) ] (con integer 0) ]
]

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

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.

The evaluate-builtins optimization isn't conservative

1 participant