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

Unexpected desugaring to selectDynamic #6551

Closed
scabug opened this issue Oct 21, 2012 · 12 comments
Closed

Unexpected desugaring to selectDynamic #6551

scabug opened this issue Oct 21, 2012 · 12 comments
Assignees
Labels
Milestone

Comments

@scabug
Copy link

scabug commented Oct 21, 2012

import language.dynamics

object Test {
  def main(args: Array[String]) {
    class Lenser[T] extends Dynamic {
      def selectDynamic(propName: String) = ???
    }

    def lens[T] = new Lenser[T]

    val qq = lens[String]
  }
}
12:03 ~/Projects/aaa$ scalac Test.scala 
Test.scala:11: error: method selectDynamic: (propName: String)Nothing does not take type parameters.
error after rewriting to lens[T].selectDynamic[String]("apply")
possible cause: maybe a wrong Dynamic method signature?
    val qq = lens[String]
             ^
one error found
@scabug
Copy link
Author

scabug commented Oct 21, 2012

Imported From: https://issues.scala-lang.org/browse/SI-6551?orig=1
Reporter: @xeno-by
Affected Versions: 2.10.0

@scabug
Copy link
Author

scabug commented Oct 21, 2012

@retronym said (edited on Oct 21, 2012 10:26:05 AM UTC):
This regressed here, assigning to Nada:

scala/scala@dbe7ef#L0R1108

@scabug
Copy link
Author

scabug commented Oct 21, 2012

@xeno-by said:
Any idea why the apply is being inserted in the first place?

@scabug
Copy link
Author

scabug commented Oct 21, 2012

@retronym said (edited on Oct 21, 2012 11:29:58 AM UTC):
You could pin the blame on two spots:

The first would be addressed by this refinement to case (8) of adapt.

  *  (8) When in both EXPRmode and FUNmode, add apply method calls to values of object type.
diff --git a/src/compiler/scala/tools/nsc/typechecker/Typers.scala b/src/compiler/scala/tools/nsc/typechecker/Typers.scala
index f82786d..c139596 100644
--- a/src/compiler/scala/tools/nsc/typechecker/Typers.scala
+++ b/src/compiler/scala/tools/nsc/typechecker/Typers.scala
@@ -1118,6 +1118,7 @@ trait Typers extends Modes with Adaptations with Tags {
           else if (inAllModes(mode, EXPRmode | FUNmode) &&
             !tree.tpe.isInstanceOf[MethodType] &&
             !tree.tpe.isInstanceOf[OverloadedType] &&
+            !tree.tpe.isInstanceOf[PolyType] &&
             applyPossible)
             insertApply()
           else if (!context.undetparams.isEmpty && !inPolyMode(mode)) { // (9)

The second part, which is what triggered the regression, is in:

object dyna {
      import treeInfo.{isApplyDynamicName, DynamicUpdate, DynamicApplicationNamed}

      def acceptsApplyDynamic(tp: Type) = tp.typeSymbol isNonBottomSubClass DynamicClass
}
...

          def applyPossible = {
            def applyMeth = member(adaptToName(tree, nme.apply), nme.apply)
            dyna.acceptsApplyDynamic(tree.tpe) || (
              if ((mode & TAPPmode) != 0)
                tree.tpe.typeParams.isEmpty && applyMeth.filter(!_.tpe.typeParams.isEmpty) != NoSymbol
              else
                applyMeth.filter(_.tpe.paramSectionCount > 0) != NoSymbol
            )
          }

tp.typeSymbol isNonBottomSubClass DynamicClass holds in the case for the PolyType [T] => Lenser[T].

@scabug
Copy link
Author

scabug commented Oct 21, 2012

@namin said (edited on Oct 21, 2012 2:29:39 PM UTC):
Thanks for the report, Eugene, and the detailed investigation, Jason.
It seems the simplest fix is to just not do rewrites when we have a dynamic polytype:

             def applyMeth = member(adaptToName(tree, nme.apply), nme.apply)
-            dyna.acceptsApplyDynamic(tree.tpe) || (
+            (dyna.acceptsApplyDynamic(tree.tpe) && !tree.tpe.isInstanceOf[PolyType]) || (

namin/scala@namin:2.10.0-wip...namin:si-6551

What do you think, Jason? Also, should I send a pull request against 2.10.0-wip or 2.10.x?

Thanks.

@scabug
Copy link
Author

scabug commented Oct 21, 2012

@retronym said:
Don't you think that the check fits better beside the existing exclusions of MethodType and OverloadedType?

Submit it with a dose of optimism to 2.10.0-wip.

@scabug
Copy link
Author

scabug commented Oct 21, 2012

@namin said:
Ah, I see. It does fit better there. Would it have the same meaning?

@scabug
Copy link
Author

scabug commented Oct 21, 2012

@retronym said:
At the moment, seeing as applyPossible is only called from that spot.

@scabug
Copy link
Author

scabug commented Oct 22, 2012

@adriaanm said:
I think the check Jason proposes makes sense. I guess you can encounter a PolyType without being in POLYmode, so just checking that you're in EXPRmode is not enough.

@scabug
Copy link
Author

scabug commented Oct 22, 2012

@retronym said (edited on Oct 22, 2012 3:18:20 PM UTC):
Further discussion on the best way to phrase this check. Penny for your thoughts, Adriaan.

scala/scala#1516 (comment)

@scabug
Copy link
Author

scabug commented Nov 28, 2012

@paulp said:
scala/scala#1681

@scabug
Copy link
Author

scabug commented Dec 5, 2012

@retronym said:
scala/scala@edbcc64

@scabug scabug closed this as completed Dec 5, 2012
@scabug scabug added the typer label Apr 7, 2017
@scabug scabug added this to the 2.10.0 milestone Apr 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants