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

Claudio/cyclic bounds #586

Merged
merged 7 commits into from
Jul 25, 2019
Merged

Claudio/cyclic bounds #586

merged 7 commits into from
Jul 25, 2019

Conversation

crusso
Copy link
Contributor

@crusso crusso commented Jul 19, 2019

Reject cyclic bounds.

Instead of doing a top sort, we simply check that, given n type parameters, we can't construct a type parameter chasing subtype derivation of depth >= n.

We do the check both in typing.ml and check_ir.ml.

@crusso crusso requested a review from rossberg July 19, 2019 17:08
@crusso
Copy link
Contributor Author

crusso commented Jul 22, 2019

ping @rossberg

@@ -251,8 +251,29 @@ and check_typ_tag env typ_tag =
let t = check_typ env typ in
T.{lab = tag.it; typ = t}

and check_typ_binds_acyclic env typ_bind cs ts =
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
and check_typ_binds_acyclic env typ_bind cs ts =
and check_typ_binds_acyclic env typ_binds cs ts =

let n = List.length cs in
let ce = List.fold_left2
(fun ce c t -> T.ConEnv.add c t ce) T.ConEnv.empty cs ts in
let chase typ_bind c =
Copy link
Contributor

Choose a reason for hiding this comment

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

This really only needs typ_bind.at

Copy link
Contributor

Choose a reason for hiding this comment

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

(Scrap that, forgot to delete.)

let chase typ_bind c =
let rec chase i ts c' =
if i > n
then
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: put on previous line

and check_typ_binds_acyclic env typ_bind cs ts =
let n = List.length cs in
let ce = List.fold_left2
(fun ce c t -> T.ConEnv.add c t ce) T.ConEnv.empty cs ts in
Copy link
Contributor

Choose a reason for hiding this comment

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

You can avoid the extra lambda by using fold_right2.

(match t with
| T.Con (c'', []) ->
chase (i+1) (t::ts) c''
| _ -> ())
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this sufficient? I suspect that without beta reducing outside constructor applications it will not detect, e.g.,

type Id<T> = T;
func f<A <: Id<A>>() = {}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dooh. This is the check we had on the CLR but that doesn't have type functions. I'll add a reduction step and tests.

Copy link
Contributor

@rossberg rossberg left a comment

Choose a reason for hiding this comment

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

Looks good. Would there be an extended version of this trick to check that recursion between type aliases is well-founded -- and uniform?

@crusso
Copy link
Contributor Author

crusso commented Jul 25, 2019

Looks good. Would there be an extended version of this trick to check that recursion between type aliases is well-founded -- and uniform?

Dunno, that's an interesting question, something like, given k constructors, if you can unfold thos constructors down a path more than k times you've got a cyle - is that enough?

In any case, Andrew K seems to think the restriction described by him and Benjamin should be applicable to uniformity. Still on my (our) stack.

@crusso crusso merged commit 6488f6b into master Jul 25, 2019
@crusso crusso deleted the claudio/cyclic-bounds branch July 25, 2019 11:36
dfinity-bot added a commit that referenced this pull request Aug 19, 2023
## Changelog for motoko-base:
Branch: next-moc
Commits: [dfinity/motoko-base@591f5af2...5bbf04cf](dfinity/motoko-base@591f5af...5bbf04c)

* [`085468f7`](dfinity/motoko-base@085468f) Apply NaN canonicalization in base tests ([dfinity/motoko-base⁠#586](https://togithub.com/dfinity/motoko-base/issues/586))
mergify bot pushed a commit that referenced this pull request Aug 19, 2023
## Changelog for motoko-base:
Branch: next-moc
Commits: [dfinity/motoko-base@591f5af2...5bbf04cf](dfinity/motoko-base@591f5af...5bbf04c)

* [`085468f7`](dfinity/motoko-base@085468f) Apply NaN canonicalization in base tests ([dfinity/motoko-base⁠#586](https://togithub.com/dfinity/motoko-base/issues/586))
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.

2 participants