Rework Collections for `Expect` and `Name` #439

JSAbrahams · 2023-01-09T15:27:09Z

Relevant issues

Do not merge before Remove unused ConstrVariant #440
Resolves Constraint generation can be generalized to remove Expect::Collection construct #437
Might resolve Check trips up when we introduce untyped generics in class signature #432

Summary

As has been a running theme for the past few days, this PR changes quite a lot more than originally intended.
However, we had some sloppy code which I figured would be best to clean up now instead of laborously splitting it over many PR's.
For now I see this a one in a set of major refactors, which for now is fine since I'm the only active developer.
Best to just improve the quality of the codebase now and get rid of as many bugs as possible by generalizing the type system as much as possible.
This includes getting rid of as much superfluous logic as possible.

Main change: `Except::Collection` to `Expect::Call { ... }`

Remove the collection Expect construct.
Instead, we use generics now and substitution of generics to deal with unification of collections.

Trim Collection type in annotated AST since that is not a valid type annotation in Python.
Add recursive substitution of names.
Useful if we want to substitute temporary names.
Add unification and substitution of generics within names.
Useful if we want to substitute generics which are temporary names.

This greatly simplifies a lot of exceptions we had to make for collections.
We also get to make use of type annotation logic now.

Secondary: Remove `NameVariant`

Instead, we just make use of Tuple[...] and Callable[Tuple[...],[...] by making helper methods which create these for us on the fly.
Under the hood, it's always StringName.
For now, I leave optionality untouched, so we don't make use of Optional[...], but instead have a boolean in TrueName.

This makes it much easier to match names directly and simplifies a lot of substitution logic within Name.
The changes here don't affect the codebase outside of name too much, because most of it was abstracted away.
But it does greatly reduce the chances of introducing a bug.
And it also simplifies the unification stage even further.

To accomodate these changes, we also revamp the generic system:

The context now also substitutes the generics within parents when performing a class lookup.
We perform matching when checking if a class if a class of another class.
Specifically, if the literal of the names are equal (checking the lits in StringName { lit1, generics: [Name] }), we check proceed to check for each generic whether self is name of other as long as the generic count is the same.

Added Tests

TODO: list tests

src/check/constrain/generate/call.rs

src/check/constrain/constraint/builder.rs

Remove the collection Expect construct. Instead, we use generics now and substitution of generics to deal with unification of collections. - Ignore collection type in output - Make temporary variables top-level Need to augment the unification rule for types a bit. If generics match and any generic is temporary, these must be substituted as well.

- __iter__() call, which gives iterator. We constrain this using a temporary type - __next__() call, whose return type is constrained to the collection type

Need to generalize how we convert mamba types to Python types in generate stage. In particular: - When encountering types in the wild - When encountering constructors in the wild And _only_ in those situations so we don't accidentally change identifiers with say class names.

JSAbrahams · 2023-01-10T08:25:14Z

Add issue that we should only override identifiers with Python type in:

types
constructor calls

Makes unification logic much simpler.

Now easier to check whether something callable or tuple. Also simply generate by not looking at type annotations, except for class arguments. We are now dependent on the check stage to annotate variables

When looking up classes in context, we convert generic parents to concrete parents (in the form of StringNames). So, Collection[Int] is a super of Set[Int]: - Set has a parent Collection[Int] - We check that Collection[Int] is super of itself: - The first part is equal to itself. We also see that it has the same amount of generics as itself. - We proceed to perform this now for each generic by matching them.

JSAbrahams · 2023-01-10T20:41:56Z

src/check/name/true_name/mod.rs

 }

 pub trait IsTemp {
    fn is_temp(&self) -> bool;
 }

