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

Fix and simplify lub/glb #2360

Merged
merged 7 commits into from
Feb 17, 2021
Merged

Fix and simplify lub/glb #2360

merged 7 commits into from
Feb 17, 2021

Conversation

rossberg
Copy link
Contributor

@rossberg rossberg commented Feb 16, 2021

Refactor lub and glb into a single unified operation and a few fixes:

  • Handle abstract type bounds correctly by using promotion for the lub.
  • Handle glb between incompatible abstract types without creating a vacuous cycle.
  • Treat type field mismatches more orthogonally.
  • Perform equivalence/subtype tests earlier, which should yield nicer results.
  • Also prefer side that is named if there is a choice.
  • Add more extensive tests.

While here, also removed subtyping between second-class types like Mut and Typ and Any/Non.

Fixes #515 (up to other potential pretty printing changes).

@rossberg rossberg requested a review from crusso February 16, 2021 14:47
@dfinity-ci
Copy link

This PR does not affect the produced WebAssembly code.

@rossberg
Copy link
Contributor Author

With @crusso on vacation, anybody else wants to review this?

Copy link
Collaborator

@nomeata nomeata left a comment

Choose a reason for hiding this comment

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

Not 100% confident on everything, in particular not subtyping on functions with type bounds, so another review by @crusso might improve quality. But if you want to get this out, here is a LGTM

Comment on lines +1063 to +1064
| _, [] -> if rel == lubs then [] else fs1
| [], _ -> if rel == lubs then [] else fs2
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
| _, [] -> if rel == lubs then [] else fs1
| [], _ -> if rel == lubs then [] else fs2
| _, [] -> if rel == glbs then fs1 else []
| [], _ -> if rel == glbs then fs2 else []

More symmetric with both cons_if below, and with combine_tags

@crusso
Copy link
Contributor

crusso commented Feb 16, 2021

This seems fishy to me, though I don't think its new behaviour:

As expected:

> func (x:{var y: Nat}): {y:Nat} {x};
stdin:7.33-7.34: type error [M0096], expression of type
  {var y : Nat}
cannot produce expected type
  {y : Nat}

So a var field never subtypes a non-var field, as expected.

Except we do allow it if the super type field has type Any.

> func (x:{var y: Nat}): {y:Any} {x};
func : {var y : Nat} -> {y : Any} 

And thus also:

> if true (object {public var x = 1}) else (object {public let x =1 });
{x = 1} : {x : Any}

If mut types really are second class, should they be included in Any?

@rossberg
Copy link
Contributor Author

Yes, that matches what the subtype relation has always defined. We have discussed restricting it before. Originally the liberal rule was nice, because it made lubs/glbs fully defined and compositional. But with the advent of Typ, that's out the window anyway. So I went ahead and changed that as part of this PR, i.e., Non <: Mut <: Any is gone, and Mut and Typ are now handled analogously.

Also tweaked tests to prevent more liberal subtyping than intended.

Comment on lines 14 to 17
{x : Nat}
and
{var x : Nat}
{x : Any}
{}
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there some words missing between the last two types in this error message or has run.sh just stripped them out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is some artefact due to our test scripts. IIRC, @kritzcreek first ran into this a while ago and might be able to tell you more.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah it's one of the regexes in run.sh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you remember which one? I can't spot one that would cause this.

Copy link
Contributor

@kritzcreek kritzcreek Feb 17, 2021

Choose a reason for hiding this comment

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

It's matching at common supertype

with:
^ *at ' from here (I think):

grep -a -E -v '^Raised by|^Raised at|^Re-raised at|^Re-Raised at|^Called from|^ *at ' $1 |

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, not a real bug then

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, this annoyed me, so I tweaked the regexp to not misfire on the error message. Seems to work. :)

| Some fs -> Obj (s1, fs)
)
| Func (s1, c1, bs1, args1, res1), Func (s2, c2, bs2, args2, res2) when
(try Obj (s1, combine_fields rel lubs glbs tf1 tf2)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not too crazy about the use of exceptions for this - meant to be spec-like - can you not have combine_fields and (for arrays) a combine_element function that handles the special second class types, sans exceptions, and just assert on the second class types in combine?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I went back and forth on how to handle the partiality here. This way, its simplest and the structure of the function remains parallel to all others, especially sub, which is valuable. If we wanted a different structure, then that should apply everywhere and better be reflected in the type definition itself.

Not too worried about the spec aspect in this case -- these are algorithms that wouldn't appear in a spec. Their declarative spec is that of a lub/glb wrt the subtype relation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough.

Copy link
Contributor

@crusso crusso left a comment

Choose a reason for hiding this comment

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

LGTM

@rossberg
Copy link
Contributor Author

Thanks Claudio! Now please turn off this damn computer!

@rossberg rossberg added the automerge-squash When ready, merge (using squash) label Feb 17, 2021
@mergify mergify bot merged commit 90bf64b into master Feb 17, 2021
@mergify mergify bot deleted the lub branch February 17, 2021 11:09
@mergify mergify bot removed the automerge-squash When ready, merge (using squash) label Feb 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cleanups for lub/glb residual problems
5 participants