Fix #2456: eliminate syntactic references to Java packages #2528

abeln · 2017-05-24T20:18:09Z

No description provided.

smarter · 2017-05-24T20:42:07Z

java.util.NoSuchElementException: key not found: class Int

That looks exactly like #2468

abeln · 2017-05-24T20:28:56Z

compiler/src/dotty/tools/dotc/transform/ElimJavaPackages.scala

@@ -0,0 +1,24 @@
+package dotty.tools.dotc.transform


@DarkDimius a couple of follow-up questions:

Adding this feels a bit hacky, because it's potentially masking bugs in subsequent phases if those subsequent phases accidentally pass a Java package to the backend.
Or, to rephrase the above: if a phase passes a java package as a value to the backend,
1.1 Is it their responsibility to not do that?
1.2 Or are we now saying that the compiler guarantees that no such references exist, past ElimJavaPackages?

Should we add some code that checks whether the invariant holds for all phases going forward? If so, does that code go in TreeChecker?

Adding this feels a bit hacky, because it's potentially masking bugs in subsequent phases if those subsequent phases accidentally pass a Java package to the backend.

Most of the phases in Dotty perform some transformation that adds a new invariant that should not be broken by future phases.

1.1 Is it their responsibility to not do that?
1.2 Or are we now saying that the compiler guarantees that no such references exist, past ElimJavaPackages?

It is their responsibility not to do that, as long as ElimJavaPackages is able to blame them :-)
This is why you should add a checkPostcondition method that will check that package references did not re-appear. TreeChecker will call this method during Ycheck.

@DarkDimius friendly ping

It's funny how now with the new github system we can have comments with wrong review dates. See reply in a comment above.

DarkDimius

Sorry, forgot to press the "submit" button.

DarkDimius · 2017-05-26T07:51:17Z

compiler/src/dotty/tools/dotc/transform/ElimJavaPackages.scala

@@ -0,0 +1,24 @@
+package dotty.tools.dotc.transform


Adding this feels a bit hacky, because it's potentially masking bugs in subsequent phases if those subsequent phases accidentally pass a Java package to the backend.

Most of the phases in Dotty perform some transformation that adds a new invariant that should not be broken by future phases.

1.1 Is it their responsibility to not do that?
1.2 Or are we now saying that the compiler guarantees that no such references exist, past ElimJavaPackages?

It is their responsibility not to do that, as long as ElimJavaPackages is able to blame them :-)
This is why you should add a checkPostcondition method that will check that package references did not re-appear. TreeChecker will call this method during Ycheck.

DarkDimius · 2017-05-26T07:51:19Z

compiler/src/dotty/tools/dotc/transform/ElimJavaPackages.scala

