Typecheck yield/resume #449
Conversation
joelburget
left a comment
joelburget
left a comment
There was a problem hiding this comment.
I think this looks pretty good. I just had the one question about whether something should be an error. I also have an additional note / question: It should be possible to reject a step like:
(step
(if flag
(yield { 'a : 1 })
"no yield"))
Based on my reading of the code, we don't currently reject this. Should we?
| debug $ "assocYRSchemas: " ++ show (a,a',b,b') | ||
| assocParams (_aId a) a' b' | ||
|
|
||
| assocStepYieldReturns _ _ = return () |
There was a problem hiding this comment.
should this be an error?
There was a problem hiding this comment.
No, as this is applied to any top level -- we only want this for matching on defpact
| let specialBind = do | ||
| args'' <- notEmpty _appInfo "Expected >1 arg" (init args') | ||
| mkApp (set fSpecial (Just (sf,SBinding (last args'))) fun') args'' | ||
| initArgs <- init <$> notEmpty _appInfo "Invalid binding invocation" args' |
There was a problem hiding this comment.
Is my understanding correct that previously all bindings required at least one argument (plus the binding), but now we have resume, which takes no arguments (plus a binding)?
There was a problem hiding this comment.
Yep. Seems to do no harm as the other binding funs have differing arity anyways (e.g., with-default-read)
|
Sigh ... flaky web test in gitlab |
|
@joelburget not sure how the typechecker will reject your example or other pathologies like noted in the PR description. The solution so far simply gathers yields and resumes, enforces they respectively associate within a step, and then performs rules across steps. This is already considerably more introspective than other typechecking tasks -- the step rules are tractable mainly because they can all be inspected at a single level (ie the defpact body). One thing I can say about your example is |
joelburget
left a comment
joelburget
left a comment
There was a problem hiding this comment.
lgtm -- thanks for figuring this out
Fixes #442
Caveats:
Types must be specified somewhere in order to typecheck. I briefly investigated being able to specialize object literals on the fly a la
{ 'foo: 1 }:object{bar}(which would be ugly anyway) but Compile didn't like it. The example in the test shows the upshot:pact/tests/pact/tc.repl
Line 155 in 2a49f37
Return types on
defpacts are weird/pathological. They associate to the return value of the last step, which itself is associated to the step app (ie, ignoring the rollback). Thus in the example we have steps returning different types.Resume/yield in rollback is currently permitted, as the environment simply gathers
yield/resumeinvocations within astepand associates them (respectively).NB:
Defun{}Funs now indicates def type, which was ignored before; resume/yield nodes are stored inStep{}ASTs.