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

Fix #102: Better main class detection #287

Merged
merged 1 commit into from May 28, 2017

Conversation

Projects
None yet
3 participants
@smarter
Copy link
Contributor

commented Apr 6, 2017

Previously, the main class detection was handled by
https://github.com/sbt/zinc/blob/1.0/internal/zinc-apiinfo/src/main/scala/xsbt/api/Discovery.scala
which looks for a main method with the correct signature in the
extracted API. This is imperfect because it relies on ExtractAPI
dealiasing types (because Discovery will look for a main method with a
parameter type of java.lang.String and won't recognize
scala.Predef.String), dealiasing means that the extracted API looses
information and thus can lead to undercompilation.

This commit partially fixes this by adding a new callback to AnalysisCallback:

    void mainClass(File sourceFile, String className)

that is used to explicitly register main entry points. This way, tools
do not need to interpret the extracted API, this is much better since it
makes it easier for zinc to evolve the API representation.

This commit does not actually changes ExtractAPI to not dealias, this
can be done in a later PR.

Note that there is another usecase for xsbt.api.Discovery that this PR
does not replace: discovering tests. This is more complicated because
different test frameworks have different ways to discover tests. For
more information, grep for "Fingerprint" in https://github.com/sbt/sbt
and https://github.com/sbt/junit-interface

Note also that the added scripted test does not actually test that this
PR is correct since sbt itself needs to be updated to use this new API,
the branch at
https://github.com/smarter/sbt/commits/fix/main-class-detection does
this, but cannot be merged in sbt until a new zinc is published.

Fix #102

@eed3si9n eed3si9n added the ready label Apr 6, 2017

@jvican

This comment has been minimized.

Copy link
Member

commented Apr 6, 2017

This commit does not actually changes ExtractAPI to not dealias, this
can be done in a later PR.

I'm unsure this is possible at all. API detection needs dealiased types to work correctly.

@smarter I'll review this one, thanks for the PR.

@smarter

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2017

I'm unsure this is possible at all. API detection needs dealiased types to work correctly.

API detection of what?

@jvican

This comment has been minimized.

Copy link
Member

commented Apr 6, 2017

API detection.

ExtractAPI.scala.

@smarter

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2017

Yes, but why do you say that ExtractAPI needs it to work correctly? After #87 and #239 zinc should handle abstract types in the API correctly, so not expanding type aliases should work too.

@smarter smarter force-pushed the smarter:fix/main-class-detection branch 3 times, most recently from 81bec72 to 1ed5b11 Apr 6, 2017

@smarter

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2017

I fixed the added test to:

  • actually work (I forgot that zinc scripted tests cannot define commands in build.sbt anymore, you have to add them to IncHandler instead)
  • actually test zinc itself, no matter what sbt uses for main class discovery (I added a checkMainClasses command to IncHandler, that behaves similarly to existing check* commands in there and uses the internal API).

@smarter smarter force-pushed the smarter:fix/main-class-detection branch from 1ed5b11 to 80a2a13 Apr 6, 2017

@smarter

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2017

Ping for review.

@jvican

jvican approved these changes May 5, 2017

Copy link
Member

left a comment

LGTM. Excellent job!

It's true that we need to put the hooks (or provide the necessary information) to detect main classes for tests. I'll look into the best way to do so.

@@ -0,0 +1,4 @@
> compile

This comment has been minimized.

Copy link
@jvican

jvican May 5, 2017

Member

These tests look great!

@@ -94,6 +96,16 @@ object ClassToAPI {
val defsEmptyMembers = clsDef :: statDef :: Nil
cmap.memo(name) = defsEmptyMembers
cmap.allNonLocalClasses ++= defs

This comment has been minimized.

Copy link
@jvican

jvican May 5, 2017

Member

👍

@jvican

This comment has been minimized.

Copy link
Member

commented May 5, 2017

@smarter Sorry, but we merged a formatting PR that invalidates the contents of this one. Can you please update this PR?

@eed3si9n @dwijnand Can you have a quick look to this? I want to merge it. If you don't have time, tell me so and I'll merge it.

@eed3si9n
Copy link
Member

left a comment

This is imperfect because it relies on ExtractAPI
dealiasing types (because Discovery will look for a main method with a
parameter type of java.lang.String and won't recognize
scala.Predef.String), dealiasing means that the extracted API looses
information and thus can lead to undercompilation.

Is it possible to fix the root cause rather than adding special case for main class detection?

@smarter

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2017

Is it possible to fix the root cause rather than adding special case for main class detection?

The root cause is the use of Discovery, the fix is to not use Discovery and the same should be done for tests, and Discovery should be removed. There is no way to make Discovery understand all the semantics of Scala type system.

@smarter smarter force-pushed the smarter:fix/main-class-detection branch from 80a2a13 to 5a4cb09 May 26, 2017

Fix #102: Better main class detection
Previously, the main class detection was handled by
https://github.com/sbt/zinc/blob/1.0/internal/zinc-apiinfo/src/main/scala/xsbt/api/Discovery.scala
which looks for a main method with the correct signature in the
extracted API. This is imperfect because it relies on ExtractAPI
dealiasing types (because Discovery will look for a main method with a
parameter type of `java.lang.String` and won't recognize
`scala.Predef.String`), dealiasing means that the extracted API looses
information and thus can lead to undercompilation.

This commit partially fixes this by adding a new callback to AnalysisCallback:
    void mainClass(File sourceFile, String className)
that is used to explicitly register main entry points. This way, tools
do not need to interpret the extracted API, this is much better since it
makes it easier for zinc to evolve the API representation.

This commit does not actually changes ExtractAPI to not dealias, this
can be done in a later PR.

Note that there is another usecase for xsbt.api.Discovery that this PR
does not replace: discovering tests. This is more complicated because
different test frameworks have different ways to discover tests. For
more information, grep for "Fingerprint" in https://github.com/sbt/sbt
and https://github.com/sbt/junit-interface

@smarter smarter force-pushed the smarter:fix/main-class-detection branch from 5a4cb09 to f10c53c May 26, 2017

@smarter

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2017

Updated, phew! Hopefully this gets merged before the next major reformatting/refactoring of the codebase.

@eed3si9n eed3si9n merged commit d253375 into sbt:1.0 May 28, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@eed3si9n eed3si9n removed the ready label May 28, 2017

smarter added a commit to smarter/sbt that referenced this pull request Jul 10, 2017

smarter added a commit to smarter/sbt that referenced this pull request Jul 10, 2017

cunei pushed a commit to cunei/zinc that referenced this pull request Oct 25, 2017

Merge pull request sbt#287 from ceedubs/cogen-try
Use seed.next for Success in Cogen for Try
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.