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

Refcap violation via gencap-constrained type parameter. #1328

Closed
Praetonus opened this issue Oct 16, 2016 · 11 comments
Closed

Refcap violation via gencap-constrained type parameter. #1328

Praetonus opened this issue Oct 16, 2016 · 11 comments
Assignees
Labels
triggers release Major issue that when fixed, results in an "emergency" release

Comments

@Praetonus
Copy link
Member

class Foo
  var i: USize = 0
  fun ref boom() => i = 3

actor Main
  new create(env: Env) =>
    let a: Foo val = Foo
    env.out.print(a.i.string())
    bla[Foo val](a)
    env.out.print(a.i.string())

  fun bla[A: Foo #read](x: A) =>
    x.boom()

This compiles fine and prints 0 then 3. The val object was indeed modified.

@Praetonus Praetonus added bug: 1 - needs investigation triggers release Major issue that when fixed, results in an "emergency" release labels Oct 16, 2016
@jemc jemc changed the title Refcap violation Refcap violation via type parameterized method. Oct 16, 2016
@jemc jemc self-assigned this Oct 16, 2016
@jemc
Copy link
Member

jemc commented Oct 16, 2016

I'll do some investigation of this one.

@jemc jemc changed the title Refcap violation via type parameterized method. Refcap violation via gencap-constrained type parameter. Oct 16, 2016
@jemc
Copy link
Member

jemc commented Oct 16, 2016

FYI this works exactly the same way (broken) when using type parameters on a type instead of on a method (when fun bla is converted to primitive Bla fun apply):

class Foo
  var i: USize = 0
  fun ref boom() => i = 3

primitive Bla[A: Foo #read]
  fun apply(x: A) =>
    x.boom()

actor Main
  new create(env: Env) =>
    let a: Foo val = Foo
    env.out.print(a.i.string())
    Bla[Foo val](a)
    env.out.print(a.i.string())

@jemc
Copy link
Member

jemc commented Oct 16, 2016

The problem is somewhere in the logic for type params in type/subtype.h.

We need to figure out why the following is seen as true:

ast_t* sub = /*(typeparamref (id A) #read x)*/;
ast_t* sup = /*(typeparamref (id A) ref x)*/;
is_subtype(sub, sup, &info, opt); // should be false, but comes back as true

I'll look more into this later today.

@jemc
Copy link
Member

jemc commented Oct 17, 2016

The issue is that is_cap_sub_cap_bound is used, which sees #read as being a subtype of ref. So either calling is_cap_sub_cap_bound is not the right semantics for this situation (and we need to call a different method), or is_cap_sub_cap_bound is implemented with the wrong semantics and needs to be fixed.

@jemc
Copy link
Member

jemc commented Oct 17, 2016

Docs for is_cap_sub_cap_bound:

Check subtyping with the knowledge that the original bound rcap was the
same. This is used when checking if a typeparamref is a subtype of itself,
possibly with an altered rcap due to aliasing or viewpoint adaptation.

This doesn't seem correct in this situation, and we should use is_cap_sub_cap instead. However, more investigation is needed to be sure that we can change this situation to use is_cap_sub_cap, while still keeping is_cap_sub_cap_bound in use for situations where it is appropriate/correct.

@jemc
Copy link
Member

jemc commented Oct 17, 2016

Unassigning myself for now since I have knowledge-dumped what I have found, and I probably won't have time to work more on this in the next few days. Someone else can feel free to pick up where I left off if they wish, or I will come back to it later in the week.

@jemc
Copy link
Member

jemc commented Oct 18, 2016

I did take a little more of a look at this tonight, and I don't feel confident stepping in with a solution without @sylvanc dropping some type theory guidance on me first. I think the problem may be a bit more general than I first thought - requiring a careful approach to avoid making other mistakes in the solution.

@jemc
Copy link
Member

jemc commented Oct 19, 2016

The solution is to change the is_subtype call at reify.c:269 to be the only place where the is_cap_sub_cap_bound strategy is used (change it to call another "new" function). The is_cap_sub_cap_bound semantics are only correct for checking type parameter constraints during reification, and at all other times (for is_subtype itself), we need to use the "normal" strategy for comparing caps.

@jemc jemc self-assigned this Oct 19, 2016
@Praetonus
Copy link
Member Author

I've encountered another instance of this bug.

interface I
  fun ref foo() => None

class C
  fun ref foo() => None

actor Main
  new create(env: Env) =>
    foo[C tag](C)

  fun foo[A: I #any](a: A) =>
    a.foo()

Here, cap_aliasing treats #any! as #alias, and then is_cap_sub_cap_bound says that #alias is a subtype of ref.

@jemc jemc removed their assignment Nov 11, 2016
@jemc
Copy link
Member

jemc commented Nov 11, 2016

To give an update on this, the fix that was discussed earlier seemed not to do the trick.

My plan is to check in with @sylvanc next wednesday to discuss the issue in depth and find the appropriate approach. However, I've unassigned myself in case anyone else wants to take a shot between now and then.

Praetonus pushed a commit to Praetonus/ponyc that referenced this issue Dec 27, 2016
This fixes an assertion failure when chaining an #any receiver.

Because of other bugs in the implementation (namely ponylang#1328), non-tag
methods can currently be called on an #any receiver. Another assertion
has been added to prevent this case silently pass.
Praetonus pushed a commit to Praetonus/ponyc that referenced this issue Dec 27, 2016
This fixes an assertion failure when chaining an #any receiver.

Because of other bugs in the implementation (namely ponylang#1328), non-tag
methods can currently be called on an #any receiver. Another assertion
has been added to prevent this case to silently pass.
jemc pushed a commit that referenced this issue Dec 27, 2016
This fixes an assertion failure when chaining an #any receiver.

Because of other bugs in the implementation (namely #1328), non-tag
methods can currently be called on an #any receiver. Another assertion
has been added to prevent this case to silently pass.
@jemc
Copy link
Member

jemc commented Feb 2, 2017

We got another report of this in #1549. I'll paste the repro here, so that we have another test case to check the fix against:

trait Foo
  fun foo()

class Bar is Foo
  fun foo() => None

actor Main
    fun call_foo[X: Foo #any](x: X) =>
      x.foo() // This works, but probably shouldn't work

    new create(e: Env) =>
      let x : Bar tag = Bar
      call_foo[Bar tag](x)

      // This fails to type check as expected
      // x.foo()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triggers release Major issue that when fixed, results in an "emergency" release
Projects
None yet
Development

No branches or pull requests

4 participants