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.

Sign up for GitHub

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

Jump to bottom

Add global variable mapping and preserve primitives #416

Merged
merged 22 commits into from
Jan 4, 2023
Merged

Add global variable mapping and preserve primitives #416

merged 22 commits into from
Jan 4, 2023

Conversation

JSAbrahams
Copy link
Owner

@JSAbrahams JSAbrahams commented Dec 30, 2022 •
edited
Loading

Relevant issues

Summary

Quite a big PR as we had to change some fundamental behaviour.
More fundamentally how we deal with constraint generation.
Diff slightly larger due to some renaming of variables or consistent change in behaviour, namely how we reference var_mapping.

Multiple constraint sets

We now change the behaviour of the ConstrBuilder such that we always add constraints to multiple sets in parallel.
This allows us to generate constraints for all possible execution paths (for instance, after an if or match).

We can then either:

  • Exit a set locally, thus preventing the constraints here from interacting with constraints further down the line.
  • Not exit the set, thus increasing for now the amount of constraints being added to.
    One can use bookmarks (usize) so one exits to the correct level at a later stage without knowing how many sets are being added to in parallel.

Pushing types

Get rid of unnecessary unions by updating logic of how push_ty operates.
Whether one has a union of types is determined there and whether any of the Names is interchangeable.

  • Streamline further some Expect logic which was redundant, which got rid of primitives by directly substituting them.
    Instead, all constraints should be generated in the generate stage, including those for primitives.
    This further simplifies the logic of primitives and makes them more like any other class, which they essentially are (like any good OOP-like language).
  • Fix is_superset_of logic in Name.
    Logic before was erroneous, but not caught due to faulty test.

Tests

Happy

  • Modify arithmetic test to get rid of unnecessary union.
  • Modify factorial test to get rid of unnecessary union of input function output.
    Sad
  • Is superset of Name where other is interchangeable but none of self is super of other
  • Like previous, but with same class but other nullable (so self is not super)

@JSAbrahams JSAbrahams added the bug: check Something in the type check module isn't working (as intended) label Dec 30, 2022
@JSAbrahams JSAbrahams added this to the v0.3.6 | Dictionaries milestone Dec 30, 2022
@JSAbrahams JSAbrahams self-assigned this Dec 30, 2022
@codecov
Copy link

codecov bot commented Dec 30, 2022 •
edited
Loading

Codecov Report

Merging #416 (14b36a8) into develop (9723d5f) will decrease coverage by 0.10%.
The diff coverage is 95.39%.

❗ Current head 14b36a8 differs from pull request most recent head f497055. Consider uploading reports for the commit f497055 to get more accurate results

@@             Coverage Diff             @@
##           develop     #416      +/-   ##
===========================================
- Coverage    88.43%   88.33%   -0.11%     
===========================================
  Files          110      110              
  Lines        12040    12000      -40     
===========================================
- Hits         10648    10600      -48     
- Misses        1392     1400       +8
Impacted Files Coverage Δ
src/check/constrain/unify/mod.rs 75.00% <20.00%> (+12.93%) ⬆️
src/check/constrain/constraint/iterator.rs 86.20% <66.66%> (-0.46%) ⬇️
src/lib.rs 82.89% <75.00%> (+1.19%) ⬆️
src/check/result.rs 82.22% <83.33%> (ø)
src/check/constrain/constraint/builder.rs 86.27% <85.71%> (-2.19%) ⬇️
src/check/constrain/generate/env.rs 94.44% <96.15%> (-0.56%) ⬇️
src/check/constrain/constraint/expected.rs 88.11% <100.00%> (-1.61%) ⬇️
src/check/constrain/generate/call.rs 88.20% <100.00%> (+0.10%) ⬆️
src/check/constrain/generate/class.rs 82.14% <100.00%> (-6.99%) ⬇️
src/check/constrain/generate/collection.rs 96.77% <100.00%> (+0.03%) ⬆️
... and 23 more

@JSAbrahams
Copy link
Owner Author

Before merging need to document why this works, because the underlying mechanics are starting to become a bit unclear.

@JSAbrahams JSAbrahams changed the title If old name is interchangeable, use instead If old Name is interchangeable, use instead Dec 31, 2022
@JSAbrahams
Copy link
Owner Author

