Report all warnings on Werror and fail at the end #18829

szymon-rd · 2023-11-02T15:55:20Z

[test_scala2_library_tasty]

szymon-rd · 2023-11-02T15:59:28Z

still tbd: massive refactor of tests, but I want to hear if anyone has something against this solution

nicolasstucki

We need to be able to check if a line emitted a warning or not in the neg tests.

One option would be to make the test framework detect the warnings and // warn tags to validate them as we do with // error.

Another option would be to make the test framework interpret fatal warnings as errors.

nicolasstucki · 2023-11-03T08:46:21Z

tests/neg-deep-subtype/1828.scala

@@ -2,10 +2,12 @@

 class Test {
  def remove[S](a: S | Int, f: Int => S):S = a match {
-    case a: S => a             // error
+    case a: S => a             // warn


This tag is not checked by the testing framework. This implies that this file in not testing anything. This implies that all tests that do not have a test file are broken.

We need to make sure that we check the position and number of warnings emitted under -Xfatal-warnings/-Werror. Our assumption when designing those tests was that we would check them as errors.

Dale recently added that feature, it should be working (at least it's in Vulpix in ParallelTesting). I will make sure it works.

Oh. Now I see. It won't test both

nicolasstucki · 2023-11-03T09:00:09Z

One option would be to make the test framework detect the warnings and // warn tags to validate them as we do with // error.

The drawback of this approach is that we might need to add more // warn in tests. This will be a bit more work on our side but would make tests more robust.

dwijnand · 2023-11-03T09:02:56Z

One option would be to make the test framework detect the warnings and // warn tags to validate them as we do with // error.

Move it to tests/warn/.

nicolasstucki · 2023-11-03T10:23:57Z

Move it to tests/warn/.

We have tests that check both warnings and errors at the same time.

szymon-rd · 2023-11-03T11:15:53Z

I can move most of them to warn. For the rest, I will maybe just create the output check files.

Outdated

szymon-rd · 2023-11-22T11:55:00Z

All the tests are passing now. There are a lot of changes, but most are just simple modifications done by scripts.

szymon-rd · 2023-11-22T13:50:55Z

Right now one failing was added, and this branch fails after rebase. But I will fix it after the review, as broken tests keep appearing on main and I will need to fix the ones that will appear before the review is finished.

nicolasstucki · 2023-12-01T09:53:58Z

compiler/src/dotty/tools/dotc/reporting/Reporter.scala

@@ -214,6 +209,10 @@ abstract class Reporter extends interfaces.ReporterResult {
  def incomplete(dia: Diagnostic)(using Context): Unit =
    incompleteHandler(dia, ctx)

+  def finalizeReporting()(using Context) = 
+    if (hasWarnings && ctx.settings.XfatalWarnings.value)
+      report(new Error("No warnings can be incurred under -Werror.", NoSourcePosition))


Suggested change

report(new Error("No warnings can be incurred under -Werror.", NoSourcePosition))

report(new Error("Unexpected warnings under -Werror.", NoSourcePosition))

nicolasstucki · 2023-12-01T09:57:03Z

compiler/test/dotty/tools/dotc/CompilationTests.scala

@@ -216,7 +216,10 @@ class CompilationTests {
  @Test def checkInitGlobal: Unit = {
    implicit val testGroup: TestGroup = TestGroup("checkInitGlobal")
    val options = defaultOptions.and("-Ysafe-init-global", "-Xfatal-warnings")
+    compileFilesInDir("tests/init-global/neg", options).checkExpectedErrors()


Suggested change

compileFilesInDir("tests/init-global/neg", options).checkExpectedErrors()

This one is repeated. Keep the one with the filter (2 lines bellow).

compileFilesInDir("tests/init-global/neg", options, FileFilter.exclude(TestSources.negInitGlobalScala2LibraryTastyBlacklisted)).checkExpectedErrors()

nicolasstucki · 2023-12-01T09:57:28Z

compiler/test/dotty/tools/dotc/CompilationTests.scala

@@ -216,7 +216,10 @@ class CompilationTests {
  @Test def checkInitGlobal: Unit = {
    implicit val testGroup: TestGroup = TestGroup("checkInitGlobal")
    val options = defaultOptions.and("-Ysafe-init-global", "-Xfatal-warnings")
+    compileFilesInDir("tests/init-global/neg", options).checkExpectedErrors()
+    compileFilesInDir("tests/init-global/pos", options).checkCompile()


Suggested change

compileFilesInDir("tests/init-global/pos", options).checkCompile()

same duplication

nicolasstucki · 2023-12-01T09:59:53Z

compiler/test/dotty/tools/dotc/CompilationTests.scala

    compileFilesInDir("tests/init-global/neg", options, FileFilter.exclude(TestSources.negInitGlobalScala2LibraryTastyBlacklisted)).checkExpectedErrors()
+    compileFilesInDir("tests/init-global/warn", defaultOptions.and("-Ysafe-init-global"), FileFilter.exclude(TestSources.negInitGlobalScala2LibraryTastyBlacklisted)).checkWarnings()


Suggested change

compileFilesInDir("tests/init-global/warn", defaultOptions.and("-Ysafe-init-global"), FileFilter.exclude(TestSources.negInitGlobalScala2LibraryTastyBlacklisted)).checkWarnings()

compileFilesInDir("tests/init-global/warn", defaultOptions.and("-Ysafe-init-global"), FileFilter.exclude(TestSources.warnInitGlobalScala2LibraryTastyBlacklisted)).checkWarnings()

Create the blacklist only if it is necessary. Otherwise remove the filter.

Suggested change

compileFilesInDir("tests/init-global/warn", defaultOptions.and("-Ysafe-init-global"), FileFilter.exclude(TestSources.negInitGlobalScala2LibraryTastyBlacklisted)).checkWarnings()

compileFilesInDir("tests/init-global/warn", defaultOptions.and("-Ysafe-init-global")).checkWarnings()

These will only be tested on lightly or with [test_scala2_library_tasty]. I will add this to your PR.

nicolasstucki · 2023-12-01T10:06:00Z

tests/init-global/neg/i12544b.scala


  def f(e: Enum): Enum = e

 @main def main(): Unit = println(Enum.b)
+
+// nopos-error: No warnings can be incurred under -Werror.


This test should probably be moved to tests/init-global/warn/i12544b.scala.

Same for other tests that only have warnings and no errors. We do not need to add checkfiles for these kinds of tests.

nicolasstucki · 2023-12-01T10:16:10Z

compiler/test/dotty/tools/vulpix/ParallelTesting.scala

@@ -776,7 +776,7 @@ trait ParallelTesting extends RunnerOrchestration { self =>
    end maybeFailureMessage

    def getWarnMapAndExpectedCount(files: Seq[JFile]): (HashMap[String, Integer], Int) =


If we do not have any // warn or // nopos-warn we should emit an error. Similar to the one for neg test:

https://github.com/lampepfl/dotty/blob/4d45087448d8e649d944b6136ceb406e001983df/compiler/test/dotty/tools/vulpix/ParallelTesting.scala#L893-L893

nicolasstucki · 2023-12-01T10:21:04Z

tests/warn/i9740.scala

    case TypedRecoveryCompleted =>
-}
+}


Dont remove the last \n of the files

nicolasstucki · 2023-12-01T10:23:03Z

tests/init-global/neg/context-sensitivity.scala

@@ -15,3 +15,5 @@ object A:
  val box1: Box = new Box(new C(5))
  val box2: Box = new Box(new D(10))
  val m: Int = box1.value.foo()
+
+// nopos-error: No warnings can be incurred under -Werror.


Don't forget to add the last \n of the file.

nicolasstucki · 2023-12-01T10:28:25Z

tests/init/warn/secondary-ctor4.scala

@@ -10,7 +10,7 @@ class M(x: Int) {
    class L1(x: Int) { val n: Int = 5 }

    class A(b: B, x: Int) {
-      println(d.n)
+    println(d.n)


Suggested change

println(d.n)

println(d.n)

nicolasstucki · 2023-12-01T10:28:42Z

tests/init/warn/simple1.scala

+class Foo {
+  val len = name.size
+  val name: String = "Jack"  // warn
+}


Suggested change

}

}

nicolasstucki · 2023-12-01T10:31:01Z

tests/warn/i16649-refutable.scala

  val '{ $a: Int } = x
  val '{ $b: Any } = x
  val '{ $c } = x
+


Suggested change

nicolasstucki

We can simplify the work needed to get this PR considerably if we split it in parts.

Add warn test infrastructure
Migrate neg tests into warn tests (in batches)
Change warning/error behavior

szymon-rd · 2023-12-01T14:51:24Z

Good idea. I will split it in a couple of PRs.

Moves first batch of tests. PR 1/5 (merge consecutively, per [Nicolas' suggestion](#18829 (review)) Split up version of #18829

…Batch 1 (#19242) Adds functionality to vulpix that allows putting nopos-warns in comments & First batch of tests moved from tests/neg to tests/warn. PR 2/5 (merge consecutively, per [Nicolas' suggestion](#18829 (review)) Split up version of #18829

The second batch of tests moved from tests/neg to tests/warn. PR 3/5 (merge consecutively, per [Nicolas' suggestion](#18829 (review)) Split up version of #18829

The last batch of tests, move tests other than tests/init/neg and tests/neg to warn. PR 4/5 (merge consecutively, per [Nicolas' suggestion](#18829 (review)) Split up version of #18829

) Final PR. Adds functionality that changes the behaviour of fatal-warnings - fixes #18634 PR 5/5 (merge consecutively, per [Nicolas' suggestion](#18829 (review)) Split up version of #18829

szymon-rd force-pushed the report-all-warns-werror branch from 26bc4ba to 4aa8e40 Compare November 2, 2023 15:56

nicolasstucki previously requested changes Nov 3, 2023

View reviewed changes

som-snytt mentioned this pull request Nov 5, 2023

-Wconf:msg doesn't work as in scala2 #18804

Open

szymon-rd force-pushed the report-all-warns-werror branch from fe5a5e2 to 71a66c4 Compare November 21, 2023 13:32

szymon-rd requested a review from nicolasstucki November 22, 2023 10:35

szymon-rd requested a review from Kordyjan November 22, 2023 16:11

nicolasstucki self-assigned this Nov 27, 2023

nicolasstucki reviewed Dec 1, 2023

View reviewed changes

nicolasstucki assigned szymon-rd and unassigned nicolasstucki Dec 1, 2023

nicolasstucki reviewed Dec 1, 2023

View reviewed changes

tests/warn/i16649-refutable.scala Outdated

val '{ $a: Int } = x

val '{ $b: Any } = x

val '{ $c } = x

Copy link

Contributor

nicolasstucki Dec 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

nicolasstucki reviewed Dec 1, 2023

View reviewed changes

hamzaremmal mentioned this pull request Dec 8, 2023

Warn about unchecked type tests in primitive catch cases #19206

Merged

szymon-rd closed this Dec 11, 2023

szymon-rd force-pushed the report-all-warns-werror branch from 5c96677 to feed938 Compare December 11, 2023 14:05

nicolasstucki added a commit that referenced this pull request Dec 21, 2023

Move tests/init from neg to warn (#19241)

beaf7b4

Moves first batch of tests. PR 1/5 (merge consecutively, per [Nicolas' suggestion](#18829 (review)) Split up version of #18829

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Report all warnings on Werror and fail at the end #18829

Report all warnings on Werror and fail at the end #18829

szymon-rd commented Nov 2, 2023 •

edited by nicolasstucki

Loading

szymon-rd commented Nov 2, 2023

nicolasstucki left a comment

nicolasstucki Nov 3, 2023

szymon-rd Nov 3, 2023

szymon-rd Nov 3, 2023

nicolasstucki commented Nov 3, 2023

dwijnand commented Nov 3, 2023

nicolasstucki commented Nov 3, 2023

szymon-rd commented Nov 3, 2023

szymon-rd commented Nov 22, 2023 •

edited

Loading

szymon-rd commented Nov 22, 2023 •

edited

Loading

nicolasstucki Dec 1, 2023

nicolasstucki Dec 1, 2023

nicolasstucki Dec 1, 2023

nicolasstucki Dec 1, 2023

nicolasstucki Dec 1, 2023

nicolasstucki Dec 1, 2023

nicolasstucki Dec 1, 2023

nicolasstucki Dec 1, 2023

nicolasstucki Dec 1, 2023

nicolasstucki Dec 1, 2023

nicolasstucki Dec 1, 2023

nicolasstucki Dec 1, 2023

nicolasstucki left a comment

szymon-rd commented Dec 1, 2023

	report(new Error("No warnings can be incurred under -Werror.", NoSourcePosition))
	report(new Error("Unexpected warnings under -Werror.", NoSourcePosition))

		compileFilesInDir("tests/init-global/neg", options, FileFilter.exclude(TestSources.negInitGlobalScala2LibraryTastyBlacklisted)).checkExpectedErrors()
		compileFilesInDir("tests/init-global/warn", defaultOptions.and("-Ysafe-init-global"), FileFilter.exclude(TestSources.negInitGlobalScala2LibraryTastyBlacklisted)).checkWarnings()

		@@ -776,7 +776,7 @@ trait ParallelTesting extends RunnerOrchestration { self =>
		end maybeFailureMessage

		def getWarnMapAndExpectedCount(files: Seq[JFile]): (HashMap[String, Integer], Int) =

Report all warnings on Werror and fail at the end #18829

Report all warnings on Werror and fail at the end #18829

Conversation

szymon-rd commented Nov 2, 2023 • edited by nicolasstucki Loading

szymon-rd commented Nov 2, 2023

nicolasstucki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolasstucki commented Nov 3, 2023

dwijnand commented Nov 3, 2023

nicolasstucki commented Nov 3, 2023

szymon-rd commented Nov 3, 2023

szymon-rd commented Nov 22, 2023 • edited Loading

szymon-rd commented Nov 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolasstucki left a comment

Choose a reason for hiding this comment

szymon-rd commented Dec 1, 2023

szymon-rd commented Nov 2, 2023 •

edited by nicolasstucki

Loading

szymon-rd commented Nov 22, 2023 •

edited

Loading

szymon-rd commented Nov 22, 2023 •

edited

Loading