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

Add missing dealias in isContextFunctionRef #15742

Merged
merged 3 commits into from
Aug 1, 2022

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Jul 24, 2022

Fixes #15738

@@ -0,0 +1,16 @@
object Test {
trait Transaction
type Transactional[T] = (Transaction) ?=> T
Copy link

Choose a reason for hiding this comment

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

Maybe (t: Transaction) ? This test as-is already passes on 3.1.3

@odersky
Copy link
Contributor Author

odersky commented Jul 25, 2022

How I fixed it:

This was a single line fix, but it took a bit to find out what needed fixing. I first played around with the example extensively changing various elements and dropping non-essential code (like the summon statements). I then found out that the difference was whether the CFT was expressed as a simple function Transactional ?=> T or as a dependent function (t: Transactional) ?=> T. In the first case it worked, in the second it didn't. Furthermore, it also worked if the dependent function was written directly, without using an alias.

I saw that in the case where it worked the ff(x) application was immediately followed by an apply which passed the implicit Transaction. I used -Yshow-tree-ids to find out the node number of the apply and then -Ydebug-tree-with-id to find the place where it was created. Unfortunately, that was in a tree copier, meaning that the original node was created elsewhere. So as an extra step, I added

  .ensuring(t => t.uniqueId != 339, i"original tree was $tree")

to the tree copier expression that created the watched out for node with number 339. That gave me node 338, and watching for his node with -Ydebug-tree-with-id finally revealed the place in Typer.adaptNoArgs. There was a complex condition that guarded the code that generated the apply:

      if ((implicitFun || caseCompanion) &&
          !isApplyProto(pt) &&
          pt != AssignProto &&
          !ctx.mode.is(Mode.Pattern) &&
          !ctx.isAfterTyper &&
          !ctx.isInlineContext) {
        typr.println(i"insert apply on implicit $tree")
        val sel = untpd.Select(untpd.TypedSplice(tree), nme.apply).withAttachment(InsertedApply, ())

I then instrumented that code showing various parts of the condition and ran it with both the good and the bad definition of Transactional. It showed that isImplicitFun was different in one call. Passing to the definition of isImplicitFun I concentrated on isContextFunctionRef, which was defined as follows.

    def isContextFunctionRef(wtp: Type): Boolean = wtp match {
      case RefinedType(parent, nme.apply, _) =>
        isContextFunctionRef(parent) // apply refinements indicate a dependent CFT
      case _ =>
        val underlying = wtp.underlyingClassRef(refinementOK = false) // other refinements are not OK
        defn.isContextFunctionClass(underlying.classSymbol)
    }

So, two branches, one for the dependent case (which is represented as a refinement), the other for the simple case. But if the whole thing was wrapped in an alias it did not work anymore. The toplevel case is not a RefinedCase, so it will fall into the second branch, and that one will return false once it hits the refinement because of the refinementOK = false. Case solved.

ADDENDUM: In the end it turned out that the faulty method was not needed at all, since the functionality was already implemented correctly by Definitions#isContextFunctionType. b5bcb30. I am not sure how two slightly incompatible implementations of a method could have co-existed for so long. Definitely something to watch out for!

It turns out the faulty method should not even have existed since it was
trying to duplicate functionality defined elsewhere.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Assertion in Erasure.adaptToType
4 participants