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

Add branching model to constraint builder #430

Merged
merged 13 commits into from
Jan 8, 2023

Conversation

JSAbrahams
Copy link
Owner

@JSAbrahams JSAbrahams commented Jan 3, 2023

Relevant issues

Summary

Another addition to the constraint builder, similar to #416 .
This time, we implement a new branching model which allows us to now finally properly check converging paths:

  • We can mark a branch point
  • Any call to a branch_from(branch point) will then create a new set.
    It will copy over all constraints from the previous set, up to the branch point.

We now only create branches for:

  • If expressions
  • Match expressions

This means that we don't unnecessarily create new sets when entering classes, for loops, while loops, etc.
Only when we have diverging execution paths.
Shadowing of variables is now no longer maintained partially by different sets, but instead by shadowing of variables in:

  • The global scope (stored in the constraint builder)
  • In the local scope, stored by the local Environment, which is used first before reverting to the global scope to lookup variables.
  • This means that when we encounter a new variable we add it both to the global scope (in the constraint builder) and the local environment.
  • We always lookup variables in the environment.
    This means that even if a mapping exists in the global scope, if the variable does not exist in the local environment at all then we still get an error.
    This same logic is also used when we recursively substitute identifiers within ASTs.
  • Lastly, we explicitly override the self from the environment with the global mapping of self when exiting a class..

As a result of this refactor, we also moved some var mapping logic to the builder to make it easier to change.
This means that Expected::from(AST) has been greatly simplified.
In fact, there was no reason for it to return a result now.

Destruction of variables

Due to the change in behavior, we had to add destruction variables in the environment in case we are no longer allowed to access them:

  • In the body of a with <resource> as <alias>, where the resource itself is now no longer accessible.

In other locations where we use a type alias we should also destruct variables.
For instance, if in match if we in future allow type aliases to be used there.

Propagate assigns

If we assign to a variable ,that variable has a type.
Normally we annotate this with a type.
We now also do this even when we propagate an assign into a match or if:

Example with if:

def my_var := if True then
    print("then")
    10
else
    print("else")
    20

Generates:

if True:
    print("then")
    my_var: int = 10
else:
    print("else")
    my_var: int = 20

Before:

if True:
    print("then")
    my_var = 10
else:
    print("else")
    my_var = 20

@JSAbrahams JSAbrahams added the enhancement: generate New feature in the core module label Jan 3, 2023
@JSAbrahams JSAbrahams added this to the v0.3.6 | Dictionaries milestone Jan 3, 2023
@JSAbrahams JSAbrahams self-assigned this Jan 3, 2023
@JSAbrahams JSAbrahams changed the title Propagate ast type to definitions in match, if Branching model in constraint builder Jan 5, 2023
@codecov
Copy link

codecov bot commented Jan 5, 2023

Codecov Report

Merging #430 (47478a7) into develop (25f6ef4) will increase coverage by 0.25%.
The diff coverage is 96.51%.

@@             Coverage Diff             @@
##           develop     #430      +/-   ##
===========================================
+ Coverage    88.34%   88.59%   +0.25%     
===========================================
  Files          110      110              
  Lines        11997    11956      -41     
===========================================
- Hits         10599    10593       -6     
+ Misses        1398     1363      -35     
Impacted Files Coverage Δ
src/check/constrain/generate/mod.rs 95.08% <ø> (ø)
src/check/constrain/mod.rs 100.00% <ø> (+28.88%) ⬆️
src/parse/collection.rs 89.51% <ø> (+2.79%) ⬆️
src/check/constrain/unify/function.rs 78.26% <12.50%> (-3.23%) ⬇️
src/check/constrain/generate/collection.rs 94.25% <92.59%> (-2.53%) ⬇️
src/generate/convert/mod.rs 95.19% <95.00%> (+0.58%) ⬆️
src/check/constrain/generate/env.rs 98.80% <95.23%> (+4.36%) ⬆️
src/check/constrain/constraint/builder.rs 96.50% <97.60%> (+10.50%) ⬆️
src/check/constrain/constraint/expected.rs 90.24% <100.00%> (+2.12%) ⬆️
src/check/constrain/constraint/iterator.rs 84.61% <100.00%> (-1.60%) ⬇️
... and 59 more

Well the unit tests now pass.
Predictably, some integration tests don't.

Mostly problems with:
- Catching exceptions
- Some issues with shadowing
@JSAbrahams JSAbrahams changed the title Branching model in constraint builder Update branching in constraint builder Jan 5, 2023
Need to fix now how variable mapping and branching
interact.
Still some issues in regard to shadow logic.

Remove Environment union and intersection.
This is a language feature we actually don't
want.
Use new system to revert behaviour of collection.
I don't think we need the Expect::Collection
construct at all.
We only need to know whether something defines
__iter__().
We can use an access constraint for that.
Will add in future.

- Ignore types because it uses type aliases
- Revert some behaviour in definition check.
  Double check if this isn't due to change in
  behaviour in generate stage.
@JSAbrahams JSAbrahams marked this pull request as ready for review January 7, 2023 16:26
@JSAbrahams JSAbrahams marked this pull request as draft January 7, 2023 16:44
@JSAbrahams JSAbrahams marked this pull request as ready for review January 8, 2023 12:02
@JSAbrahams JSAbrahams changed the title Update branching in constraint builder Add branching model in constraint builder Jan 8, 2023
@JSAbrahams JSAbrahams changed the title Add branching model in constraint builder Add branching model to constraint builder Jan 8, 2023
@JSAbrahams JSAbrahams merged commit 7d06ae2 into develop Jan 8, 2023
@JSAbrahams JSAbrahams deleted the propagate-type-to-vardef branch January 8, 2023 12:16
@JSAbrahams JSAbrahams mentioned this pull request Jan 21, 2023
JSAbrahams added a commit that referenced this pull request Jan 21, 2023
* Propagate ast type to definitions in match, if

* Add branch points to constraint builder

* Only check raises caught if in function

* Move AST map logic to expected

* Allow change order variable mapping

* Remove unused default Constraints

* Environment mapping takes precedence in Expect map

* Fix constraint generation in lookup

* Add destruction of variables in environment

* Preserve self mapping in env between classes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement: generate New feature in the core module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Type annotation cannot be unified with union types Constraint builder should implement branching model
1 participant