-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Update -Wconf:src to match Scala 2 behavior
#24772
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
base: main
Are you sure you want to change the base?
Update -Wconf:src to match Scala 2 behavior
#24772
Conversation
After blowing the dust off my Windows VM, I can confirm this is platform dependent. On Windows: scala> java.io.File("C:\\foo\\..\\bar\\f.txt").toPath.toAbsolutePath.normalize
val res0: java.nio.file.Path = C:\bar\f.txt
scala> java.io.File("C:\\foo\\..\\bar\\f.txt").toPath.toAbsolutePath.toUri.normalize.getRawPath
val res1: String = /C:/bar/f.txtOn macOS: scala> java.io.File("C:\\foo\\..\\bar\\f.txt").toPath.toAbsolutePath.normalize
val res0: java.nio.file.Path = /Users/luc/code/scala/scala13/sandbox/C:\foo\..\bar\f.txt
scala> java.io.File("C:\\foo\\..\\bar\\f.txt").toPath.toAbsolutePath.toUri.normalize.getRawPath
val res1: String = /Users/luc/code/scala/scala13/sandbox/C:%5Cfoo%5C..%5Cbar%5Cf.txtSo arguably the current implementation using |
Hmm, arguably, yes. So if you want, I could:
One other thing I noticed was that, unlike the Scala 2 implementation, the @Test def `Wconf src filter only matches entire directory path components`: Unit =
val path = Path("foobar/File.scala")
val result = wconfSrcFilterTest(
argsStr = "-Wconf:src=bar/.*\\.scala:s",
warning = diagnosticWarning(util.SourceFile(new PlainFile(path), "UTF-8"))
)
assertEquals(result, Right(reporting.Action.Warning))Note that the current tests reverse the [error] Test dotty.tools.dotc.config.ScalaSettingsTests.Wconf src filter only matches entire directory path components failed:
java.lang.AssertionError: expected:<Right(Silent)> but was:<Right(Warning)>, took 0.001 sec
[error] at dotty.tools.dotc.config.ScalaSettingsTests.Wconf src filter only matches entire directory path components(ScalaSettingsTests.scala:308)So the fourth option would be:
|
3b08f8a to
d1c809b
Compare
|
@lrytz I've restored the Once everything looks good, I can update the PR title and description and squash the commits accordingly. (And let me know if I should go back and touch up the Scala 2 implementation as well.) BTW, the one job failing at the moment appears to be due to a transient infrastructure failure. |
|
LGTM, but let's also update the flag help: scala3/compiler/src/dotty/tools/dotc/config/ScalaSettings.scala Lines 246 to 247 in 7acc426
Here's the Scala 2 version: https://github.com/scala/scala/blob/8073bce6dd787d96b627a05438ca9199961358b4/src/compiler/scala/tools/nsc/settings/Warnings.scala#L58-L62 It might have to wait for 2.10, @Gedochao? The change is that
That's not needed IMO. |
You mean 3.10. |
d1c809b to
857941d
Compare
Adds `^` and `$` anchors to the `-Wconf:src` regex to ensure it matches entire path segments by default. Mostly replicates the Scala 2 `src` filter anchoring logic except for the `rootDir` bit, which Scala 3 doesn't support: - https://github.com/mbland/scala/blob/v2.13.18/src/compiler/scala/tools/nsc/Reporting.scala#L862-L875 - https://docs.scala-lang.org/scala3/guides/migration/options-lookup.html Applies the new `anchored` function after parsing the `-Wconf:src` argument as a regex first. This guards against pathological cases of invalid patterns that may become valid after anchoring, such as `\` becoming `/\$`. (One of the existing test cases covers this specific invalid regex case.) Adds new test cases, including cases to validate the existing behavior of normalizing paths without resolving symlinks. This is to help ensure that the Scala 2 issue from scala/bug#13145 (which scala/scala#11192 resolves) doesn't ever appear. Also extracts the `diagnosticWarning`, `virtualFile`, and `plainFile` helper methods to reduce duplication between new and existing test cases.
857941d to
a3eaa64
Compare
-Wconf:src to match Scala 2 behavior
|
@lrytz I've updated the flag help (using most of the same text from Scala 2), squashed the commits with an updated message, updated the pull request title and description accordingly, and rebased the branch on the latest |
Adds
^and$anchors to the-Wconf:srcregex to ensure it matches entire path segments by default. Mostly replicates the Scala 2srcfilter anchoring logic except for therootDirbit, which Scala 3 doesn't support:Applies the new
anchoredfunction after parsing the-Wconf:srcargument as a regex first. This guards against pathological cases of invalid patterns that may become valid after anchoring, such as\becoming/\$. (One of the existing test cases covers this specific invalid regex case.)Adds new test cases, including cases to validate the existing behavior of normalizing paths without resolving symlinks. This is to help ensure that the Scala 2 issue from scala/bug#13145 (which scala/scala#11192 resolves) doesn't ever appear.
Also extracts the
diagnosticWarning,virtualFile, andplainFilehelper methods to reduce duplication between new and existing test cases.Fixes #24771. Review by @lrytz.