+  override def phaseName: String = "elimJavaPackages"
+
+  override def transformSelect(tree: tpd.Select)(implicit ctx: Context, info: TransformerInfo): tpd.Tree = {
+    tree.denot.info match {


DarkDimius · 2017-05-26T07:53:55Z

compiler/src/dotty/tools/dotc/transform/ElimJavaPackages.scala

+
+  override def transformSelect(tree: tpd.Select)(implicit ctx: Context, info: TransformerInfo): tpd.Tree = {
+    tree.denot.info match {
+      case myTpe@TypeRef(prefix, _) if prefix.termSymbol.flags.is(allOf(Package, JavaDefined)) =>


the overload that takes FlagConjunction does not use a value-class and is less efficient than the one that takes a FlagSet. It's better to do two separate tests here.

abeln · 2017-05-31T19:13:10Z

compiler/src/dotty/tools/dotc/transform/ElimJavaPackages.scala

+    }
+  }
+
+  override def checkPostCondition(tree: Tree)(implicit ctx: Context): Unit = {


@DarkDimius I added the postcondition, but is there a way to run all the tests with -Ycheck enabled? (Is that done by default in the CI?)

Both CI and local test suite runs by default with Ychecks enabled.
https://github.com/lampepfl/dotty/blob/master/compiler/test/dotc/tests.scala#L71

abeln · 2017-05-31T19:19:20Z

compiler/src/dotty/tools/dotc/transform/ElimJavaPackages.scala

+  * Eliminates syntactic references to Java packages, so that there's no chance
+  * they accidentally end up in the backend.
+  */
+class ElimJavaPackages extends MiniPhaseTransform {


Should I be extending DenotTransformer here? I'd think InfoTransformer doesn't apply, because the type of the tree is unchanged. But maybe IdentityDenotTransformer then?

InfoTransformer doesn't apply, because the type of the tree

DenotTransformer are required when you change "type of a name", i.e. Denotation.
E.g. if a method/field used to return one type, but now returns another. Or if it takes more agruments. Or if a field became a method. Or if new methods\fields\types\classes are introduced.

You don't need to extend DenotTransformer here if are rebuilding trees that won't affect typing of "names". This phase doesn't.

nicolasstucki · 2017-05-31T20:18:42Z

compiler/src/dotty/tools/dotc/transform/ElimJavaPackages.scala

+  /**
+    * Is the given tree a syntactic reference to a Java package?
+    */
+  private def isJavaPackage(tree: Tree)(implicit ctx: Context): Boolean = {


If you do private def isJavaPackage(tree: Select)... you will not need the first match.

You should do it in checkPostCondition where performance is not really needed.

Done. PTAL.

After typechecking, replace TypeRefs trees of the form Select(p, _) : tpe@TypeRef where p refers to a Java package, by Ident(tpe)

abeln · 2017-05-31T21:47:09Z

compiler/src/dotty/tools/dotc/transform/ElimJavaPackages.scala

+
+  override def transformSelect(tree: Select)(implicit ctx: Context, info: TransformerInfo): Tree = {
+    if (isJavaPackage(tree)) {
+      assert(tree.tpe.isInstanceOf[TypeRef], s"Expected tree with type TypeRef, but got ${tree.tpe.show}")


How performance-sensitive is the miniphase code? Should I get rid of the assertion (and the asInstanceOf call below) in favour of some code duplication?

Selects are one the the most common nodes, so transformSelects should be fast:
#510

isInstanceOf & asInstanceOf checks are very fast on HotSpot, the slower part is allocation of a lambda that is the second argument of assert.

Ok, so should I get rid of the assert, or are we good to go here?

abeln mentioned this pull request May 24, 2017

Teach Ycheck to spot references to Java packages #2456

Closed

abeln commented May 25, 2017

View reviewed changes

DarkDimius reviewed May 27, 2017

View reviewed changes

abeln force-pushed the java-packages branch from 6ef8037 to b911233 Compare May 31, 2017 19:11

abeln commented May 31, 2017

View reviewed changes

abeln changed the title ~~[WIP] Fix #2456: eliminate syntactic references to Java packages~~ Fix #2456: eliminate syntactic references to Java packages May 31, 2017

abeln force-pushed the java-packages branch from b911233 to 35a2e01 Compare May 31, 2017 19:17

abeln commented May 31, 2017

View reviewed changes

nicolasstucki reviewed May 31, 2017

View reviewed changes

Fix scala#2456: eliminate syntactic references to Java packages

434a7ee

After typechecking, replace TypeRefs trees of the form Select(p, _) : tpe@TypeRef where p refers to a Java package, by Ident(tpe)

abeln force-pushed the java-packages branch from 35a2e01 to 434a7ee Compare May 31, 2017 21:40

abeln commented May 31, 2017

View reviewed changes

DarkDimius approved these changes Jun 2, 2017

View reviewed changes

DarkDimius merged commit 025cfa5 into scala:master Jun 2, 2017

abeln deleted the java-packages branch June 7, 2017 22:31

Fix #2456: eliminate syntactic references to Java packages #2528

Fix #2456: eliminate syntactic references to Java packages #2528

Uh oh!

Conversation

abeln commented May 24, 2017

Uh oh!

smarter commented May 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DarkDimius left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!