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

Constrained monomorphs #9549

Merged
merged 29 commits into from
Jun 10, 2020
Merged

Constrained monomorphs #9549

merged 29 commits into from
Jun 10, 2020

Conversation

Simn
Copy link
Member

@Simn Simn commented Jun 9, 2020

This PR adds support for constraints on monomorphs by introducing the tm_constraints field on tmono. When a monomorph is bound via Monomorph.bind, the compiler checks if the binding is valid in check_constraints.

There are currently two sources of constraints:

  1. Type parameters with constraints: These now all go through spawn_constrained_monos, which creates the monomorph and adds the constraints from type parameters, if exists.
  2. Field access: This replaces open anonymous structures. When a field is accessed on a monomorph that is either unconstrained or has the MOpenStructure flag, a structural constraint for that field is added to the monomorph.

Furthermore, if we have an unbound monomorph after typing a function, that monomorph is Monomorph.closed by using its constraints to find a type.

Some information about related issues:

For now, my goal here is to "not break anything". Afterwards, we can look into what further possibilities this opens. This includes:

  • An alternative to @:multiType
  • Removal of the Opened flag for structures
  • Additional constraints like array access

I'm not 100% confident in the changes made here, but we have to start somewhere.

| TAnon _ when List.mem MOpenStructure m.tm_constraints ->
(* If we assign an open structure monomorph to another structure, the semantics want us to merge the
fields. This is kinda weird, but that's how it has always worked. *)
constrain_to_type m None t;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we also check if anon's fields don't conflict with current constraints here?
I didn't find such a check down the constraint_to_type calls path, and Monomorph.bind is called from various places other than unification.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes I think a check is missing here. I think we can do this in classify_constraints, where we're building the fields map anyway. This should avoid any problems with too early unification.

Copy link
Member

Choose a reason for hiding this comment

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

Same question for other branches of this match

monos

let safe_mono_close ctx m p =
Copy link
Member

Choose a reason for hiding this comment

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

Return value of this function is never used.
It should be changed to unit probably. And Monomorph.close too.

@RealyUniqueName
Copy link
Member

I'm not 100% confident in the changes made here, but we have to start somewhere.

I 100% agree. Let's merge this and deal with the issues should they arise.

@Simn
Copy link
Member Author

Simn commented Jun 9, 2020

I'll deal with field constraint conflicts later. That requires some proper handling to avoid getting Unify_error exceptions thrown in places we don't want them. The problem seems pretty difficult to reproduce anyway.

@Simn Simn merged commit 0ddd26d into development Jun 10, 2020
Gama11 added a commit that referenced this pull request Jun 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants