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

Traverse synthetic top-level members in API #572

Merged
merged 4 commits into from Aug 15, 2018

Conversation

@jvican
Copy link
Member

commented Aug 13, 2018

When a case class is defined with no companion, the companion is
synthesized by the compiler and every object creation of that case class
is proxied to the synthesized apply method of the newly synthesized
companion. From this perspective, if we have a case class A(x: Int)
and a use site A(1), ExtractUsedNames will extract the name to the
apply method in A(1) and mark it as used.

However, this is wrong. When the user changes the signature of the case
class, only the signature of the case class constructor changes and this
change is not propagated to the apply signature (ExtractAPI traverses
trees, and the synthesized module has no trees as it is added in namer).
Therefore, when we compare changed names in the old and new API, Zinc
concludes that only references to A;<init>; must be recompiled and
since the use site that contained A(1) had no such reference, then
it's ignored.

To fix this problem, we protect ourselves from this point of indirection
and extract the proper name of the case class constructor iff the
companion case class is indeed synthesized by the compiler. Note that
when the user defines object A alongside the definition of A, Zinc
does the right thing.

After some more investigation, the problem at hand is that we don't traverse
synthetic top-level trees in API and therefore we don't extract the
used names from them.

This was causing us to ignore any names in the synthetic companion and,
on top of that, also synthesized top-level members like package object
definitions! Now, thanks to removing the filtering out that was
happening in isTopLevel, our previous issue is fixed and with it also
a pending test checking that package objects are recognized when
introduced in a change packageobject-and-traits.

This commit also adds a checkRecompilations 2 in one of the tests
related to package objects to make sure that test is behaving correctly.

Note that this fixes sbt/sbt#4316 and also
fixes issues with default parameters in case classes.

@jvican

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2018

I propose @smarter as a reviewer. On an unrelated topic, @eed3si9n Can we give Guillaume merge rights to this repository so that he can review?

@jvican jvican force-pushed the scalacenter:topic/default-parameters branch from 7a6dfa8 to b7928f3 Aug 13, 2018
@eed3si9n

This comment has been minimized.

Copy link
Member

commented Aug 13, 2018

@jvican

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2018

Thank you Eugene 😄 Can you also have a look at the PR?

@jvican jvican requested a review from eed3si9n Aug 13, 2018
Copy link
Member

left a comment

The explanation makes sense, and I like the fact that the change itself is small change to bridge code.
LGTM

@eed3si9n

This comment has been minimized.

Copy link
Member

commented Aug 13, 2018

Why not target 1.2.x branch?

@jvican

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2018

Because then somebody has to back port it to 1.x, which is the main master branch, and PRs get spread out across different branches that I need to manage myself downstream (in scalacenter/zinc). I would prefer if we just kept everything to 1.x and then branch out before the 1.x.n release (and cherry-picking the changes that should make it or not).

@eed3si9n

This comment has been minimized.

Copy link
Member

commented Aug 13, 2018

Sounds good.

@smarter

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2018

ExtractAPI traverses
trees, and the synthesized module has no trees as it is added in namer

I don't understand this, namer is definitely going to create trees for the synthesized module, and this will be done before the extractapi phase happens, something is fishy here.

@jvican

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2018

I don't understand this, namer is definitely going to create trees for the synthesized module, and this will be done before the extractapi phase happens, something is fishy here.

You're right, I was misled by the fact that showCode wouldn't show me the synthesized code so I assumed namer was setting up completers working on synthesized trees in new compilation units.

The issue at hand here is that we're filtering out synthetic top level members in API.scala, and therefore the synthetic companion is ignored. It looks safer to remove the synthetic check there as it's more general than this fix.

    def isTopLevel(sym: Symbol): Boolean = {
      !ignoredSymbol(sym) &&
      sym.isStatic &&
      !sym.isImplClass &&
      !sym.hasFlag(Flags.SYNTHETIC) &&
      !sym.hasFlag(Flags.JAVA) &&
      !sym.isNestedClass
    }

It looks like the filtering out of synthetic top level members comes from the dawn of time, before the Zinc 1.0 algorithm, around nine years ago: sbt/sbt@2977fd4.

On top of this, it looks like removing that seems to make packageobjects-and-traits passing.

@jvican

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2018

I've updated the code with this fix, all tests are passing locally, module the pending one that is now passing. I've added a new commit instead of squashing it with the previous one because I believe this discussion will be useful to people in the future and it's better if we let git history reflect that.

@smarter I was not able to find any example where extracting used names from synthetic top-level members would be problematic. Let me know if you find something.

@eed3si9n @smarter Could you have a look again?

@jvican jvican changed the title Fix mismatch between apply methods and initializers Traverse synthetic top-level members in API Aug 14, 2018
@smarter

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2018

I don't think dotty filters synthetic stuff and that hasn't caused any problem as far as I know, but I haven't checked if your testcase passes currently in Dotty.

@jvican jvican force-pushed the scalacenter:topic/default-parameters branch from 9759ead to 4e0023c Aug 14, 2018
@jvican

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2018

I'm coming back to this soon. In the CI, packageobject-and-traits does not pass but in my computer it does. So I'm investigating what's happening (and it doesn't look related to cached compiler bridges).

jvican added 2 commits Aug 13, 2018
When a case class is defined with no companion, the companion is
synthesized by the compiler and every object creation of that case class
is proxied to the synthesized apply method of the newly synthesized
companion. From this perspective, if we have a `case class A(x: Int)`
and a use site `A(1)`, `ExtractUsedNames` will extract the name to the
apply method in `A(1)` and mark it as used.

However, this is wrong. When the user changes the signature of the case
class, only the signature of the case class constructor changes and this
change is not propagated to the apply signature (`ExtractAPI` traverses
trees, and the synthesized module has no trees as it is added in namer).
Therefore, when we compare changed names in the old and new API, Zinc
concludes that only references to `A;<init>;` must be recompiled and
since the use site that contained `A(1)` had no such reference, then
it's ignored.

To fix this problem, we protect ourselves from this point of indirection
and extract the proper name of the case class constructor iff the
companion case class is indeed synthesized by the compiler. Note that
when the user defines `object A` alongside the definition of `A`, Zinc
does the right thing.

Note that this fixes sbt/sbt#4316 and also
fixes issues with default parameters in case classes.
The previous commit was a good fix for the wrong problem. After some
more investigation, the problem at hand is that we don't traverse
synthetic top-level trees in `API` and therefore we don't extract the
used names from them.

This was causing us to ignore any names in the synthetic companion and,
on top of that, also synthesized top-level members like package object
definitions! Now, thanks to removing the filtering out that was
happening in `isTopLevel`, our previous issue is fixed and with it also
a pending test checking that package objects are recognized when
introduced in a change `packageobject-and-traits`.

This commit also adds a `checkRecompilations 2` in one of the tests
related to package objects to make sure that test is behaving correctly.
@jvican jvican force-pushed the scalacenter:topic/default-parameters branch 2 times, most recently from dce5f93 to ce1a38d Aug 14, 2018
jvican added 2 commits Aug 14, 2018
Fix the sources because of this bug
scala/bug#9427.  The test was for some reason
passing locally, thanks to these changes it's not anymore.

Fixing this and the other packageobjects tests is left as future work.
@jvican jvican force-pushed the scalacenter:topic/default-parameters branch from ce1a38d to 820b2ac Aug 15, 2018
@jvican jvican merged commit 746d474 into sbt:1.x Aug 15, 2018
1 check passed
1 check passed
continuous-integration/drone/pr the build was successful
Details
@eed3si9n eed3si9n added this to the 1.3.0 milestone Apr 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.