+pub trait MatchTempName {
+    fn temp_map(&self, other: &StringName, mapping: HashMap<Name, Name>, pos: Position) -> TypeResult<HashMap<Name, Name>>;


Hm perhaps some of this logic could be re-used for match_name?
They are very similar, except for the if statement.
So if you can pass a predicate then they're the same basically.

Do this at the end perhaps when we have a build where all the tests pass again.
Will make it much easier to quickly verify the refactor.

src/check/name/mod.rs

src/check/context/clss/mod.rs

src/check/constrain/unify/finished.rs

src/check/constrain/unify/expression/mod.rs

src/check/constrain/generate/definition.rs

src/check/constrain/constraint/mod.rs

Also match identifier with any expression if no type present.

src/generate/convert/definition.rs

src/common/position.rs

src/check/name/mod.rs

src/check/context/clss/mod.rs

src/check/constrain/unify/ty.rs

src/check/constrain/generate/definition.rs

src/check/constrain/generate/collection.rs

src/check/constrain/unify/link.rs

src/check/constrain/unify/function.rs

JSAbrahams · 2023-01-11T18:01:03Z

src/check/constrain/generate/definition.rs

-        }
-    };
+    match (ty, expr) {
+        (Some(ty), Some(expr)) => {


Have we tested type and expr?
Should the logic below for Some(expr) not also be here?
Tuple literals are bound, so it should be fine, but perhaps just a small comment to make it clear so this doesn't appear like an oversight.

If it should be here, then better to have wildcard for ty and just have a small if let Some(...) = ... binding for ty in the body.

src/check/constrain/unify/link.rs

And remove unnecessary push_ty in unify_ty.

For tuples we make an exception by iterating over the elements and checking that they each define __str__(). Also add extra rule for self. In union, self is always equal to entity_name. Other arguments are left as-is.

Should restructure name such the Name can have nested unions.

codecov · 2023-01-12T15:43:58Z

Codecov Report

Merging #439 (cb8fb18) into develop (7d06ae2) will decrease coverage by 0.13%.
The diff coverage is 85.27%.

@@             Coverage Diff             @@
##           develop     #439      +/-   ##
===========================================
- Coverage    88.59%   88.46%   -0.14%     
===========================================
  Files          110      107       -3     
  Lines        11956    11729     -227     
===========================================
- Hits         10593    10376     -217     
+ Misses        1363     1353      -10

Impacted Files	Coverage Δ
src/check/constrain/constraint/iterator.rs	`84.61% <0.00%> (ø)`
src/check/constrain/unify/mod.rs	`75.00% <ø> (ø)`
src/check/context/field/mod.rs	`54.28% <ø> (ø)`
src/generate/convert/control_flow.rs	`90.25% <ø> (ø)`
src/parse/lex/result.rs	`14.28% <0.00%> (ø)`
src/parse/result.rs	`43.07% <0.00%> (ø)`
src/check/result.rs	`76.92% <33.33%> (+1.36%)`	⬆️
src/generate/convert/ty.rs	`57.14% <33.33%> (-12.25%)`	⬇️
src/check/context/parent/mod.rs	`42.85% <42.85%> (ø)`
src/check/context/arg/generic.rs	`76.54% <50.00%> (ø)`
... and 71 more

src/check/constrain/unify/function.rs

* Add substitution of names Remove the collection Expect construct. Instead, we use generics now and substitution of generics to deal with unification of collections. - Ignore collection type in output - Make temporary variables top-level Need to augment the unification rule for types a bit. If generics match and any generic is temporary, these must be substituted as well. * Match and substitute generic * Create two constraints for every col - __iter__() call, which gives iterator. We constrain this using a temporary type - __next__() call, whose return type is constrained to the collection type * Get rid of Generic NodeTy Need to generalize how we convert mamba types to Python types in generate stage. In particular: - When encountering types in the wild - When encountering constructors in the wild And _only_ in those situations so we don't accidentally change identifiers with say class names. * Add type annotation to non-nested builders * Get rid of tuple construct * Remove NameVariants * Simplify Name to Core in generation * Match generics when checking parent When looking up classes in context, we convert generic parents to concrete parents (in the form of StringNames). * Deal with variable generics for tuples * Rework identifier matching with ty, expr We added extra logic to deal with: - tuple literals - streamlining of tuples of identifiers Tuple matching still trips up when not matching with tuple literals. Perhaps we do need some logic in the unification stage, but this should be a last resort. * Name hash is deterministic * Recursively deconstruct builders * Reorder generation in lookup * Add extra __str__ rule to unify stage For tuples we make an exception by iterating over the elements and checking that they each define __str__(). Also add extra rule for self. In union, self is always equal to entity_name. Other arguments are left as-is. * Ensure nested Unions to_py deterministic * Remove unnecessary class and function union

JSAbrahams added the code quality: general Code quality issue not relevant to a particular module label Jan 9, 2023

JSAbrahams added this to the v0.3.6 | Dictionaries milestone Jan 9, 2023

JSAbrahams self-assigned this Jan 9, 2023

JSAbrahams mentioned this pull request Jan 9, 2023

Parsing and generation of dictionaries #438

Merged

JSAbrahams commented Jan 9, 2023

View reviewed changes

src/check/constrain/generate/call.rs Outdated Show resolved Hide resolved

JSAbrahams commented Jan 9, 2023

View reviewed changes

src/check/constrain/constraint/builder.rs Outdated Show resolved Hide resolved

JSAbrahams mentioned this pull request Jan 9, 2023

Remove unused ConstrVariant #440

Merged

JSAbrahams force-pushed the rework-collection branch from ad4e7e4 to d7eb8fe Compare January 9, 2023 16:08

JSAbrahams added 6 commits January 9, 2023 18:27

[ci skip] Match and substitute generics

bd7082f

[ci skip] Any StringName is always super

90d135d

[ci skip] Create two constraints for every col

cbe1add

- __iter__() call, which gives iterator. We constrain this using a temporary type - __next__() call, whose return type is constrained to the collection type

Flip around collection lookup constr generation

64d7029

[ci skip] Test set with multiple types

0c9f674

JSAbrahams added 6 commits January 10, 2023 14:04

Add type annotation to non-nested builders

6ef10db

Also get rid of tuple construct

6d534f0

Makes unification logic much simpler.

[ci skip] Remove NameVariants

e5d6cbd

Now easier to check whether something callable or tuple. Also simply generate by not looking at type annotations, except for class arguments. We are now dependent on the check stage to annotate variables

[ci skip] Simplify Name to Core in generation

7beaa09

[ci skip] Deal with variable generics for tuples

45d4cb3

JSAbrahams changed the title ~~Remove Expect::Collection~~ Rework Collections for Expect and Name Jan 10, 2023

JSAbrahams linked an issue Jan 10, 2023 that may be closed by this pull request

Constraint generation can be generalized to remove Expect::Collection construct #437

Closed

JSAbrahams commented Jan 10, 2023

View reviewed changes

src/check/name/mod.rs Outdated Show resolved Hide resolved

JSAbrahams commented Jan 10, 2023

View reviewed changes

src/check/context/clss/mod.rs Outdated Show resolved Hide resolved

JSAbrahams commented Jan 10, 2023

View reviewed changes

src/check/constrain/unify/finished.rs Show resolved Hide resolved

JSAbrahams commented Jan 10, 2023

View reviewed changes

src/check/constrain/unify/expression/mod.rs Outdated Show resolved Hide resolved

JSAbrahams commented Jan 10, 2023

View reviewed changes

src/check/constrain/generate/definition.rs Show resolved Hide resolved

JSAbrahams force-pushed the rework-collection branch from 89b6fe6 to ad2cbad Compare January 11, 2023 13:54