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 type system logic for detecting if a type param can match itself. #2737

Closed
wants to merge 1 commit into from

Conversation

jemc
Copy link
Member

@jemc jemc commented Jun 4, 2018

This PR fixes type system logic for detecting if a type param can match itself.

For example, this snippet doesn't compile with current ponyc master, but should:

trait val T[A: T[A]]

class val C1 is T[C1]
class val C2 is T[C2]

primitive Foo[A: (T[A] val & (C1 | C2)) = U64]
  fun apply(a: (A | None)): Bool =>
    match a
    | let _: A => true
    else false
    end

actor Main
  new create(env: Env) =>
    env.out.print(Foo[C1](C1).string())
    env.out.print(Foo[C1](None).string())

Note that this breaks a few methods in Iter that are actually capability-unsafe, so we have to figure out what to do about that. I'm marking as "DO NOT MERGE" and "needs discussion during sync" so we can discuss this aspect.

This may be related to #2584, but I don't think it has the same problem as @Praetonus' solution in #2597. Note that my code example above doesn't have the issue of None being a possible subtype of A. As such, I don't think we have to wait for @Praetonus' RFC to merge this. Still, I'll ask @Praetonus to comment here if he can.

This may also be related to #2646, which had a runtime segfault in Iter.filter_stateful, one of the methods found to be unsafe by this type system fix.

@jemc jemc added needs discussion during sync do not merge This PR should not be merged at this time labels Jun 4, 2018
@Theodus
Copy link
Contributor

Theodus commented Oct 3, 2018

The filter_stateful, filter_map_stateful, filter, filter_map, skip_while, and take_while methods will need to have their return types changed from Iter[X]^ to Iter[X!]^. The current return types are unsafe and impossible to implement correctly on a generic Iter[A], since a predicate {(A): B} cannot be given the result of Iter[A].next() and the result of iter[A].next() cannot be stored without becoming A!.

@SeanTAllen
Copy link
Member

@jemc the example given now compiles with ponyc master. Do we still need this? Should I spend time getting this capable of passing CI and not have conflicts? Or should we close it?

@SeanTAllen
Copy link
Member

@jemc and I discussed, I will open a PR with just the regression test in test/libponyc/matchtype.cc added. If it passes CI as expected, then we will merge the new PR and close this. Otherwise, Joe and I regroup.

@SeanTAllen SeanTAllen self-assigned this Sep 25, 2020
SeanTAllen added a commit that referenced this pull request Sep 25, 2020
The bug that 2737 was meant to fix is no longer an issue however
we don't have a regression test for it. This PR takes the regression
test from @jemc's original PR and adds it on its own.
SeanTAllen added a commit that referenced this pull request Sep 29, 2020
The bug that 2737 was meant to fix is no longer an issue however
we don't have a regression test for it. This PR takes the regression
test from @jemc's original PR and adds it on its own.
@SeanTAllen SeanTAllen closed this Sep 29, 2020
@SeanTAllen SeanTAllen deleted the fix/typeparam-match-typeparam branch September 29, 2020 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge This PR should not be merged at this time
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants