Fix broken Scaladoc external Java references

[steven-barnes]

The problem was that PlainNioFile.underlyingSource was expanding "/modules/java.base" into a full path, where as the lookup in findExternalLink was using relative paths ("/modules/java.base.jmod")

Fixes scala/bug#11839

Supersedes #8647

[lrytz]

Is this a fix for scala/bug#11839?

[ashawley]

This change breaks the example REPL session Jason posted in #7782. I'm on Mac OSX and using Java 11.0.2.

$ java -version
java version "11.0.2" 2019-01-15 LTS
Java(TM) SE Runtime Environment 18.9 (build 11.0.2+9-LTS)
Java HotSpot(TM) 64-Bit Server VM 18.9 (build 11.0.2+9-LTS, mixed mode)
$ ./build/quick/bin/scala
Welcome to Scala 2.13.2-20200118-041642-d2ec0a5 (Java HotSpot(TM) 64-Bit Server VM, Java 11.0.2).

scala> :power

scala> val path = symbolOf[java.lang.String].associatedFile
val path: scala.reflect.io.AbstractFile = /modules/java.base/java/lang/String.class

scala> .underlyingSource
val res0: Option[scala.reflect.io.AbstractFile] = Some(/modules/java.base.jmod)

scala> .get.exists
val res1: Boolean = false

scala> :quit

$ ./build/quick/bin/scala -release 8
Welcome to Scala 2.13.2-20200118-041642-d2ec0a5 (Java HotSpot(TM) 64-Bit Server VM, Java 11.0.2).

scala> :power

scala> val path = symbolOf[java.lang.String].associatedFile
val path: scala.reflect.io.AbstractFile = /8/java/lang/String.sig

scala> .underlyingSource
val res0: Option[scala.reflect.io.AbstractFile] = Some(/Library/Java/JavaVirtualMachines/jdk-11.0.2.jdk/Contents/Home/lib/ct.sym)

scala> .get.exists
val res1: Boolean = true

[steven-barnes]

OK, I can take a look. Do we have integration tests? That would help immensely

[steven-barnes]

Is this a fix for scala/bug#11839?

Yes, sorry. I couldn't find instructions for linking

[dwijnand]

Is this a fix for scala/bug#11839?

Yes, sorry. I couldn't find instructions for linking

Edited the PR description - have a look.

OK, I can take a look. Do we have integration tests? That would help immensely

Does this need an "integration test"? I think it's just a smoke test on part of the API using a known given like java.lang.String. I think you could just write unit test for that.

[steven-barnes]

I'm a newbie and don't know how you do things. In standard testing ideology, underlyingSource would need to be mocked in a unit test. We need to test how underlyingSource interacts with the code in findExternalLink. I can certainly just bang it into a unit test if that is OK.

[ashawley]

Ideally, there would have been a test added in #7782. Unfortunately, there isn't any infrastructure for JDK11 tests, yet. It would be nice if there was a JDK11 section of the partest suite or something. Although, maybe it could be done in JUnit, too.

[dwijnand]

Ah, right. No, I guess I meant a "functional test" (so, without any mocking, sometimes some setting overrides, and without integrating with anything external and without any mocking). So within https://github.com/scala/scala/tree/2.13.x/test/junit/scala/tools/nsc somewhere, I would think.

[ashawley]

We're getting off-topic. Reviewing this fix requires someone who is an expert with both Java 11 module support and with Scaladoc.

[dwijnand]

Although, maybe it could be done in JUnit, too.

Given that behaviour should be true on all versions, I was thinking just with JUnit, and I think we must have the tests running on JDK 11 somewhere, right?

[ashawley]

Given that behaviour should be true on all versions

It isn't. For Java 8, it's rt.jar still. For Java 11, it's a module.

[dwijnand]

In either case symbolOf[java.lang.String].associatedFile.underlyingSource.get.exists should be true (which you discovered isn't the case with the current PR), right?

[ashawley]

Yes, but I only tested on Java 11, and I don't know a lot about #7782 or the check Jason wrote.

[steven-barnes]

In order to fix this problem, I will have to go back to using absolute paths, which causes problems elsewhere. I can hack findExternalLink to use absolute paths everywhere, but I suspect something else will break, and we have insufficient tests. I need that Java11 / Scaladoc expert to chime in here...

[retronym]

👋

Thanks for contributing a fix here. I'll see if I can help reconcile different uses of underlyingSource to help figure this out.

First of all, I was curious how Javadoc users deal with external links into Java SDK API docs. Looks like the Maven Javadoc plugin has is clever enough to automatically configure the base URL of the SDK docs and also ships with config files (example) that map packages (e.g. java.util.function to the defining JPMS module (e.g java.base). 💯 for usability, no manual mapping configuration needed by build tool users.

Okay, as to testing, we do have functional tests that exercise external links in Scaladoc, such as https://github.com/scala/scala/blob/2.13.x/test/scaladoc/run/t191.scala. That seems focused on linking to the Scala SDK docs. But it could be extended or duplicated to test linking to Java SDK docs.

Our CI builds currently do not run on a multiple versions of Java. We'll add Java 11 to the mix soon. Tests that are Java version dependent (ie, should only run on Java 9+, or have assert different things on different Java versions) can be usually be expressed by adding logic to the test. You can then at least run the test manually on Java 11 on your workstation, even though CI won't exercise those paths.

AbstractFile.underlyingSource was originally a way to navigate from a link to a JAR entry to the JAR itself. Java 9 changes things a bit here by removing rt.jar in favour of the virtual jrt:// virtual file system. Zinc (the Scala incremental compiler used in SBT and elsewhere) uses this map the references from compiled classed to JARs on the classpath. I figured the least-bad option for jrt:// backed classes was to have underlyingSource return the .jmod file -- if that was updated or removed (however unliikely that is for a JDK install) Zinc ought to detect the change.

But I didn't consider that this path would need to be exposed to Scaladoc users trying valiantly to configure external URL mappings. The prior answer of "/modules/java.base" was a bit nicer still somewhat confusing to Scaladoc users IMO as it looks like an on-disk path that doesn't actually exist.

How about we add a new Scaladoc option --jdk-api-doc-base that the user could configure to https://docs.oracle.com/en/java/javase/$N/docs/api. findExternalLink could then have a special case when it detects that a symbol is part of the JDK:

val isJDK = isChildOf(sym.associatedFile.underlyingSource, sys.props("java.home"))

It could then detect the JPMS module of the symbol (I can help with the details of this) and construct the correct URL from the base.

WDYT?

[steven-barnes]

You are the expert, it sounds reasonable, but I'm fuzzy on all the details. Thanks for pointing out the test, I overlooked that somehow when poking around in the tests dir.

[steven-barnes]

A few nitpicks; the existing scaladoc options seem to use single dash, rather than double. So should the option be: -jdk-api-doc-base?

The java API doc URL could theoretically be hardcoded; however the pattern above only works with Java version 11 and up:

https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/Throwable.html

For earlier Java versions, this seems to be correct:

https://docs.oracle.com/javase/8/docs/api/java/lang/Throwable.html

[ashawley]

The current proposal by Jason for -jdk-api-doc-base might fix links for the JDK, but not be a general solution for Java libraries that are distributed as modules. TheapiMappings setting in sbt is a Map[File,URL] and can support multiple mappings.

[SethTisue]

new PR is #8663 and is ready for review