Fix #2137: Create dummy companions for top-level objects without a real one #2139
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This reverts commit 57641b9. Using `Type#select` instead of `Type#member` turned out to not help at all in avoiding false dependencies, you can still get a symbol whose denotation does not reallyExists. A proper fix for this issue is contained in the next commit.
|
/cc @felixmulder My added special test will conflict with your PR, sorry! |
|
@felixmulder Actually scratch that, I don't need to do anything special to get the test to run correctly with partest. |
smarter
force-pushed
the
fix/false-companion
branch
from
dc50896 to
47a2593
Compare
… a real one Previously, we sometimes ended up forcing a companion class symbol from a previous run or from the classpath which lead to weird issues like in `false-companion`. Even if we end up not forcing such a symbol, its presence can still lead to issue: before this commit incremental compilation of `dotty-compiler-bootstrapped` was broken because we recorded a false dependency on the non-bootstrapped `dotty-compiler` jar. The added test is currently marked pending because it does not work with JUnit (which doesn't handle separate compilation), only partest. I didn't managed to get it to work right, and this won't be necessary once our testing framework is overhauled by scala#2125 anyway, so I'll just have to remember to enable this test afterwards.
smarter
force-pushed
the
fix/false-companion
branch
from
47a2593 to
df40ac6
Compare
|
While the test works fine with partest,I couldn't get it to even compile with JUnit, and making it do so seems complicated, so I'll just disable it until #2125 is in. |
|
What's the problem with junit? |
|
it will try to compile the four files together, but two of them define the same object, so it fails. I tried using the trick you used for |
|
@felixmulder http://dotty-ci.epfl.ch/lampepfl/dotty/1418/4 has been marked in progress for 21 hours, even though all tests finished successfully, any idea what's going on? This also seems to be happenning in http://dotty-ci.epfl.ch/lampepfl/dotty/1423/3 which succeeded a while ago but has been marked in progress for 2 hours now. Meanwhile, I'll push-force commits with a different timestamp to force the CI to rerun. |
The previous commit introduced two new usages of `denotNamed` which broke tests/run/t1987b because `denotNamed` indirectly calls `packageObj` which caches the package object, except that at this point the package object is not yet entered, so the cache was incorrect. We fix this by using `effectiveScope.lookup` instead since we only need to look into the current scope, this is also true for the two existing usage of `denotNamed` in `createCompanionLinks` so they were also replaced by `effectiveScope.lookup`.
smarter
force-pushed
the
fix/false-companion
branch
from
df40ac6 to
b641181
Compare
|
We should look over upgrading the CI to the latest version, but in the mean
time - the best solution is to ask Fabien to restart it if you can't fix it.
Cheers,
Felix
PS - you have SSH access to both machines, so you should also be able to
manually restart it :)
…On Fri, Mar 24, 2017, 17:50 Guillaume Martres ***@***.***> wrote:
@felixmulder <https://github.com/felixmulder>
http://dotty-ci.epfl.ch/lampepfl/dotty/1418/4 has been marked in progress
for 21 hours, even though all tests finished successfully, any idea what's
going on? This also seems to be happenning in
http://dotty-ci.epfl.ch/lampepfl/dotty/1423/3 which succeeded a while ago
but has been marked in progress for 2 hours now. Meanwhile, I'll push-force
commits with a different timestamp to force the CI to rerun.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2139 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABdYwRWobfVehS0F3dW-e9OROoxoy5ieks5ro_RtgaJpZM4MnCR3>
.
|
|
I was hoping there was some debug information we could recover to understand what's going on before restarting stuff |
|
You could read the docker logs for the image?
…On Fri, Mar 24, 2017, 18:24 Guillaume Martres ***@***.***> wrote:
I was hoping there was some debug information we could recover to
understand what's going on before restarting stuff
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2139 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABdYwesqBAaHW_zCObVgt9jdnGh0tcrpks5ro_wwgaJpZM4MnCR3>
.
|
|
How do I know what logs correspond to a specific test run? |
|
On vacation so: ¯\_(ツ)_/¯
…On Fri, Mar 24, 2017, 19:17 Guillaume Martres ***@***.***> wrote:
How do I know what logs correspond to a specific test run?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2139 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABdYwcqqKkx6tXQRahZf2RLrnxuXq5oLks5rpAingaJpZM4MnCR3>
.
|
|
Ping for review. |
DarkDimius
approved these changes
Mar 28, 2017
|
why would it conflict? |
|
It doesn't conflict currently so I think it's fine. |
smarter
added a commit
to dotty-staging/dotty
that referenced
this pull request
Mar 29, 2017
In scala#2139 I added code to create dummy companions for companion-less objects, but not for companion-less classes because I thought it wasn't needed. But it turns out that even if the classpath only has `Foo.class` and not `Foo$.class`, a module for `Foo` is entered by `initializeFromClassPath` when it calls `enterClassAndModule`, so we have to add dummy companions to classes. I don't have a test to illustrate this issue, but note that it fixes the incremental compilation bug demonstrated by https://github.com/dotty-staging/dotty/commits/incremental-compilation-bug. Note: I verified that adding a dummy companion to "class Foo" did not cause the backend to emit a Foo$.class, so this should have no visible impact.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, we sometimes ended up forcing a companion class symbol from
a previous run or from the classpath which lead to weird issues like in
tests/pos-special/false-companion. Even if we end up not forcing sucha symbol, its presence can still lead to issue: before this commit
incremental compilation of
dotty-compiler-bootstrappedwas brokenbecause we recorded a false dependency on the non-bootstrapped
dotty-compilerjar.