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

Make tuples be subtypes of empty interfaces (like Any). #1937

Merged
merged 2 commits into from
Jun 23, 2017

Conversation

jemc
Copy link
Member

@jemc jemc commented Jun 1, 2017

After this commit, a tuple is considered a subtype of Any ${cap}
if all of the elements of the tuple are subcaps of the cap.

This is a principle of least surprise issue, as it is surprising
that a type param constraint of Any #any does not behave the same
as an unconstrained type param, with the latter accepting tuple
type args, and the latter not doing so.

Previously, there was no way to allow tuples of any cardinality
as a type argument, while restricting what caps were allowed to
be in those tuples (such as for the concerns of aliasing or
sendability). For example, class List[A] in the collections
package allowed tuples as type args, but class List[A: Any #share]
in the collections/persistent package did not. Now they both
allow tuple type args, and the Any #share constrains the type arg
so that all tuple elements must be shareable (val or tag).

Resolves #1767.

@jemc jemc added do not merge This PR should not be merged at this time changelog - added Automatically add "Added" CHANGELOG entry on merge labels Jun 1, 2017
@jemc
Copy link
Member Author

jemc commented Jun 1, 2017

I've marked this as DO NOT MERGE because I still need to verify that the runtime will do the correct thing in boxing the tuple when it is assigned to an Any reference.

@Praetonus
Copy link
Member

The crash in the tests is caused by the reachability map not being aware of the new subtyping relationship between tuples and empty interfaces. The code generator then believes that generating the __is function for some tuple isn't needed because it doesn't see the supertype needing it.

The add_types_to_trait and add_traits_to_type functions in reach.c should be updated to account for tuple/interface subtyping. We should be careful to not add any method to a tuple when doing that (i.e. add_methods_to_type should either not be called on a tuple or not have any method to add). Since tuples are subtypes of empty interfaces only, we should be fine either way.

Copy link
Contributor

@plietar plietar left a comment

Choose a reason for hiding this comment

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

This currently crashes when the tuple contains non-single types (eg union, intersection or tuple)

class C
class D
primitive P
  fun apply(x: (C val | D val, C ref)) =>
    let x': Any box = consume x

I think we can make (T1, T2) a subtype of Any cap by checking if both T1 and T2 are subtypes of Any cap, and it would take care of recursively checking through the types to check the capability

child != NULL;
child = ast_sibling(child))
{
if(!is_sub_cap_and_eph(child, super, check_cap, errorf, opt))
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of is_sub_cap_and_eph, I would make this is_x_sub_x(child, super, ...)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea - makes sense.

@jemc jemc force-pushed the fix/tuple-sub-empty-interface branch from f66e0be to 1f298e9 Compare June 4, 2017 01:32
@jemc
Copy link
Member Author

jemc commented Jun 4, 2017

I've made the fix that @plietar pointed out, but @Praetonus, I'm a little confused on your point, so I was hoping you could give some more guidance.

I looked at the source and call sites of the add_types_to_trait, add_traits_to_type, and it seems like they're already being called for tuple appropriately for tuples, and relying on is_subtype to determine if they should take action, so now that is_subtype is changed to support the subtype relationship, I'd expect it to work. Maybe I'm missing some more subtle point here, though.

@Praetonus
Copy link
Member

@jemc You're right, that's not the problem.

I think that what's missing is a call to add_methods_to_type in the TK_TUPLETYPE case of add_types_to_trait.

@plietar
Copy link
Contributor

plietar commented Jun 6, 2017

The problem here I think is that an Any reference can now be a tuple, and thus in an identity comparison we need to generate a call to __is to compare contents.

However, since #1934, this method is generated only if it is reachable, and it will never be considered reachable for Any, hence the failed assertion.

Note that #1934 also had a bug which leads to a similar crash. In the following example, when marking the __is method on the top-level tuple type ((U8, U16) | None, U8) it
does not mark it on the inner type (U8, U16). When generating the former it will then fail to find the latter.

let x : (((U8, U16) | None, U8) | None) = None
let y : (((U8, U16) | None, U8) | None) = None
x is y

Unfortunately if we make tuples subtypes of Any, and we want identity of tuple to compare contents, then I think we need to generate a __is method for all tuple types, and allocate a vtable index for Any (and all thus for all types). The tests pass on this PR with #1934 reverted.

@Praetonus
Copy link
Member

I think you're right @plietar, the identity reachability isn't recursive when it should indeed be. I'll make a fix.

@jemc
Copy link
Member Author

jemc commented Jun 6, 2017

@Praetonus - what about this part of @plietar's comment?

Unfortunately if we make tuples subtypes of Any, and we want identity of tuple to compare contents, then I think we need to generate a __is method for all tuple types, and allocate a vtable index for Any (and all thus for all types).

My takeaway from that comment is that if we make the change that this PR was opened to make, then we can't also have the optimization from #1934. So, if that's the case, we should likely just revert #1934, or come up with another solution for the tuple problem, instead of implementing tuple-sub-Any.

@Praetonus
Copy link
Member

Praetonus commented Jun 6, 2017

I think we can have both tuple-sub-Any and the reachability optimisation. It won't be as efficient as before since Any now has more subtypes, but people who want performance shouldn't be using Any anyway.

@Praetonus
Copy link
Member

The bug should be fixed in #1947.

@SeanTAllen
Copy link
Member

@Praetonus what's the performance impact on Any usage? We have to use Any in our code do to some various other bits of fun in the type system that is more involved than this comment. What do you imagine the performance impact of this will be?

@Praetonus
Copy link
Member

There would only be an impact on is and digestof with Any operands, with an additional check to see whether the operand is a tuple. Based on the code, this should be a load, a bitwise operation and a conditional branching.

@jemc
Copy link
Member Author

jemc commented Jun 21, 2017

Once the other identity fixes get merged, I will bump the commit here so CI runs again, and merge this if it passes.

@jemc jemc removed the do not merge This PR should not be merged at this time label Jun 21, 2017
@SeanTAllen SeanTAllen mentioned this pull request Jun 21, 2017
@Praetonus
Copy link
Member

#1947 has been merged.

jemc added 2 commits June 22, 2017 12:44
After this commit, a tuple is considered a subtype of `Any ${cap}`
if all of the elements of the tuple are subcaps of the cap.

This is a principle of least surprise issue, as it is surprising
that a type param constraint of `Any #any` does not behave the same
as an unconstrained type param, with the latter accepting tuple
type args, and the latter not doing so.

Previously, there was no way to allow tuples of any cardinality
as a type argument, while restricting what caps were allowed to
be in those tuples (such as for the concerns of aliasing or
sendability). For example, `class List[A]` in the `collections`
package allowed tuples as type args, but `class List[A: Any #share]`
in the `collections/persistent` package did not. Now they both
allow tuple type args, and the `Any #share` constrains the type arg
so that all tuple elements must be shareable (`val` or `tag`).

Resolves #1767.
@jemc jemc force-pushed the fix/tuple-sub-empty-interface branch from 1f298e9 to 47a03b5 Compare June 22, 2017 19:44
@jemc
Copy link
Member Author

jemc commented Jun 22, 2017

I've rebased this PR for running on latest master. We'll see how CI goes this time.

@jemc
Copy link
Member Author

jemc commented Jun 23, 2017

CI is good - I'm merging it.

@jemc jemc merged commit 0af06e8 into master Jun 23, 2017
ponylang-main added a commit that referenced this pull request Jun 23, 2017
@jemc jemc deleted the fix/tuple-sub-empty-interface branch June 23, 2017 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - added Automatically add "Added" CHANGELOG entry on merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants