partest: Enable separate compilation #1289

smarter · 2016-06-01T01:09:35Z

partest can separately compile files based on their suffix (_1, _2,
...), it turns out that this feature was never enabled in the dotty
version of partest and no one noticed (it prints warnings in
./tests/partest-generated/gen.log which no one reads).

Enabling this revealed some latent separate compilation bugs that will need to
be fixed in the next few commits.

There are four (edit: actually more than that) test failures after this commit: https://gist.github.com/smarter/5cfff93170dae8864b9920e0d0398b63

I don't know if I'll have time to look at all of them, so it would be nice if people could assign themselves (ping @DarkDimius, @felixmulder, @liufengyun, @odersky, @VladimirNik) to try to fix one of them and push it to this PR (you can edit this issue to assign yourself):

t1751 and t294

Assignee: @smarter
These are the same issue related to inference and Java annotations:

error: none of the overloaded alternatives of constructor SuiteClasses in class SuiteClasses with types
(value: Class[_]*)SuiteClasses
(value: Array[Class[_]])SuiteClasses
match arguments (Array[Nothing])
@SuiteClasses(Array())
^

colltest5
- Assignee: @smarter
- The value class StringOps has an overloaded method ++, we replace overloading by numbers in the corresponding extension methods ($extension0, $extension1, ...) to avoid dealing with overloading after Typer (I'm no longer a fan of that scheme that we copied from scalac and considered replacing it by overloading). It seems to have gone wrong here (we emitted $plus$plus$extension0(String, String) when compiling the first file but when compiling the second file we looked for $plus$plus$extension1(String, String):
```
java.lang.NoSuchMethodError: strawman.collections.CollectionStrawMan5$StringOps$.$plus$plus$extension1(Ljava/lang/String;Ljava/lang/String;)Ljava/lang/String;
```

t7859

Assignee: @VladimirNik
The getter of x in the value class A should not be private because we need to access it from separate compilation units to unbox the value class:

java.lang.AssertionError: assertion failed: private getter x in class A in /home/smarter/opt/dotty/tests/partest-generated/run/t7859/A_1.scala accessed from value a in /home/smarter/opt/dotty/tests/partest-generated/run/t7859/B_2.scala
    at scala.Predef$.assert(Predef.scala:165)
    at dotty.tools.dotc.transform.ExpandPrivate.ensurePrivateAccessible(ExpandPrivate.scala:75)

t1186, t1235, t1254, t1642
- Assignee: @smarter
- The classpath used for javac is incorrect
volatile
- Assignee: @smarter
- java filename should match public class name to make javac happy
t1263, t1745
- Assignee: ...
- Emitted classfiles do not contain type parameter information used by javac, see Emitted classfiles do not contain type parameter information used by javac #1303, moved to pending for now
colltest4
- Assignee: @felixmulder
- There is a bug in CollectionStrawMan4:
```
object ListBuffer extends IterableFactory[ListBuffer] {
  def fromIterable[B](coll: Iterable[B]): ListBuffer[B] = coll match {
    case pd @ View.Partitioned(partition: View.Partition[B]) =>
      partition.distribute(new ListBuffer[B]())
      pd.forced.get.asInstanceOf[ListBuffer[B]]
```
The cast is wrong, pd.forced.get returns a List, not a ListBuffer, this should be fixed both in ./tests/run/colltest4/CollectionStrawMan4_1.scala and ./src/strawman/collections/CollectionStrawMan4.scala, stacktrace: https://gist.github.com/smarter/5aab6857a20b0979b2909355171ab341
t0288
- Assignee: @odersky
- This one seems tricky: it's a stale symbol error on the second run of dotty, the symbol is test$Outer$Inner$$$outer which is weird because Outer.Inner is a java inner class so it doesn't have an $outer field, someone familiar with how we deal with outer accessors should have a look.

smarter · 2016-06-02T13:55:27Z

src/dotty/tools/dotc/transform/ExpandPrivate.scala

@@ -93,6 +102,8 @@ class ExpandPrivate extends MiniPhaseTransform with IdentityDenotTransformer { t
      case Apply(sel @ Select(_: Super, _), _)
      if sym.is(PrivateParamAccessor) && sel.symbol.is(ParamAccessor) && sym.name == sel.symbol.name =>
        sym.ensureNotPrivate.installAfter(thisTransform)
+      case _ if (isVCPrivateParamAccessor(sym)) =>


Just do:

case _ => if (isVC...)

Instead of two case _ cases

Fixed. Thank you!

felixmulder · 2016-06-02T18:53:06Z

@smarter: is this ready for review after the tests pass?

smarter · 2016-06-02T18:55:45Z

Turns out there's more issues than the three I listed above, so not yet

smarter · 2016-06-02T22:38:18Z

OK, after fixing some stuff, there are two remaining test failure, I've updated the PR description with the details.

smarter · 2016-06-02T23:12:04Z

/rebuild

smarter · 2016-06-03T12:45:06Z

tests/run/colltest4/CollectionStrawMan4_1.scala

@@ -217,7 +217,7 @@ object CollectionStrawMan4 {
    def fromIterable[B](coll: Iterable[B]): ListBuffer[B] = coll match {
      case pd @ View.Partitioned(partition: View.Partition[B]) =>
        partition.distribute(new ListBuffer[B]())
-        pd.forced.get.asInstanceOf[ListBuffer[B]]
+        new ListBuffer[B] ++= pd.forced.get.asInstanceOf[List[B]]


You don't even need the cast, pd.forced.get works, but it would be interesting to know if this is what @odersky intented or if there's a design flaw somewhere. Also please squash your commits related to this into one commit

The code looks correct to me. The cast seems to be a left-over from a
previous version.

On Fri, Jun 3, 2016 at 2:45 PM, Guillaume Martres [email protected]
wrote:

In tests/run/colltest4/CollectionStrawMan4_1.scala
#1289 (comment):

@@ -217,7 +217,7 @@ object CollectionStrawMan4 {
def fromIterable[B](coll: Iterable[B]): ListBuffer[B] = coll match {
case pd @ View.Partitioned(partition: View.Partition[B]) =>
partition.distribute(new ListBufferB)

pd.forced.get.asInstanceOf[ListBuffer[B]]

new ListBuffer[B] ++= pd.forced.get.asInstanceOf[List[B]]

You don't even need the cast, pd.forced.get works, but it would be
interesting to know if this is what @odersky https://github.com/odersky
intented or if there's a design flaw somewhere. Also please squash your
commits related to this into one commit

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/lampepfl/dotty/pull/1289/files/61ce7bb9be6a74d19b246e621f8fbc02581684e5..8f3b4bf6cc2927dc2b1d9e9515dd0451b5359233#r65700740,
or mute the thread
https://github.com/notifications/unsubscribe/AAwlVoqkdv3GnuHJWa4SDmvsTQq6NGcfks5qICHWgaJpZM4IrGUX
.

Prof. Martin Odersky
LAMP/IC, EPFL

odersky · 2016-07-27T15:50:32Z

/rebuild

partest can separately compile files based on their suffix (_1, _2, ...), it turns out that this feature was never enabled in the dotty version of partest and no one noticed (it prints warnings in ./tests/partest-generated/gen.log which no one reads), tests with *.java files should be compiled both with javac and dotty, but compiling with javac was also disabled. Enabling this revealed some latent bugs that will be fixed in the next few commits.

This is necessary to unbox the value class when accessing it from separate compilation units

This test failed before because strawman.collections.CollectionStrawMan5 is defined in two places: - src/strawman/collections/CollectionStrawMan5.scala - tests/run/colltest5/CollectionStrawMan5_1.scala The first will be compiled by scalac (unless the tests are run through a bootstrapped dotty) and the second will be compiled by dotty, the value class encoding of scalac and dotty are not binary compatible. This would not be a problem if we always used the `CollectionStrawMan5` coming from the partest output directory and ignored the one in the dotty sources, but which one gets picked depends on the classpath and whether compilation is joined or separate, see scala#1301. For now, it's safer and simpler to just avoid having tests which define a class that is also defined in the sources of dotty. Also, fix a bug in colltest4 where it was importing CollectionStrawMan5 instead of CollectionStrawMan4

This lead to inference failures when separately compiling t1751 and t294, this did not happen under joint compilation because JavaParser does not create the overloaded constructor

Some java tests require the scala-library to be present on the classpath, this fixes tests/pos/java-interop/{t1186, t1235, t1254, t1642}. Also correctly redirect the output of javac so that it will be displayed by partest --verbose

javac wants the public class name to match the filename.

Currently, the classfiles emitted by dotty do not contain the type parameters information that javac relies on. Fixing this is tracked by scala#1303.

Java's naming convention is different from Scala's.

odersky · 2016-07-27T17:32:49Z

Fixed issue with t0288 and rebased to latest master.

odersky · 2016-07-27T17:33:19Z

/rebuild

In this case, supertype is not stable and should not be cached.

odersky · 2016-07-27T21:47:49Z

@smarter Your commits LGTM. Would you like to review the rest?

smarter · 2016-07-27T21:58:19Z

src/dotty/tools/dotc/core/Types.scala

@@ -2645,6 +2645,9 @@ object Types {
      if (ctx.period != validSuper) {
        cachedSuper = tycon match {
          case tp: TypeLambda => defn.AnyType
+          case tp: TypeVar =>
+            // supertype not stable, since underlying might change


The supertype should be stable if tp.inst.exists

Good point. Fixed in latest commit.

smarter · 2016-07-28T06:26:53Z

LGTM, good to have this in!

smarter reviewed Jun 2, 2016
View reviewed changes

smarter mentioned this pull request Jun 2, 2016

Have a consistent classpath order between partest joined compilation, partest separate compilation, partest run tests, dotc and dotr #1301

Closed

smarter force-pushed the fix/partest-separate branch 2 times, most recently from c9dfeb2 to 27733bd Compare June 2, 2016 18:23

felixmulder added the In progress label Jun 2, 2016

felixmulder added the Needs attention label Jun 2, 2016

smarter force-pushed the fix/partest-separate branch from 27733bd to 3872ab8 Compare June 2, 2016 22:06

smarter mentioned this pull request Jun 2, 2016

Emitted classfiles do not contain type parameter information used by javac #1303

Closed

felixmulder assigned felixmulder and unassigned felixmulder Jun 3, 2016

smarter reviewed Jun 3, 2016
View reviewed changes

felixmulder force-pushed the fix/partest-separate branch 3 times, most recently from 906374f to 58607c1 Compare June 3, 2016 12:50

felixmulder assigned odersky Jun 9, 2016

smarter and others added 9 commits July 27, 2016 19:28

Make private accessor in value class not-private.

216511f

This is necessary to unbox the value class when accessing it from separate compilation units

Remove overloaded constructor for annotations

1fa136f

This lead to inference failures when separately compiling t1751 and t294, this did not happen under joint compilation because JavaParser does not create the overloaded constructor

partest: put more stuff on javac classpath

8c35854

Some java tests require the scala-library to be present on the classpath, this fixes tests/pos/java-interop/{t1186, t1235, t1254, t1642}. Also correctly redirect the output of javac so that it will be displayed by partest --verbose

pos/java-interop/volatile: Fix compilation with javac

301a956

javac wants the public class name to match the filename.

Move java tests relying on type parameters to pending

a452fa2

Currently, the classfiles emitted by dotty do not contain the type parameters information that javac relies on. Fixing this is tracked by scala#1303.

Fix colltest4: ListBuffer[A]#fromIterable had an incorrect cast

40efc35

Don't generate outer accessors for Java innner classes.

4b0cc8a

Java's naming convention is different from Scala's.

odersky force-pushed the fix/partest-separate branch from 58607c1 to 4b0cc8a Compare July 27, 2016 17:31

Fix HkApply#superType if type constructor is a TypeVar

16a688f

In this case, supertype is not stable and should not be cached.

odersky added Needs review and removed In progress labels Jul 27, 2016

smarter reviewed Jul 27, 2016
View reviewed changes

odersky mentioned this pull request Jul 27, 2016

Ensure that partest runs bootstrapped Dotty. #1115

Merged

Refine HKApply#superType

04e6d5e

dottybot added the reviewed label Jul 28, 2016

smarter merged commit 79e0fe0 into scala:master Jul 28, 2016

smarter removed Needs review labels Jul 28, 2016

allanrenucci deleted the fix/partest-separate branch December 14, 2017 19:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

partest: Enable separate compilation #1289

partest: Enable separate compilation #1289

Uh oh!

smarter commented Jun 1, 2016 •

edited by odersky

Loading

Uh oh!

smarter Jun 2, 2016

Uh oh!

VladimirNik Jun 2, 2016

Uh oh!

felixmulder commented Jun 2, 2016

Uh oh!

smarter commented Jun 2, 2016

Uh oh!

smarter commented Jun 2, 2016 •

edited

Loading

Uh oh!

smarter commented Jun 2, 2016

Uh oh!

smarter Jun 3, 2016

Uh oh!

odersky Jun 3, 2016

Uh oh!

odersky commented Jul 27, 2016

Uh oh!

odersky commented Jul 27, 2016

Uh oh!

odersky commented Jul 27, 2016

Uh oh!

odersky commented Jul 27, 2016

Uh oh!

smarter Jul 27, 2016

Uh oh!

odersky Jul 27, 2016

Uh oh!

smarter commented Jul 28, 2016

Uh oh!

Uh oh!

partest: Enable separate compilation #1289

partest: Enable separate compilation #1289

Uh oh!

Conversation

smarter commented Jun 1, 2016 • edited by odersky Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smarter Jun 2, 2016

Choose a reason for hiding this comment

Uh oh!

VladimirNik Jun 2, 2016

Choose a reason for hiding this comment

Uh oh!

felixmulder commented Jun 2, 2016

Uh oh!

smarter commented Jun 2, 2016

Uh oh!

smarter commented Jun 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smarter commented Jun 2, 2016

Uh oh!

smarter Jun 3, 2016

Choose a reason for hiding this comment

Uh oh!

odersky Jun 3, 2016

Choose a reason for hiding this comment

Uh oh!

odersky commented Jul 27, 2016

Uh oh!

odersky commented Jul 27, 2016

Uh oh!

odersky commented Jul 27, 2016

Uh oh!

odersky commented Jul 27, 2016

Uh oh!

smarter Jul 27, 2016

Choose a reason for hiding this comment

Uh oh!

odersky Jul 27, 2016

Choose a reason for hiding this comment

Uh oh!

smarter commented Jul 28, 2016

Uh oh!

Uh oh!

smarter commented Jun 1, 2016 •

edited by odersky

Loading

smarter commented Jun 2, 2016 •

edited

Loading