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

Check if a fatal warning issued in typer is silenced, before converting it into an error #18089

Merged
merged 1 commit into from
Oct 12, 2023

Conversation

KacperFKorban
Copy link
Member

closes lampepfl#17741
closes lampepfl#17735

@KacperFKorban KacperFKorban marked this pull request as ready for review June 28, 2023 18:19
@KacperFKorban KacperFKorban requested a review from dwijnand June 28, 2023 18:20
Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

I have a number of issues with this.

First the premise: I don't see how this is doing anything before converting it to an error, if anything it looks like it's converting warnings to errors before running through Wconf.

Your test doesn't seem to capture the intent of the original 17741 and the issue 17735 doesn't have a test case either. Your 17741 seems to be triggering a auto-apply-insertion warning, which is correctly suppressed by @nowarn. So it's nothing to do with -Wnonunit-statement, which is also missing from the test case anyways.

…ng it into an error

closes lampepfl#17741
closes lampepfl#17735
@KacperFKorban
Copy link
Member Author

Converting warnings to errors before checking -Wconf was a mistake, I fixed it now and added a test to make sure.

This change is supposed to address the fact that when using StoreReporter with fromTyperState = true, issueUnconfigured was called before issueIfNotSuppressed. Because of that, warnings under -Xfatal-warnings were converted to errors before any check for -Wconf or @nowarn was done. (related PR: #13911)

The bug had nothing to do with -Wnonunit-statement or -Wvalue-discard, so I used a simpler test before. I added the tests for i17735 and i17741 now. (Though in i17741, I used a macro-expanded version)

@KacperFKorban KacperFKorban requested a review from dwijnand June 30, 2023 17:43
@KacperFKorban
Copy link
Member Author

@dwijnand Have you had some time to read my last comment? (It might have gotten buried in your notifications)

@dwijnand dwijnand merged commit ef28cd6 into scala:main Oct 12, 2023
@dwijnand dwijnand deleted the fix-i17741 branch October 12, 2023 17:10
@Kordyjan Kordyjan added this to the 3.4.0 milestone Dec 20, 2023
WojciechMazur added a commit that referenced this pull request Jun 20, 2024
…e converting it into an error" to LTS (#20648)

Backports #18089 to the LTS branch.

PR submitted by the release tooling.
[skip ci]
WojciechMazur added a commit that referenced this pull request Jun 20, 2024
…e converting it into an error" to LTS (#20701)

Backports #18089 to the LTS branch.

PR submitted by the release tooling.
[skip ci]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants