Skip to content

Keep export clause until first transform, use for incremental compilation #10182

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

Merged
merged 4 commits into from
Dec 3, 2020

Conversation

bishabosha
Copy link
Member

@bishabosha bishabosha commented Nov 5, 2020

not sure what else should cause enough indirection for this to fail

now:

  • Keep export clause trees until FirstTransform
  • Add Export trees to TASTy format and QuoteContext reflection API
  • Use Export trees to record wildcard export as dependencies by inheritance in incremental compilation.
  • Restrict wildcard export from a package prefix

@bishabosha bishabosha requested review from smarter and sjrd November 5, 2020 13:01
@bishabosha bishabosha changed the title add regression test for export class incremental compilation add regression test for export clause incremental compilation Nov 5, 2020
@bishabosha bishabosha force-pushed the test-incremental-exports branch from 70abd87 to 1e0d0e1 Compare November 5, 2020 13:02
Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pushed a commit showing how things are broken.

@smarter smarter force-pushed the test-incremental-exports branch from d05e455 to b66ea6d Compare November 5, 2020 14:38
@smarter
Copy link
Member

smarter commented Nov 5, 2020

To get this to work, ExtractDependencies will need to handle wildcard exports from a class in the same way it handles inheriting from that type: by recording a DependencyByInheritance, wildcard exports from a package can never be handled correctly and should be disallowed.

@bishabosha
Copy link
Member Author

Thanks, right so the best way to break stuff is to add a new definition

@bishabosha bishabosha marked this pull request as draft November 16, 2020 14:54
@bishabosha bishabosha force-pushed the test-incremental-exports branch from 6f7285b to 45d840b Compare November 16, 2020 15:35
@bishabosha bishabosha changed the title add regression test for export clause incremental compilation Keep export clause until first transform, use for incremental compilation Nov 16, 2020
@bishabosha bishabosha force-pushed the test-incremental-exports branch from 948017d to 02660c7 Compare November 17, 2020 14:59
@bishabosha bishabosha force-pushed the test-incremental-exports branch from 6fe121d to 353b508 Compare November 18, 2020 11:36
@bishabosha bishabosha marked this pull request as ready for review November 18, 2020 18:34
@bishabosha
Copy link
Member Author

I believe this is ready now

@bishabosha bishabosha force-pushed the test-incremental-exports branch from bd2b5dc to d0f20a9 Compare November 18, 2020 18:38
Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clean the history of this PR to remove some of the back-and-forth ? I think having everything in one commit would be fine.

@bishabosha bishabosha force-pushed the test-incremental-exports branch from d0f20a9 to 019de01 Compare November 18, 2020 18:55
@bishabosha bishabosha requested a review from smarter November 19, 2020 09:59
@bishabosha bishabosha force-pushed the test-incremental-exports branch 4 times, most recently from 9de8784 to 743a6b6 Compare November 30, 2020 12:20
@bishabosha bishabosha requested a review from smarter November 30, 2020 15:23
@bishabosha bishabosha assigned smarter and unassigned bishabosha Nov 30, 2020
Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

otherwise lgtm

Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, one more thing: we should disallow wildcard export from a package because (unless I'm mistaken) we can't do incremental compilation for that for the same reason we can't do it for imports: sbt/zinc#226

@bishabosha
Copy link
Member Author

bishabosha commented Dec 1, 2020

@smarter do you mean just check in ExtractDependencies for if the prefix is a package?, or reject it with an error in typer e.g.

@smarter
Copy link
Member

smarter commented Dec 1, 2020

In typer yes

@bishabosha bishabosha requested a review from smarter December 2, 2020 13:44
@bishabosha
Copy link
Member Author

added a error when wildcard exports come from a package prefix

Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise LGTM, thanks for working on this!

@bishabosha
Copy link
Member Author

@smarter I tested the following (commenting out the package prefix check) and confirmed that sbt/zinc#226 does prevent new members being seen:

// A.scala
export p1._
export p2._

class A {
  val b = new B()
}
// p1.scala
package p1

class B
// p2.scala
package p2

class Ignored // needed so that A.scala compiles

// If after p2 is compiled we add `class B` here, then `A.scala` is not recompiled
// class B 

@bishabosha bishabosha force-pushed the test-incremental-exports branch 3 times, most recently from 61aa323 to 74e9fa9 Compare December 3, 2020 15:06
Add Export trees to TASTy format and QuoteContext reflection API

Use Export trees to record wildcard export as dependencies
by inheritance in incremental compilation.

Motivation for these changes:
- enable semantic tools such as doc tools to inspect where
  exports occur
- when a class A exports members of (b: B) with a
  wildcard, there was no dependency recorded to trigger a
  recompile of A if B adds new members. Keeping the export
  trees in the dependencies phase lets us inspect for the
  wildcard case which has a different dependency
  relationship than named exports.
@bishabosha bishabosha force-pushed the test-incremental-exports branch from 74e9fa9 to 666739b Compare December 3, 2020 15:12
@bishabosha bishabosha force-pushed the test-incremental-exports branch from 1db0f94 to 2c6eeb2 Compare December 3, 2020 15:33
@bishabosha bishabosha merged commit 222fe0b into scala:master Dec 3, 2020
@bishabosha bishabosha deleted the test-incremental-exports branch December 3, 2020 21:51
@Kordyjan Kordyjan modified the milestones: 3.0.0-M3, 3.0.0 Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants