Fix for #22461 Empty ClassPath attribute in one or more classpath jars causes crash

[philwalk]

Change to how an empty or null jar manifest ClassPath: property is handled:

in dotty.tools.dotc.classpath.ClassPathFactory:

• classesInExpandedPath(...) returns and empty IndexedSeq rather than crash
• isJarOrZip returns false on a null reference rather than crash

in dotty.tools.dotc.classpath.FileUtils:

• createSourcePath fails with an error message on a null file parameter.

In the context of #22461, this causes an empty ClassPath: property to be treated the same as a mispelled or missing classpath entry, which are silently ignored, matching the behaviour of legacy scripts.

Closes #22461

[som-snytt]

I was interested in trying it out because this is more-or-less shared code with Scala 2. I did not yet reproduce.

I would not expect to handle a possible null in more than one place. Dotty is usually complaining to me that such-and-such might be null, I have to sprinkle | Null if I think that is desired.

[philwalk]

I would not expect to handle a possible null in more than one place.

The critical change is here consisting of a null check inserted after line 71.
EDIT:
As shown in the stacktrace of #22461, an empty String is considered to be a file by if Files.exists(path)

The other changes don't address this bug, and are optional, intended to provide a more to-the-point error message.

Wow, I just noticed I uninitentionally commited a file related to #22443. I will revert that and reduce other changes to the absolute minimum.

[philwalk]

The proposed changes are now reduced to the bare minimum necessary.
A hypothesis as to why the fix is necessary:

          if Files.exists(path)               // `path` is the empty String
          entry = AbstractFile.getFile(path)  // `entry` IS null (Windows only)

I will try to manually verify this

EDIT: the following script verifies the problem in Windows:

#!/usr/bin/env -S scalaQ
object AbstractFileTest {
  def main(args: Array[String]): Unit = {
    import dotty.tools.io.AbstractFile
    import java.nio.file.{Path, Paths, Files}
    val path = Paths.get("")
    printf("path [%s] exists : %s\n", path,  Files.exists(path))
    val entry = AbstractFile.getFile(path)
    printf("entry [%s]\n", entry)
  }
}

# ./abstractFileTest.sc
[warning] MainGenericRunner class is deprecated since Scala 3.5.0, and Scala CLI features will not work.
[warning] Please be sure to update to the Scala CLI launcher to use the new features.
[warning] Check the Scala 3.5.0 release notes to troubleshoot your installation.
path [] exists : true
entry [null]

EDIT:
So an alternate fix would be to replace ClassPathFactory line 71L

  if Files.exists(path)

with this:

  if path.toFile.exists

EDIT:
Although it would fix the problem, performance can be very bad when path doesn't exist, as I recall. If so, the best performance might be this:

  if path.trim.nonEmpty && Files.exists(path)

[philwalk]

The JVM does the same thing in WSL as in Windows, so the reason this bug is Windows-only must be due to what AbstractFile does with an empty String Path.

# WSL
$ wsl /opt/scala/bin/scala -e 'printf("%s\n", java.nio.file.Files.exists(java.nio.file.Paths.get("")))'
true

# Windows
$ /opt/scala/bin/scala -e 'printf("%s\n", java.nio.file.Files.exists(java.nio.file.Paths.get("")))'
true

[philwalk]

To my surprise, the problem exists also in WSL, at least implied by the result of the abstractFileTest.sc script.
Here's how I did the test in WSL:

cd ~/workspace/scala3
sbt dist/Universal/packageBin
export SCALA_HOME=`pwd -P`/dist/linux-x86_64/target/universal/stage
export PATH="$SCALA_HOME/bin:$PATH"

BTW, the universal distribution includes scala_legacy in the bin directory.
Here's the test script:

#!/usr/bin/env -S scala_legacy

object AbstractFileTest {
  def main(args: Array[String]): Unit = {
    import dotty.tools.io.AbstractFile
    import java.nio.file.{Path, Paths, Files}
    val path = Paths.get("")
    printf("path [%s] exists : %s\n", path,  Files.exists(path))
    val entry = AbstractFile.getFile(path)
    printf("entry [%s]\n", entry)
  }
}

and here's the output:

(base) philwalk@d5:~/workspace/scala3$ ./abstractFileTest.sc
[warning] MainGenericRunner class is deprecated since Scala 3.5.0, and Scala CLI features will not work.
[warning] Please be sure to update to the Scala CLI launcher to use the new features.
[warning] Check the Scala 3.5.0 release notes to troubleshoot your installation.
path [] exists : true
entry [null]

[som-snytt]

FYI the equivalent code on Scala 2 was changed to test !isDirectory instead of isFile. I even touched ScriptRunner! by coincidence.

scala/scala#6365

I see there was one ancient attempt to improve the code in dotty, but I understand the assumption was that it would be replaced altogether at some point. I assume no one would object to any change you deem useful.

I don't know whether this is useful information.

➜  cat af-test.scala

object AbstractFileTest {
  def main(args: Array[String]): Unit = {
    //import dotty.tools.io.AbstractFile
    import scala.reflect.io.{AbstractFile, Path}
    import java.nio.file.{Paths, Files}
    val path = Paths.get("")
    printf("path [%s] exists : %s\n", path,  Files.exists(path))
    val x = Path(path.toFile)
    val entry = AbstractFile.getFile(x)
    printf("entry [%s]\n", entry)
  }
}
➜  scala af-test.scala
path [] exists : true
entry []

[hamzaremmal]

@philwalk Can you please drop the first commit and adjust the second ? The first commit has nothing to do this the original issue, and reverting it's change will only cause conflicts with your other PR when merging.

[hamzaremmal]

I would suggest making the change here instead:

scala3/compiler/src/dotty/tools/io/Jar.scala

Line 53 in fb66f34

for (m <- manifest ; cp <- m.attrs.get(Name.CLASS_PATH)) yield cp

def classPathString: Option[String] =
  for (m <- manifest ; cp <- m.attrs.get(Name.CLASS_PATH) if !cp.isBlank()) yield cp

[philwalk]

@philwalk Can you please drop the first commit and adjust the second ? The first commit has nothing to do this the original issue, and reverting it's change will only cause conflicts with your other PR when merging.

Looking into it ...

[som-snytt]

I commented on the ticket that the empty attribute is out of spec, so I like the clever idea of pushing the filter down to Jar.

[philwalk]

@philwalk Can you please drop the first commit and adjust the second ? The first commit has nothing to do this the original issue, and reverting it's change will only cause conflicts with your other PR when merging.

It's sounds like I can revert both commits and make your suggested change to Jar.scala, does that sound right?

[hamzaremmal]

@philwalk Can you please drop the first commit and adjust the second ? The first commit has nothing to do this the original issue, and reverting it's change will only cause conflicts with your other PR when merging.

It's sounds like I can revert both commits and make your suggested change to Jar.scala, does that sound right?

Yes, just make sure to have a single commit please

[philwalk]

Also, just to be clear, this will revert the change to dist/libexec/common-shared, undoing a needed (but unrelated) fix, but with the intent of creating a new PR for that fix later?

[hamzaremmal]

Also, just to be clear, this will revert the change to dist/libexec/common-shared, undoing a needed (but unrelated) fix, but with the intent of creating a new PR for that fix later?

There is already a PR for that change: #22444