Something went wrong after rebase on develop.
So turns out that what we were doing here was actually quite dangerous, could've known.

@JSAbrahams
Copy link
Owner Author

JSAbrahams commented Jan 2, 2023 •
edited
Loading

It seems that we are now also accidentally allowing any argument, regardless of type, to be passed to functions.
Possibly something wrong with how the any boolean in Name affects is_superset_of.
See #426

This was linked to issues Jan 2, 2023
Unifying with fun args creates unnecessary union #404
Closed
Calls to function within current environment not constrained #426
Closed
This was referenced Jan 3, 2023
Merged
Closed
@JSAbrahams JSAbrahams marked this pull request as ready for review January 3, 2023 19:53
@JSAbrahams
Copy link
Owner Author

JSAbrahams commented Jan 3, 2023 •
edited
Loading

Must investigate significant drop in coverage first.

One thing which I'm not sure about is what if we shadow a variable with another one of a different type.
I say this because I'm not quite sure how the environment and global mapping interact.
But I can't think of a definitive way to test this, but a good place to start would be to shadow variables inside a function:

  • Shadow function argument
  • Shadow class field
  • Shadow outer field

In each test, make sure that we call methods only of that specific class.

@JSAbrahams JSAbrahams changed the title If old Name is interchangeable, use instead Add global variable mapping and don't throw away primitives Jan 3, 2023
@JSAbrahams JSAbrahams changed the title Add global variable mapping and don't throw away primitives Add global variable mapping and preserve primitives Jan 3, 2023
@JSAbrahams
If old name is interchangeable, use instead
6121aa9 
This means that old unions are preserved.
However, this doesn't quite explain to me why
we get rid of the unnecessary unions.
One would expect that we instead get even more
unnecessary unions.
As of now I can't quite yet explain this change
in behaviour, but should be documented once I
understand the change in behaviour.
@JSAbrahams
If expr of var declaration empty, ignore
fc91f5d 
Typically an indication that something somewhere
else went wrong.
However, an extra failsafe isn't too bad, though
it does negatively affect coverage if we fix the
problem elsewhere of course, though perhaps we're
treating the metric with that line of thought.
@JSAbrahams
Fix any_super and is_superset_of in Name
7444a10
@JSAbrahams
[ci skip] Simplify Expression try_from AST
a5d2acd 
By treating all primitives as their type, we
are unable to properly substitute.
However, does cause some tests to fail which
relied on this behaviour:
If expressions, however, no longer appear to be
typed.
@JSAbrahams
[ci skip] Split up constraints for if else arms
77e4444 
If as a whole is now typed.
However, not when used, such as in function body
or variable definition.
@JSAbrahams
[ci skip] Also gen then outside if
8007f51 
This is to ensure that the constraints are
available outside the if.
For instance, when we want to substitute the if
with the then branch.
We don't use the environment, so we shouldn't
have any variable contamination.
@JSAbrahams
Constraintbuilder deals with diverging paths
42f7d23 
We can now create multiple sets which are added to
in parallel for diverging paths.
The number of active sets is determined by the
difference between two internal unsigend numbers.
@JSAbrahams
[ci skip] Remove old constr, prevent contamination
8056c60 
We do now have cross contamination with variable
names.
Are not properly shadowed now.

Remove drain filter in ConstrBuilder
The method using this was used in a very specific
context in with.
If not reverted, can now compile with stable rust!
@JSAbrahams
Move variable mapping to constraint builder
e6d4039
@JSAbrahams
[ci skip] Make variable mapping top-level
dde7108 
If mapping of new var non-existent, add it to
mapping.
@JSAbrahams
Add local variable mapping to environment
69ce928 
This takes precedent, meaning that local shadows
are used.
If not found, then use global.
Useful if we have multiple execution paths and
later down the line need to perform substitutions.
@JSAbrahams
Fix off-by-one for exit set
befe23f
@JSAbrahams
Define self when entering class using new system
9c3b96a 
As opposed to creating a new constraint.
When accessing self this should be dealt with
using the global variable mapping now.
@JSAbrahams
Ignore empty Name in push_ty
c20cc1b
@JSAbrahams
Add test for shadowing in script, class, function
ffbb235
@JSAbrahams
Generate constraints for ast without access
ba0c2b0 
- Simplify class logic in environment
- Add logic for self type in match_id.
- Remove logic for adding constraints for any
  potential field access of self.
