Skip to content

Fix #2186: Synchronize classpath handling with Scala 2.12 #2191

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 2 commits into from
Apr 11, 2017

Conversation

smarter
Copy link
Member

@smarter smarter commented Apr 4, 2017

This commit is a very crude port of the classpath handling as it exists
in the 2.12.x branch of scalac (hash: 232d95a198c94da0c6c8393624e83e9b9ac84e81),
this replaces the existing Classpath code that was adapted from scalac
years ago.

I'll update this commit with more details and proper attribution later,
for now I just want to run this on the CI.

@odersky I had to add a special case to PackageLoader to make sure that Scala2 impl classes (ending with $class) were not entered in scope, do you know how we avoided impl classes being in scope so far?

@smarter smarter force-pushed the sync-classpath-scalac branch 4 times, most recently from 36ae1a5 to bf3326c Compare April 5, 2017 00:50
@smarter
Copy link
Member Author

smarter commented Apr 5, 2017

@odersky I had to add a special case to PackageLoader to make sure that Scala2 impl classes (ending with $class) were not entered in scope, do you know how we avoided impl classes being in scope so far?

Responding to myself: this was handled by DefaultJavaContext#isValidName in the old classpath implementation, of course there is no equivalent in Scala 2.12 since implementation classes don't exist anymore, but we still have to deal with them as long as we maintain binary compat with Scala 2.11.

@odersky
Copy link
Contributor

odersky commented Apr 5, 2017

Right. I just figured out the same thing :-).

@smarter smarter force-pushed the sync-classpath-scalac branch from bf3326c to 6cd429d Compare April 9, 2017 20:23
@smarter
Copy link
Member Author

smarter commented Apr 9, 2017

This is pretty cool: dotty.tools.dotc.CompilationTests.compilePos is now twice as fast to complete, I think this is all due to the shared global ClassPath cache: https://github.com/dotty-staging/dotty/blob/6cd429d86bd88d3e7c4d49a62e0d46eef720517c/compiler/src/dotty/tools/dotc/classpath/ZipAndJarFileLookupFactory.scala#L14-L41

smarter added 2 commits April 11, 2017 16:04
This commit is a very crude port of the classpath handling as it exists
in the 2.12.x branch of scalac (hash: 232d95a198c94da0c6c8393624e83e9b9ac84e81),
this replaces the existing Classpath code that was adapted from scalac
years ago.

This code was written by Grzegorz Kossakowski, Michał Pociecha, Lukas
Rytz, Jason Zaugg and other scalac contributors, many thanks to them!

For more information on this implementation, see the description of the
PR that originally added it to scalac: scala/scala#4060

Changes made to the copied code to get it to compile with dotty:
- Rename scala.tools.nsc.util.ClassPath to dotty.tools.io.ClassPath
- Rename scala.tools.nsc.classpath.* to dotty.tools.dotc.classpath.*
- Replace "private[nsc]" by "private[dotty]"
- Changed `isClass` methods in FileUtils to skip Scala 2.11
  implementation classes (needed until we stop being retro-compatible with
  Scala 2.11)

I also copied PlainFile.scala from scalac to get access to
`PlainNioFile`.
`dir.listFiles` will return null if called on a directory that no
longer exists, somehow partest triggers that.
@smarter smarter force-pushed the sync-classpath-scalac branch from 6cd429d to 223f32b Compare April 11, 2017 14:05
@smarter smarter changed the title [WIP] Fix #2186: Synchronize classpath handling with Scala 2.12 Fix #2186: Synchronize classpath handling with Scala 2.12 Apr 11, 2017
@smarter smarter requested a review from odersky April 11, 2017 14:05
@smarter
Copy link
Member Author

smarter commented Apr 11, 2017

This should now be ready for review, all new files are copied with some small changes. See the main commit message for more details.

@lrytz, @retronym : Does this look sane to you? Any particular thing we should be aware of?

@odersky
Copy link
Contributor

odersky commented Apr 11, 2017

LGTM

@smarter smarter merged commit 65dc7ad into scala:master Apr 11, 2017
smarter added a commit to dotty-staging/dotty that referenced this pull request Apr 26, 2017
Since scala#2191, `dotty.tools.dotc.io.PlainFile` is no longer an alias
for  `scala.reflect.io.PlainFile` but its own class. However, the
backend still uses PlainFile from scala.reflect. This mess should soon
be fixed by getting rid of scala-reflect (scala#2271). Meanwhile, this commit
fixes ExtractDependencies to pattern match on the correct class, and
thus avoid missing dependencies.
smarter added a commit to dotty-staging/dotty that referenced this pull request Apr 26, 2017
Since scala#2191, `dotty.tools.dotc.io.PlainFile` is no longer an alias
for  `scala.reflect.io.PlainFile` but its own class. However, the
backend still uses PlainFile from scala.reflect. This mess should soon
be fixed by getting rid of scala-reflect (scala#2271). Meanwhile, this commit
fixes ExtractDependencies to pattern match on the correct class, and
thus avoid missing dependencies.
@DarkDimius
Copy link
Contributor

Related: scala/bug#10289

@smarter
Copy link
Member Author

smarter commented Apr 27, 2017

@DarkDimius Thanks for pointing me to that, I opened #2317 to keep track of it.

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.

3 participants