From f2f877c5c17f0f5a22faef1607f3bb7c5fa1157f Mon Sep 17 00:00:00 2001 From: Tom Grigg Date: Mon, 8 Mar 2021 16:09:05 -0800 Subject: [PATCH 1/3] Fix clear_out() not deleting files in cmdTests Shell wildcards are not expanded when quoted, and thus the contents of the temporary output directories were never being cleared. One of the tests in bootstrapCmdTests had to be fixed as a result, since it was relying on a TASTy file created by a previous test still existing in the $OUT directory after having been cleared. --- project/scripts/bootstrapCmdTests | 1 + project/scripts/cmdTestsCommon.inc.sh | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/project/scripts/bootstrapCmdTests b/project/scripts/bootstrapCmdTests index d4d7313d300c..4b40b6aa2651 100755 --- a/project/scripts/bootstrapCmdTests +++ b/project/scripts/bootstrapCmdTests @@ -37,6 +37,7 @@ clear_out "$OUT" # check that `scalac -from-tasty` compiles and `scala` runs it echo "testing ./bin/scalac -from-tasty and scala -classpath" clear_out "$OUT1" +./bin/scalac "$SOURCE" -d "$OUT" ./bin/scalac -from-tasty -d "$OUT1" "$OUT/$TASTY" ./bin/scala -classpath "$OUT1" "$MAIN" > "$tmp" test "$EXPECTED_OUTPUT" = "$(cat "$tmp")" diff --git a/project/scripts/cmdTestsCommon.inc.sh b/project/scripts/cmdTestsCommon.inc.sh index 1247481aec12..dc81a3e35b52 100644 --- a/project/scripts/cmdTestsCommon.inc.sh +++ b/project/scripts/cmdTestsCommon.inc.sh @@ -15,5 +15,5 @@ tmp=$(mktemp) clear_out() { local out="$1" - rm -rf "$out/*" + rm -rf "$out"/* } From f2937c3949c4656f1c2ade7af3d0437b1a0bb4cd Mon Sep 17 00:00:00 2001 From: Tom Grigg Date: Mon, 8 Mar 2021 15:22:45 -0800 Subject: [PATCH 2/3] Fix broken dotty.tools.io.Path#parent The previous implementation wrongly assumed that if `jpath.normalize.getParent` returns null, then `jpath` must be a root. However the null in this case really only means that no name elements remain after normalization and the call to `getParent`. Since redundant name elements such as "." and ".." may be removed by `normalize`, and `getParent` may simply remove the last name element, there are cases other than roots to consider, such as the current directory. Some examples of broken behavior prior this commit: scala> Path("./foo.txt").parent val res0: dotty.tools.io.Directory = ./foo.txt // should be Directory(.) scala> Path("foo.txt").parent val res1: dotty.tools.io.Directory = foo.txt // should be Directory(.) scala> Path(".").parent val res4: dotty.tools.io.Directory = . // should be Directory(..) The changes here are based in part on the Scala 2.13 implementation of scala.reflect.io.Path#parent Fixes #11644 --- compiler/src/dotty/tools/io/Path.scala | 10 ++++++++- compiler/test/dotty/tools/io/PathTest.scala | 25 +++++++++++++++++++++ project/scripts/bootstrapCmdTests | 6 +++++ tests/pos/i11644.scala | 1 + 4 files changed, 41 insertions(+), 1 deletion(-) create mode 100644 compiler/test/dotty/tools/io/PathTest.scala create mode 100644 tests/pos/i11644.scala diff --git a/compiler/src/dotty/tools/io/Path.scala b/compiler/src/dotty/tools/io/Path.scala index e632bf488ec9..2f52cf9430b1 100644 --- a/compiler/src/dotty/tools/io/Path.scala +++ b/compiler/src/dotty/tools/io/Path.scala @@ -127,7 +127,15 @@ class Path private[io] (val jpath: JPath) { * @return The path of the parent directory, or root if path is already root */ def parent: Directory = jpath.normalize.getParent match { - case null => Directory(jpath) + case null => + if (isAbsolute) // it should be a root + toDirectory + else if (segments.nonEmpty && segments.last == "..") + (this / "..").toDirectory + else path match { + case "" | "." => Directory("..") + case _ => Directory(".") + } case parent => Directory(parent) } def parents: List[Directory] = { diff --git a/compiler/test/dotty/tools/io/PathTest.scala b/compiler/test/dotty/tools/io/PathTest.scala new file mode 100644 index 000000000000..e2fa4505dd2c --- /dev/null +++ b/compiler/test/dotty/tools/io/PathTest.scala @@ -0,0 +1,25 @@ +package dotty.tools.io + +import org.junit.Assert._ +import org.junit.Test + +class PathTest { + // Ref https://github.com/lampepfl/dotty/issues/11644#issuecomment-792457275 + @Test def parent(): Unit = { + assertEquals(Directory(".."), Path("").parent) + assertEquals(Directory(".."), Path(".").parent) + assertEquals(Directory("..") / "..", Path("..").parent) + assertEquals(Directory("."), Path("foo.txt").parent) + assertEquals(Directory("."), (Path(".") / "foo.txt").parent) + assertEquals(Directory("."), (Path("bar") / ".").parent) + assertEquals(Directory("."), (Path("foo") / ".." / "." / "bar").parent) + assertEquals(Directory("baz") / "bar", (Path(".") / "baz" / "bar" / "foo.txt").parent) + + for (root <- Path.roots) { + val rootDir = Directory(root.path) + assertEquals(rootDir, root.parent) + assertEquals(rootDir, (root / "foo.txt").parent) + assertEquals(rootDir / "baz" / "bar", (root / "baz" / "bar" / "foo.txt").parent) + } + } +} diff --git a/project/scripts/bootstrapCmdTests b/project/scripts/bootstrapCmdTests index 4b40b6aa2651..7683c53f9e82 100755 --- a/project/scripts/bootstrapCmdTests +++ b/project/scripts/bootstrapCmdTests @@ -58,3 +58,9 @@ grep -qe "def main(args: scala.Array\[scala.Predef.String\]): scala.Unit =" "$tm echo "testing ./bin/scaladoc" clear_out "$OUT1" ./bin/scaladoc -project Hello -d "$OUT1" "$OUT/out.jar" + +# check compilation when the class/tasty files already exist in the current directory +echo "testing i11644" +cwd=$(pwd) +clear_out "$OUT" +(cd "$OUT" && "$cwd/bin/scalac" "$cwd/tests/pos/i11644.scala" && "$cwd/bin/scalac" "$cwd/tests/pos/i11644.scala") diff --git a/tests/pos/i11644.scala b/tests/pos/i11644.scala new file mode 100644 index 000000000000..3e0b75e49e37 --- /dev/null +++ b/tests/pos/i11644.scala @@ -0,0 +1 @@ +@main def hello = println("hello, world") From df1546add88a6f0a7c02609e2144ac9e73eddf44 Mon Sep 17 00:00:00 2001 From: Tom Grigg Date: Wed, 10 Mar 2021 19:09:05 -0800 Subject: [PATCH 3/3] Don't normalize paths in Path#parent Calling java.nio.file.Path#normalize may result in resolving to a different path than intended, as in the case where the given path contains a `..` element and the preceding name is symbolic link. --- compiler/src/dotty/tools/io/Path.scala | 38 +++++++++++----- compiler/test/dotty/tools/io/PathTest.scala | 50 +++++++++++++++------ 2 files changed, 64 insertions(+), 24 deletions(-) diff --git a/compiler/src/dotty/tools/io/Path.scala b/compiler/src/dotty/tools/io/Path.scala index 2f52cf9430b1..208b18078b7e 100644 --- a/compiler/src/dotty/tools/io/Path.scala +++ b/compiler/src/dotty/tools/io/Path.scala @@ -126,17 +126,33 @@ class Path private[io] (val jpath: JPath) { /** * @return The path of the parent directory, or root if path is already root */ - def parent: Directory = jpath.normalize.getParent match { - case null => - if (isAbsolute) // it should be a root - toDirectory - else if (segments.nonEmpty && segments.last == "..") - (this / "..").toDirectory - else path match { - case "" | "." => Directory("..") - case _ => Directory(".") - } - case parent => Directory(parent) + def parent: Directory = { + // We don't call JPath#normalize here because it may result in resolving + // to a different path than intended, such as when the given path contains + // a `..` component and the preceding name is a symbolic link. + // https://docs.oracle.com/javase/8/docs/api/java/nio/file/Path.html#normalize-- + // + // Paths ending with `..` or `.` are handled specially here as + // JPath#getParent wants to simply strip away that last element. + // For the `..` case, we should take care not to fall into the above trap + // as the preceding name may be a symbolic link. This leaves little choice + // (since we don't want to access the file system) but to return the given + // path with `..` appended. By contrast, a `.` as the final path element + // is always redundant and should be removed before computing the parent. + if path.isEmpty then + Directory("..") + else if jpath.endsWith("..") then + (this / "..").toDirectory + else if jpath.endsWith(".") then + jpath.getParent match // strip the trailing `.` element + case null => Directory("..") + case p => new Path(p).parent + else jpath.getParent match + case null => + if isAbsolute then toDirectory // it should be a root + else Directory(".") + case x => + Directory(x) } def parents: List[Directory] = { val p = parent diff --git a/compiler/test/dotty/tools/io/PathTest.scala b/compiler/test/dotty/tools/io/PathTest.scala index e2fa4505dd2c..731d29cee8e6 100644 --- a/compiler/test/dotty/tools/io/PathTest.scala +++ b/compiler/test/dotty/tools/io/PathTest.scala @@ -1,25 +1,49 @@ package dotty.tools.io -import org.junit.Assert._ import org.junit.Test class PathTest { // Ref https://github.com/lampepfl/dotty/issues/11644#issuecomment-792457275 @Test def parent(): Unit = { - assertEquals(Directory(".."), Path("").parent) - assertEquals(Directory(".."), Path(".").parent) - assertEquals(Directory("..") / "..", Path("..").parent) - assertEquals(Directory("."), Path("foo.txt").parent) - assertEquals(Directory("."), (Path(".") / "foo.txt").parent) - assertEquals(Directory("."), (Path("bar") / ".").parent) - assertEquals(Directory("."), (Path("foo") / ".." / "." / "bar").parent) - assertEquals(Directory("baz") / "bar", (Path(".") / "baz" / "bar" / "foo.txt").parent) + testParent(Path(""), Directory("..")) + testParent(Path("."), Directory("..")) + testParent(Path(".") / ".", Directory("..")) + testParent(Path(".."), Directory("..") / "..") + testParent(Path("..") / ".", Directory("..") / "..") + testParent(Path("..") / "..", Directory("..") / ".." / "..") + testParent(Path(".") / "..", + Directory("..") / "..", + Directory(".") / ".." / "..") + + testParent(Path("foo") / ".", Directory(".")) + testParent(Path("foo") / ".." / "bar", Directory("foo") / "..") + testParent(Path("foo") / ".." / "." / "bar", Directory("foo") / ".." / ".") + + testParent(Path("foo.txt"), Directory(".")) + testParent(Path(".") / "foo.txt", Directory(".")) + testParent(Path(".") / "baz" / "bar" / "foo.txt", + Directory(".") / "baz" / "bar", + Directory("baz") / "bar") for (root <- Path.roots) { - val rootDir = Directory(root.path) - assertEquals(rootDir, root.parent) - assertEquals(rootDir, (root / "foo.txt").parent) - assertEquals(rootDir / "baz" / "bar", (root / "baz" / "bar" / "foo.txt").parent) + testParent(root, root) + testParent(root / ".", root) + testParent(root / "..", root / ".." / "..") + testParent(root / "foo" / ".", root) + testParent(root / "foo.txt", root) + testParent(root / "baz" / "bar" / "foo.txt", root / "baz" / "bar") + testParent(root / "foo" / "bar" / "..", root / "foo" / "bar" / ".." / "..") } } + + /** The parent of a path may have multiple valid non-canonical representations. + * Here we test that the parent of the specified path is among a curated list + * of representations we consider to be valid. + */ + private def testParent(path: Path, expected: Path*): Unit = { + val actual = path.parent + val some = if (expected.length > 1) " one of" else "" + assert(expected.contains(actual), + s"""expected$some: ${expected.mkString("<",">, <",">")} but was: <$actual>""") + } }