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

SI-7818 Cast our way out of extended existential angst #2916

Merged
merged 1 commit into from Sep 10, 2013

Conversation

retronym
Copy link
Member

@retronym retronym commented Sep 6, 2013

substituteSymbols is not sophisticated enough to
operate on TypeSkolem-s which are based on one of the
"from" symbols.

The pertinant usage of substituteSymbols for this bug in
in Extender. Recapping on that transform:

// orig
class C[T](...) extends AnyVal { def foo[U] = <rhs> }

// transform
class C[T] extends AnyVal { ... }
object C  { def foo$extension[T', U'] = <rhs'> }

Where <rhs'> has been subtituted with, among other things,
[T, U] ~> [T', U'].

In this case our expected type contains a new type parameter
(of the extension method), whereas the type of the RHS contains
an existential skolem still pinned to the corresponding class type
parameter.

tree.tpe         = Observable1#7037[_$1#12344]
<_$1#12344>.info =  <: T#7040
pt               = Observable1#7037[T#15644]

The limitation of substution is lamented in the comments
of adaptMismatchedSkolems, which faces the harder version of
the issue where the skolems are in the expected type.

But, we're in the "easy" case with the skolems in the tree's type;
we can cast our way out of the problem.

Review by @adriaanm, /cc @odersky

`substituteSymbols` is not sophisticated enough to
operate on `TypeSkolem`-s which are based on one of the
"from" symbols.

The pertinant usage of `substituteSymbols` for this bug in
in `Extender`. Recapping on that transform:

    // orig
    class C[T](...) extends AnyVal { def foo[U] = <rhs> }

    // transform
    class C[T] extends AnyVal { ... }
    object C  { def foo$extension[T', U'] = <rhs'> }

Where `<rhs'>` has been subtituted with, among other things,
`[T, U] ~> [T', U']`.

In this case our expected type contains a new type parameter
(of the extension method), whereas the type of the RHS contains
an existential skolem still pinned to the corresponding class type
parameter.

    tree.tpe         = Observable1#7037[_$1#12344]
    <_$1#12344>.info =  <: T#7040
    pt               = Observable1#7037[T#15644]

The limitation of substution is lamented in the comments
of `adaptMismatchedSkolems`, which faces the harder version of
the issue where the skolems are in the expected type.

But, we're in the "easy" case with the skolems in the tree's type;
we can cast our way out of the problem.

See also f335e44 / ed915c5.
@ghost ghost assigned adriaanm Sep 6, 2013
@adriaanm
Copy link
Contributor

adriaanm commented Sep 6, 2013

LGTM -- I had to pull similar stunts in the pattern matcher

@adriaanm
Copy link
Contributor

adriaanm commented Sep 6, 2013

What if we deskolemize and then substitute?

@retronym
Copy link
Member Author

retronym commented Sep 6, 2013

I'm not sure how to implement that.

I imagine the principled approach would run something like:

// traverse the LHS and find refs in types to local existential skolems
val existentials = for {
    tree <- LHS
    tp0 <- List(t.symbol.info, t.tpe) 
    TypeRef(_, sym, _) <- tp
    if sym.isExistentialSkolem && defines(tree, sym) // ala packedType
} yield sym

// clone and modify the info of those symbols to account for the new home in the
// extension method
// TODO what about the 'unpackLocation'?
val newSyms = deriveSymbols(existentials, _.substInfo(classTParams, methodTParams))
// substitute
tree.substitute(existentials, newSyms)

@retronym
Copy link
Member Author

retronym commented Sep 6, 2013

What do you know, it works: retronym/scala@scala:2.10.x...ticket/7818-subst-skolem

That might be worth pursuing further on master. Let's discuss it further over a coffee at BC next week :)

retronym added a commit that referenced this pull request Sep 10, 2013
SI-7818 Cast our way out of extended existential angst
@retronym retronym merged commit 1015d12 into scala:2.10.x Sep 10, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants