Skip to content
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

Make fatal warnings not fail compilation early & aggregate warns #19245

Merged
merged 5 commits into from
Jan 31, 2024

Conversation

szymon-rd
Copy link
Contributor

@szymon-rd szymon-rd commented Dec 11, 2023

Final PR. Adds functionality that changes the behaviour of fatal-warnings - fixes #18634
PR 5/5 (merge consecutively, per Nicolas' suggestion

Split up version of #18829

@szymon-rd szymon-rd force-pushed the fatal-warnings-no-early-exit branch from cbe9041 to ee10082 Compare December 12, 2023 12:14
@szymon-rd szymon-rd force-pushed the fatal-warnings-no-early-exit branch from ee10082 to 06d518d Compare December 12, 2023 14:09
@szymon-rd szymon-rd force-pushed the fatal-warnings-no-early-exit branch from 06d518d to daf1718 Compare December 15, 2023 15:44
@szymon-rd szymon-rd force-pushed the fatal-warnings-no-early-exit branch 2 times, most recently from f8bbc39 to 2c46f0a Compare December 17, 2023 21:33
@szymon-rd szymon-rd force-pushed the fatal-warnings-no-early-exit branch from 2c46f0a to 9ad6c72 Compare December 18, 2023 10:39
@szymon-rd szymon-rd force-pushed the move-error-warn-b3 branch 8 times, most recently from 039744e to bfe0c70 Compare January 25, 2024 14:34
Base automatically changed from move-error-warn-b3 to main January 25, 2024 17:27
@szymon-rd szymon-rd force-pushed the fatal-warnings-no-early-exit branch from 9ad6c72 to 68f4dbc Compare January 25, 2024 17:33
@szymon-rd
Copy link
Contributor Author

szymon-rd commented Jan 25, 2024

There was some problem with moving these tests to warn when I tried it before, but I will look a bit into that now to make sure.

@szymon-rd szymon-rd force-pushed the fatal-warnings-no-early-exit branch from 0f48680 to f0b2216 Compare January 26, 2024 12:37
@szymon-rd
Copy link
Contributor Author

I moved most of them to warn

@@ -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))
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@nicolasstucki nicolasstucki left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM

@@ -216,7 +216,6 @@ class CompilationTests {
@Test def checkInitGlobal: Unit = {
implicit val testGroup: TestGroup = TestGroup("checkInitGlobal")
val options = defaultOptions.and("-Ysafe-init-global", "-Xfatal-warnings")
Copy link
Contributor

Choose a reason for hiding this comment

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

We should inline options into compileFilesInDir("tests/init-global/pos", options,

@szymon-rd szymon-rd force-pushed the fatal-warnings-no-early-exit branch from 6251697 to e410ad4 Compare January 30, 2024 13:36
@szymon-rd szymon-rd force-pushed the fatal-warnings-no-early-exit branch from e410ad4 to ded027e Compare January 30, 2024 13:43
@nicolasstucki
Copy link
Contributor

This uncovered a bug in tests/neg/inline-unstable-accessors.scala. This should not be fixed in this PR. I opened issue #19572 to track this bug.

To workaround this issue do

package object H:
  inline def inlined =
-   G.valBinaryAPI2 +
+   // G.valBinaryAPI2 + // FIXME should error (now fails -Ycheck)
    G.valBinaryAPI3
package J:
  inline def inlined =
-    I.valBinaryAPI2 +
+   //  I.valBinaryAPI2 + // FIXME should error (now fails -Ycheck)
    I.valBinaryAPI3

and update the checkfile.

@szymon-rd szymon-rd force-pushed the fatal-warnings-no-early-exit branch from ded027e to 35b0509 Compare January 31, 2024 12:16
@szymon-rd szymon-rd merged commit 62f0324 into main Jan 31, 2024
37 checks passed
@szymon-rd szymon-rd deleted the fatal-warnings-no-early-exit branch January 31, 2024 14:15
@Kordyjan Kordyjan added this to the 3.4.1 milestone Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Not give up on compilation instantly when -Werror is on and warning appears
3 participants