SI-5954 Adds implementation restriction preventing companions in package... #1882
Conversation
|
Wouldn't the test be more straightforward as |
|
Started jenkins job pr-rangepos at https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/1350/ |
|
Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/2065/ |
|
jenkins job pr-rangepos: Failed - https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/1350/ |
|
jenkins job pr-scala-testsuite-linux-opt: Failed - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/2065/ |
|
@retronym Yup, didn't realize we had such things. |
|
Re-submitted as a single commit, with simpler test in neg. |
|
Oh, hey, the kitteh woke up and puked on my pr. (sigh) |
|
PLS REBUILD ALL |
|
Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/2070/ |
|
jenkins job pr-scala-testsuite-linux-opt: Failed - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/2070/ |
|
Started jenkins job pr-rangepos at https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/1356/ |
|
jenkins job pr-rangepos: Failed - https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/1356/ |
|
|
PLS REBUILD ALL |
|
Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/2071/ |
|
jenkins job pr-scala-testsuite-linux-opt: Failed - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/2071/ |
|
Started jenkins job pr-rangepos at https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/1357/ |
|
jenkins job pr-rangepos: Failed - https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/1357/ |
|
Looking at the tests that are failing, some are tests that were for bugs that duplicate this one except that the tests fail to actually test the problem. But other tests are for other tangential issues. They just happen to also fall afoul of the re-compile problem that this restriction is preventing. So my proposal is to add a compiler switch that disables the restriction being added in this PR and modify some of the failing tests to use that flag. That way we don't lose valuable tests that exercise functionality that will one-day actually be useful when the re-compile problem is solved. How say you @retronym, @paulp, @adriaanm, @gkossakowski ? |
|
Sounds like a reasonable stepping stone to this musketeer, so long as we leave a ticket open. |
|
PLS REBUILD ALL |
|
Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/2075/ |
|
jenkins job pr-scala-testsuite-linux-opt: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/2075/ |
|
Started jenkins job pr-rangepos at https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/1362/ |
|
jenkins job pr-rangepos: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/1362/ |
| @@ -0,0 +1,16 @@ | |||
| t5954.scala:36: error: implementation restriction: package object A cannot contain case class D. class D should be placed directly in package A instead. | |||
There was a problem hiding this comment.
Shouldn't ${m} after .(dot) be capitalized?
...package object A cannot contain case class D. Class D should ...
|
Hmm, this desperately needs squashing. |
Companion objects (and thus also case classes) in package objects caused an assert about an overloaded symbol when everything was compiled twice. It's a hairy problem that doesn't fit in 2.10.1. So this fix adds an implementation restriction. It also has a test to make sure the error messages are clean and reasonably friendly, and does not catch other things defined in package objects. The test includes a commented out test in case somebody thinks they've solved the underlying problem. A handful of tests were falling afoul of the new implementation restriction. I verified that they do in fact fail on second compile so they aren't false casualties. But they do test real things we'd like to work once the re-compile issue is fixed. So I added a -X flag to disable the implementation restriction and made all the tests accidentally clobbered by the restriction use that flag.
|
Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/2086/ |
|
jenkins job pr-scala-testsuite-linux-opt: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/2086/ |
|
Started jenkins job pr-rangepos at https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/1373/ |
|
jenkins job pr-rangepos: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/1373/ |
|
@retronym lets get this puppy put to bed |
|
🐶 Lucky! Go To Mattress! |
|
:squirrel:! (clarification: http://www.youtube.com/watch?v=bBWrMQVsuak) |
SI-5954 Adds implementation restriction preventing companions in package...
|
@JamesIry Apparently I was too slow to provide useful feedback. I'm not a big fan of introducing more compiler options but your balance seems to be around 0 so far so this is fine. Also, thanks for following style of other implementation restrictions and for providing very good comments in the code. This is an example of how we should roll in scala/scala. Also, https://issues.scala-lang.org/browse/SI-6109 is still open if you have apetite for something bigger :-) |
|
I'm leaving the associated bug open because this implementation restriction is pretty arbitrary from a user point of view and it would be nice to solve the underlying problem. |
We want 2.10.1 to be a drop-in replacement for 2.10.0, so we can't start warning where we weren't warning in 2.10.0. See SI-5954 (scala#1882, scala#2079) for when it was an implementation restriction, which was then weakened to a warning. It's now hidden behind -Ydebug.
... objects
Companion objects (and thus also case classes) in package objects caused an
assert about an overloaded symbol when everything was compiled twice. It's a
hairy problem that doesn't fit in 2.10.1. So this fix adds an implementation
restriction. It also has a test to make sure the error messages are clean
and reasonably friendly, and the test includes a commented out test in case
somebody thinks they've solved the underlying problem.
@review gkossakowski