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

Drowning in compilation warnings #90

Open
MateuszKowalewski opened this issue Jul 1, 2023 · 4 comments
Open

Drowning in compilation warnings #90

MateuszKowalewski opened this issue Jul 1, 2023 · 4 comments

Comments

@MateuszKowalewski
Copy link
Contributor

MateuszKowalewski commented Jul 1, 2023

The Scala pattern-match exhaustivity checker isn't the smartest, that's a know issue.

But having so much warnings around regarding not checked matches feels risky.

It's very hard to see new warnings popping up when there are a few hundred other "know good" warnings around.

On the one hand side it makes sense to have the pattern-match warnings as a reminder that something has to happen there, but on the other hands side one does not see the forest for the trees any more.

I would like to suppress all the warnings on the pattern matches that are "known OK" but generate warnings currently by annotating (only) the affected places. Does this make sense for you?

@TomasMikula
Copy link
Owner

Suppressing warnings on pattern matches that are currently known to be exhaustive is not necessarily, as they might cease to be exhaustive in the future, without notice.

But I would go for it if, for every suppression, there will be a link to a scalac issue that should make that suppression unnecessary.

@MateuszKowalewski
Copy link
Contributor Author

I fully understand the sentiment! That's why I asked.

The problem here is: There are likely way too much Scala issues regarding the pattern-match exhaustivity checker. That's a know problem area in Scala. Therefore I would not expect any quick progress on that ground.

But the warnings in Libretto are an issue. If you have hundreds of warning (and that's what we're talking about here) you won't notice any new warnings or some of different kinds. That's a real problem.

I've started already to add @nowarn annotations. Really a lot of leg work, but imho important!

(I will keep that at least locally, no mater whether you pick up the change or not.)

But I would still like to convince you that fine granular warning suppression has value here, even without an attached Scala ticket number. I would not make that a prerequisite, no matter the feelings about the state of Scala in this regard.

I could go complaining to the Scala people in the next step, sure, but first I would clean the compile here.

(We got anyway already a first fix in the IDE which is Libretto related. Will land soon, I hope, and make the error marker go away in FreeScaletto.)

@MateuszKowalewski
Copy link
Contributor Author

Suppressing warnings on pattern matches that are currently known to be exhaustive is not necessarily, as they might cease to be exhaustive in the future, without notice.

I don't buy this argument fully, to be honest.

You're technically right. But only if you monitor a diff of the warnings.

There are so many warnings that you likely wouldn't notice any newly introduced one (without having a script that diffs the warnings).

The other thing is: When you change code (as this is the only way to break the status quo) you would usually anyway temporary comment out the warning suppression. Than you get locally some limited amount of warnings that you can check for sanity. Without having all the other warnings in all other placed disabled at the same time you would need to use your warning-diff-script, which is imho more involved than the procedure that I'm proposing.

So I see a necessity to suppress warnings even it's know that this shouldn't be needed actually.

I think this logic is flawless, isn't it? 😄

@TomasMikula
Copy link
Owner

TomasMikula commented Jul 5, 2023

Suppressing warnings on pattern matches that are currently known to be exhaustive is not necessarily, as they might cease to be exhaustive in the future, without notice.

I don't buy this argument fully, to be honest.

You're technically right. But only if you monitor a diff of the warnings.

I omitted a word in my statement which made it confusing. I meant to say "is not necessarily better", whereas the autocorrect in your head may have corrected it as "is not necessary" 😄

When you change code (as this is the only way to break the status quo) you would usually anyway temporary comment out the warning suppression.

I add a case to an ADT in one place. I don't know all the places where it is pattern matched on to go remove the warning supressions. If I did, I would just go add the respective case statements.

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

No branches or pull requests

2 participants