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

typechecker: Evaluate bound expressions only once #1464

Closed
wants to merge 2 commits into from

Conversation

cg-jl
Copy link
Contributor

@cg-jl cg-jl commented Jul 3, 2023

Now Jakt inserts declarations to save the bound expressions before
checking them, and uses the variable names to access the bound parts.
This ensures that the original expression isn't run more than once.

Fixes #1450.

To do this I had to remove support for bindings inside match cases, part of
a feature made by @robryanx in commit
abab871, since it both allowed for weird match use cases and made the solution to this
issue more complicated.

@cg-jl cg-jl force-pushed the use-cached-names-in-is branch 3 times, most recently from 8d58f70 to 09fed15 Compare July 3, 2023 15:15
This was a way to achieve some level of pattern nesting, but it makes
for cryptic matches and makes it more difficult to avoid evaluating the
bound expression more than once, since its context doesn't easily allow
for statement insertions.
Now Jakt inserts declarations to save the bound expressions before
checking them, and uses the variable names to access the bound parts.
This ensures that the original expression isn't run more than once.
@cg-jl cg-jl changed the title typechecker: Bound expressions should only be evaluated once typechecker: Evaluate bound expressions only once Jul 3, 2023
Copy link
Member

@alimpfard alimpfard left a comment

Choose a reason for hiding this comment

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

Seems like this breaks stage 2 🤔


mut control_flow = checked_block.control_flow
if checked_else is Some(stmt) and stmt is Block(block: checked_else_block) {
control_flow = control_flow.unify_with(checked_else_block.control_flow)
Copy link
Member

Choose a reason for hiding this comment

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

This line needs an indent

@cg-jl
Copy link
Contributor Author

cg-jl commented Jul 4, 2023

Seems like this breaks stage 2

Yeah, I'm working on it. I've messed up some control flow based checks with producing CheckedBlocks manually.

@stale
Copy link

stale bot commented Jul 25, 2023

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions!

@stale stale bot added the stale label Jul 25, 2023
@stale
Copy link

stale bot commented Aug 1, 2023

This pull request has been closed because it has not had recent activity. Feel free to re-open if you wish to still contribute these changes. Thank you for your contributions!

@stale stale bot closed this Aug 1, 2023
@cg-jl
Copy link
Contributor Author

cg-jl commented Aug 4, 2023

Sorry for the delay. The idea here was to wrap the resulting values from a guard or an if in a block, converting:

if some_computation() is Some(x) {
    do_something_with(x)
}

into:

{
   let _cached_result = some_computation()
   if _cached_result is Some(x) {
       do_something_with(x)
   }
}

This looks redundant in Jakt code, but internally the typechecker transforms the first snippet to something like:

if some_computation().has_value() {
   do_something_with(some_computation())
}

This was hard to fix at the codegen level because it was already receiving the duplicate expressions from the typechecking stage. I opted for doing this in the typechecker, then. But by doing this I didn't get the metadata for the wrapping blocks correct, and the branch analysis of the typechecker for reachable/unreachable stuff then bit me in stage 2.

I haven't been able to get this metadata correct and I don't think it would be good anyway, I think it would be better to somehow generate parsed expressions again and let the typechecker expand on it, or generate those parsed expressions earlier in the parsing stages.

I will be leaving this as closed, I've become burnt out after trying seemingly incoherent modifications to solve the stage 2 problem.

@cg-jl cg-jl deleted the use-cached-names-in-is branch October 7, 2023 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

if foo() is Some(x) runs foo() twice
2 participants