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

Warning summary respects -nowarn #8923

Merged
merged 3 commits into from
May 4, 2020
Merged

Warning summary respects -nowarn #8923

merged 3 commits into from
May 4, 2020

Conversation

som-snytt
Copy link
Contributor

Previously, hasWarning meant a warning had been displayed
(as opposed to counted). Since last refactor, it means
counted, so warnings were summarized under -nowarn.
Until 2.13.1, there was no summary under -nowarn.

Let -nowarn mean -Xmaxwarns 0 (to mean show me no warnings)
and additionally don't summarize any warnings (including
erroring under -Werror).

Fixes scala/bug#11952

@scala-jenkins scala-jenkins added this to the 2.13.3 milestone Apr 26, 2020
@lrytz
Copy link
Member

lrytz commented Apr 27, 2020

Some tests failed because summaries no longer show in repl tests

@som-snytt
Copy link
Contributor Author

som-snytt commented Apr 27, 2020

Edit: fixed just the REPL output, which was due to withSuppressedWarnings also tamping maxwarns but not restoring it.

@som-snytt som-snytt marked this pull request as draft April 27, 2020 09:17
@som-snytt som-snytt marked this pull request as ready for review April 27, 2020 10:57
@som-snytt
Copy link
Contributor Author

It still says 1 warning, which is better than 9.

[info] Non-compiled module 'compiler-bridge_2.13' for Scala 2.13.3-20200429-100920-dd427d7. Compiling...
1 warning

@som-snytt
Copy link
Contributor Author

@lrytz That leaking warning is because reportSuspendedMessages (which sets suppressionsComplete) is at end of typer phase, so a warning from patmat (if not "suppressed") is issued immediately. Why does that happen after typer and not after the run, at summarize time?

The division of state and responsibility between PerRunReporting and the Reporter is confusing to me.

Probably just make ConsoleReporter not issue summary under -nowarn, then the warning count is irrelevant.

@som-snytt
Copy link
Contributor Author

Also picks up a commit to check that compiler options to a partest DirectTest are kosher.

Noticed while linting run tests; maybe there is a neg test that intentionally tests bad args?

@som-snytt som-snytt requested a review from lrytz April 29, 2020 22:39
@lrytz
Copy link
Member

lrytz commented Apr 30, 2020

That leaking warning is because reportSuspendedMessages (which sets suppressionsComplete) is at end of typer phase, so a warning from patmat (if not "suppressed") is issued immediately.

I don't really follow what happens here. suppressionsComplete is about @nowarn annotations within source code, which we only collect during typer. So to avoid displaying warnings that would be hidden by a @nowarn, all warnings are buffered until after typer.

All that should not have anything to do with settins.nowarn. Are you saying that some patmat warning is getting issued in 2.13.2 under -nowarn?

The division of state and responsibility between PerRunReporting and the Reporter is confusing to me.

I think the division exists because of being within or outside the global cake. But I see the point: it would probably be good to move everything to Reporter that doesn't need access to Symbol, and make RunReporting a few convenience methods.

@som-snytt
Copy link
Contributor Author

Yes, patmat issues warning, debug stack trace:

        at scala.reflect.internal.Reporter.increment(Reporting.scala:121)
        at scala.reflect.internal.Reporter.filteredInfo(Reporting.scala:115)
        at scala.reflect.internal.Reporter.warning(Reporting.scala:110)
        at scala.tools.nsc.Reporting$PerRunReporting.issueWarning(Reporting.scala:116)
        at scala.tools.nsc.Reporting$PerRunReporting.checkSuppressedAndIssue(Reporting.scala:130)
        at scala.tools.nsc.Reporting$PerRunReporting.warning(Reporting.scala:251)
        at scala.tools.nsc.Reporting$PerRunReporting.warning(Reporting.scala:255)
        at scala.tools.nsc.typechecker.Contexts$ContextReporter.warning(Contexts.scala:1638)
        at scala.tools.nsc.typechecker.Contexts$Context.warning(Contexts.scala:813)
        at scala.tools.nsc.transform.patmat.Interface$MatchMonadInterface.reportMissingCases(PatternMatching.scala:196)

It is "counted", so ConsoleReporter on finish summarizes it.

This PR adds two checks for -nowarn at PerRunReporter.summarize and also ConsoleReporter. This doesn't seem terrible to me, as the semantics are simply, "Don't print warning-related messages, including summaries."

But if -nowarn sets -Xmaxwarns 0 why are there any warnings? Because maxwarns only tells reporters.Reporter to cap the warnings that are displayed; the count is "upstream" in internal.Reporter.

As a footnote, not only cake per se but the split due to reflect is ongoing confusion. By confusion, I mean I couldn't understand the whole picture as a casual contributor.

Previously, hasWarning meant a warning had been displayed
(as opposed to counted). Since last refactor, it means
counted, so warnings were summarized under -nowarn.
Until 2.13.1, there was no summary under -nowarn.

Let -nowarn mean -Xmaxwarns 0 (to mean show me no warnings)
and additionally don't summarize any warnings (including
erroring under -Werror).

Named constants for Reporter.filter

Test that suppression is relaxed in debug mode.

Restore maxwarns after suppressing

Scaladoc also touches nowarn

ConsoleReporter respects nowarn, since PerRunReporting
suppression mechanism has a checkpoint after typer,
such that subsequent warnings are issued directly and
therefore bump the warnings count.
If there is an error running a forked command, such as
a Test or javac, log it instead of letting sbt swallow
the exception. (It offers to let you run the last command
to see the stack trace.)

This helps if javac goes missing.
Previously, a bad compiler option was ignored.

Add a common idiom to StreamCapture to set stdout;
the idiom may be suboptimal for testing.

Convert a partest to junit for regex, and eliminate
one check file.
@SethTisue SethTisue added the prio:blocker release blocker (used only by core team, only near release time) label Apr 30, 2020
@som-snytt
Copy link
Contributor Author

[error] Caused by: sbt.ForkMain$ForkError: java.io.IOException: No space left on device

@som-snytt
Copy link
Contributor Author

/rebuild

Copy link
Member

@lrytz lrytz left a comment

Choose a reason for hiding this comment

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

Thank you for giving so much of your patience and time budgeds!

@lrytz lrytz merged commit 8538d98 into scala:2.13.x May 4, 2020
@SethTisue SethTisue removed the prio:blocker release blocker (used only by core team, only near release time) label May 5, 2020
@som-snytt som-snytt deleted the issue/11952 branch May 9, 2024 17:14
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.

Scala 2.13.2 bridge builds with warnings
4 participants