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

Allow composite traits to be used as types #314

Merged
merged 1 commit into from
Feb 5, 2016
Merged

Allow composite traits to be used as types #314

merged 1 commit into from
Feb 5, 2016

Conversation

EliasC
Copy link
Contributor

@EliasC EliasC commented Dec 21, 2015

There was no support in the backend for using composite traits as types,
causing programs like the following to crash.

trait T
trait U

passive class C : T + U -- This is fine, not used as a type

class Main
  def main() : void
    new C : T + U -- This crashes the compiler

This commit adds proper backend support for types like T + U,
introducing the C type capability, which is a pointer to a struct that
contains only the self type that prefixes all passive classes. This
means that any reference to a passive class (trait and capability
references included) can be cast to a capability and used as such.

This commit also adds proper tracing support for trait types, which were
erroneously ignored previously during tracing. It should therefore now
be safe to have fields of trait type or to pass trait references between
active objects.

@albertnetymk
Copy link
Contributor

I think for capability_t, the struct and typedef could be merged.

With this commit, single trait type T could be considered as a special case of capability type, and much code related to TraitType could be cleaned up or removed. In addition, some defects may be revealed, I believe.

trait T {
  require f() : void
}

trait U

passive class C : T + U {
  def f() : void {
    print 3;
  }
}

class Main
  def main() : void
    let c = new C : T + U
    in c.f()

Should this compile?

@EliasC
Copy link
Contributor Author

EliasC commented Jan 7, 2016

Your example program should definitely compile! I will fix that, and will see if I can clean up code using TraitType as per your suggestion.

@EliasC
Copy link
Contributor Author

EliasC commented Jan 28, 2016

Fixed bug found by @albertnetymk as well as several others (and added a test case). Ready for review again!

@albertnetymk
Copy link
Contributor

Why introduce another variable inside trace function in ClassDecl.hs?

isArrayType is misspelled, not missed in hasResultType.

Why is hasSameKind recursive? I thought checking one level is enough, for subtypeOf is recursive?

The change of signature of findMethod in order to accommodate method calling on capa type seems not well thought out. I would propose to have another function to query for the type containing the called method. For sure, this is rather subjective.

Judging from the fact that isRefType t is turned into isRefTpye t || isCapabilityType t in multiple places, it may make sense to include capa in ref type category.

@EliasC
Copy link
Contributor Author

EliasC commented Feb 2, 2016

Why introduce another variable inside trace function in ClassDecl.hs?

The new function traceCapability gets simpler if it gets a variable since it needs fewer memory accesses, meaning traceVariable (which calls traceCapability) needs to get a variable instead of a field access.

isArrayType is misspelled, not missed in hasResultType.

Nice catch! Fixing that now.

Why is hasSameKind recursive? I thought checking one level is enough, for subtypeOf is recursive?

For the current usage, I agree that one level of checking would be enough (note that this is not part of this PR though). The question is if Just 42 and Just [1,2,3] really hasSameKind in the general case?

The change of signature of findMethod in order to accommodate method calling on capa type seems not well thought out. I would propose to have another function to query for the type containing the called method. For sure, this is rather subjective.

Subjective, but also right! Separated into findMethod and findMethodWithCalledType.

Judging from the fact that isRefType t is turned into isRefTpye t || isCapabilityType t in multiple places, it may make sense to include capa in ref type category.

Maybe, but then we would need to introduce an additional category for what is currently considered to be a refType, namely a type that can be connected to a single class or trait. I propose we leave this as a future refactoring.

@albertnetymk
Copy link
Contributor

@EliasC Could you resolve the conflict?

There was no support in the backend for using composite traits as types,
causing programs like the following to crash.
```
trait T
trait U

passive class C : T + U -- This is fine, not used as a type

class Main
  def main() : void
    new C : T + U -- This crashes the compiler
```

This commit adds proper backend support for types like `T + U`,
introducing the C type `capability`, which is a pointer to a struct that
contains only the self type that prefixes all passive classes. This
means that any reference to a passive class (trait and capability
references included) can be cast to a `capability` and used as such.

This commit also adds proper tracing support for trait types, which were
erroneously ignored previously during tracing. It should therefore now
be safe to have fields of trait type or to pass trait references between
active objects.
@EliasC
Copy link
Contributor Author

EliasC commented Feb 5, 2016

@albertnetymk: Rebased and resolved!

@albertnetymk
Copy link
Contributor

I don't think I got your point on traceCapability and traceCapability:

class B
class A
  b:B
class Main
  def main() : void {
    ();
  }

The trace function changes from:

void _enc__trace_A(void* p)
{
  _enc__active_A_t* this = p;
  pony_traceactor(((pony_actor_t*) this->_enc__field_b));
}

to:

void _enc__trace_A(void* p)
{
  _enc__active_A_t* this = p;
  _enc__active_B_t* _enc__field_b = this->_enc__field_b;
  pony_traceactor(((pony_actor_t*) _enc__field_b));
}

Seems to me unnecessary.

note that this is not part of this PR though

Well, it kind of depends on how one view it. isCapabilityType is moved from the first part, calling hasSameKind recursively, to the third part, returning True. Since hasSameKind is only called by subtypeOf, changing the first part to return True seems a more reasonable fix.

The question is if Just 42 and Just [1,2,3] really hasSameKind in the general case?

It doesn't make much to ask hasSameKind without mentioning subtypeOf, for it's only called by subtypeOf. Maybe this will not be case in the future, though.

@EliasC
Copy link
Contributor Author

EliasC commented Feb 5, 2016

Regarding traceCapability, the (subjectively) positive effects are only visible when tracing capability types. The following program

trait B
class A
  b:B
class Main
  def main() : void {
    ();
  }

generates the following trace-function

void _enc__trace_A(void* p)
{
  _enc__active_A_t* this = p;
  _enc__trait_B_t* _enc__field_b = this->_enc__field_b;
  pony_trace_fn _enc__trace__enc__field_b = ((capability_t*) _enc__field_b)->_enc__self_type->trace;
  _enc__trace__enc__field_b(_enc__field_b);
}

which avoids the the double memory access needed by the following line:

((capability_t*) _this->_enc__field_b)->_enc_self_type->trace(this->_enc__field_b);

changing the first part to return True seems a more reasonable fix

Sounds dangerous to me. The current fix only changes the behavior for capabilityTypes (which is what the commit promises) whereas your change affects the behavior for many types.

It doesn't make much to ask hasSameKind without mentioning subtypeOf, for it's only called by subtypeOf.

The fact that hasSameKind is not a function local to subtypeOf suggests that it should define its own behavior rather than depending on its usage from a function in a separate module.

@albertnetymk
Copy link
Contributor

OK, I see. I disagree on comments on hasSameKind, but it's subjective.

Would merge this in one hour, if no objections.

albertnetymk added a commit that referenced this pull request Feb 5, 2016
Allow composite traits to be used as types
@albertnetymk albertnetymk merged commit 469e63b into parapluu:development Feb 5, 2016
@albertnetymk albertnetymk deleted the fix/composite-trait-types branch February 5, 2016 09:49
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.

3 participants