@JSAbrahams
Generate constraints for ast without access
bc0cf86 
- Simplify class logic in environment
- Add logic for self type in match_id.
- Remove logic for adding constraints for any
  potential field access of self.
@JSAbrahams
Copy link
Owner Author

Streamlined quite some logic.
After this, implementing dictionary should hopefully be more straightforward.

@JSAbrahams
Only define self in functions, not class-level
4d92a83 
- Ignore type aliases explicitly for now
- Remove unnecessary logic in match_id
- Do actually shadow self, as self may de defined
  multiple times in sets inheriting from top-level
  set.
@JSAbrahams
Use bitwise OR in Name is_superset_of method
c582bf9
@JSAbrahams
Remove redundant reference
14b36a8
@JSAbrahams
Use panic to denote bugs in the check stage
f497055
@JSAbrahams JSAbrahams merged commit 75a5ee1 into develop Jan 4, 2023
@JSAbrahams JSAbrahams deleted the fix-unnecessary-union branch January 4, 2023 19:27
JSAbrahams added a commit that referenced this pull request Jan 21, 2023
@JSAbrahams
Add global variable mapping and preserve primitives (#416)
ef7ee64 
* If old name is interchangeable, use instead

This means that old unions are preserved.
However, this doesn't quite explain to me why
we get rid of the unnecessary unions.
One would expect that we instead get even more
unnecessary unions.
As of now I can't quite yet explain this change
in behaviour, but should be documented once I
understand the change in behaviour.

* If expr of var declaration empty, ignore

Typically an indication that something somewhere
else went wrong.
However, an extra failsafe isn't too bad, though
it does negatively affect coverage if we fix the
problem elsewhere of course, though perhaps we're
treating the metric with that line of thought.

* Fix any_super and is_superset_of in Name

* Simplify Expression try_from AST

By treating all primitives as their type, we
are unable to properly substitute.
However, does cause some tests to fail which
relied on this behaviour:
If expressions, however, no longer appear to be
typed.

* Split up constraints for if else arms

If as a whole is now typed.
However, not when used, such as in function body
or variable definition.

* Also gen then outside if

This is to ensure that the constraints are
available outside the if.
For instance, when we want to substitute the if
with the then branch.
We don't use the environment, so we shouldn't
have any variable contamination.

* Constraintbuilder deals with diverging paths

We can now create multiple sets which are added to
in parallel for diverging paths.
The number of active sets is determined by the
difference between two internal unsigend numbers.

* Remove old constr, prevent contamination

We do now have cross contamination with variable
names.
Are not properly shadowed now.

Remove drain filter in ConstrBuilder
The method using this was used in a very specific
context in with.
If not reverted, can now compile with stable rust!

* Move variable mapping to constraint builder

* Make variable mapping top-level

If mapping of new var non-existent, add it to
mapping.

* Add local variable mapping to environment

This takes precedent, meaning that local shadows
are used.
If not found, then use global.
Useful if we have multiple execution paths and
later down the line need to perform substitutions.

* Fix off-by-one for exit set

* Define self when entering class using new system

As opposed to creating a new constraint.
When accessing self this should be dealt with
using the global variable mapping now.

* Ignore empty Name in push_ty

* Add test for shadowing in script, class, function

* Generate constraints for ast without access

- Simplify class logic in environment
- Add logic for self type in match_id.
- Remove logic for adding constraints for any
  potential field access of self.

* Explicitly destroy mapping in Environment

* Only define self in functions, not class-level

- Ignore type aliases explicitly for now
- Remove unnecessary logic in match_id
- Do actually shadow self, as self may de defined
  multiple times in sets inheriting from top-level
  set.

* Use panic to denote bugs in the check stage
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@JSAbrahams JSAbrahams

Labels
bug: check Something in the type check module isn't working (as intended)
Projects
None yet
Milestone
v0.3.6 | Dictionaries
1 participant
@JSAbrahams