-
-
Notifications
You must be signed in to change notification settings - Fork 38
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
Rewrite applied LET/IN
s in the builder
#2536
Conversation
Following #2534, but putting it where it'll help with quint conversion.
Now being delegated to the builder.
This reverts commit 9d5a00a. This can't be useful as is, because any input will choke on the type checker before we ever get to the preprocessing logic. The previous commits move the core logic into the builder, where it fixes the problem hit during conversion.
Codecov Report
@@ Coverage Diff @@
## main #2536 +/- ##
==========================================
- Coverage 78.54% 78.50% -0.05%
==========================================
Files 442 441 -1
Lines 15560 15535 -25
Branches 2518 2507 -11
==========================================
- Hits 12221 12195 -26
- Misses 3339 3340 +1
... and 4 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
This will be passing once informalsystems/apalache#2536 is merged.
def appOp(Op: TlaEx, args: TlaEx*): TlaEx = { | ||
Op match { | ||
// This is a workaround for the fact that that we currently de-lambda, | ||
// because lambdas are not supported in the Apalache IR. See | ||
// https://github.com/informalsystems/apalache/issues/2532 | ||
case LetInEx(nameEx @ NameEx(operName), decl) if operName == decl.name => | ||
val appliedByName = buildBySignatureLookup(TlaOper.apply, nameEx +: args: _*) | ||
LetInEx(appliedByName, decl)(appliedByName.typeTag) | ||
case _ => buildBySignatureLookup(TlaOper.apply, Op +: args: _*) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I like us pushing computations, which have nothing to do with building expression trees, but rather compensate for a particular deficiency in our rewriting process, into the Builder itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review and discussion!
To note the upshot of our convo:
- We identified that, in principle, to hold to the logic of TLA, we should not allow returning operator. So, in principle, the
LET/IN
form used to encode lambdas shouldn't even be allowed. - However, we have inherited a situation in which this otherwise illegal form is used internally to represent lambdas.
- Since we should be able to apply lambdas, it makes sense given our current imperfect state to support this construction in the builder.
- The key caveat is that this should be removed (and replaced with a validation check) once we introduce lambdas via Support
LAMBDA
in the Apalache IR #2532.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed this in a call.
This will be passing once informalsystems/apalache#2536 is merged.
This will be passing once informalsystems/apalache#2536 is merged.
Closes #2531
Borrows from and reverts #2534. See c59ed3d and
#2534 (comment)
for context.
This provides a workaround for #2532, by rewriting
(LET f = ... IN f)(x)
as(LET f = ... IN f(x))
.make fmt-fix
(or had formatting run automatically on all files edited)./unreleased/
for any new functionality