Skip to content

Add option enabling default warnings #19580

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

Closed
wants to merge 1 commit into from

Conversation

szymon-rd
Copy link
Contributor

@szymon-rd szymon-rd commented Jan 31, 2024

Resolve #18559

@SethTisue
Copy link
Member

fyi @som-snytt

@som-snytt
Copy link
Contributor

Yeah, I already said -Xlint, but I guess divergence in options is what it is. (I think paulp removed -Wall because you never want all of them, since at least one will be noisy. If you don't have a noisy warning, then you don't have enough warnings.) (Also, not a fan of all when there is _.)

@szymon-rd
Copy link
Contributor Author

szymon-rd commented Feb 1, 2024

@som-snytt We already have -Xlint for private/type param shadowing, the desc is advanced warning. So we have WvalueDiscard, WNonUnitStatement, WimplausiblePatterns, WunstableInlineAccessors, Wunused, and Xlint. One idea is that it may be better to change the Xlint in the direction of Wshadow and keep all of them separate. The second thing is that given that almost all the linting is already -W, the option enabling them should start with -W. The intention is to enable all of them, but we will probably skip some in the future. Maybe the real goal is not to enable them all, but to enable the recommended set that will keep your code decent. I am open to hearing other options but I'd like them to start with -W - -Wsafe, or maybe just -W / -Warn

@szymon-rd
Copy link
Contributor Author

szymon-rd commented Feb 1, 2024

-Warn could be a full version of -W. If no value is provided, the default is used (-W == -W:default == -Warn == -Warn:default). For real Wall with all warnings, -Warn:all could be introduced.

The existence of -Warn as alias to -W is up for discussion. As for the -Xlint I would keep it for compact as an alias for -W, as it was present in Scala 2. I would not make it default. X in -Xlint historically stands for advanced options, usually a bit more complex, not as approachable, and sometimes unstable. Warning & linting is meant to be basic, simple, and stable.

@szymon-rd szymon-rd changed the title Add -Wall option Add option enabling default warnings Feb 1, 2024
@SethTisue
Copy link
Member

I agree that it's strongly desirable to use -W and avoid -X. The only reason Scala 2's -Xlint is -Xlint is that the -W prefix hadn't been introduced yet at the time. If -Xlint is kept at all, I even think it should be deprecated. It's not that hard for crossbuilt projects to use different flags for the two versions. Nearly every open source project I've seen already passes different compiler flags for 2 and 3.

I don't see a need for -Warn as an alias for -W — let's not proliferate alternatives and aliases. Also I think it's peculiar naming, it reads as "W + arn", like "Wconf" is "W + conf".

@som-snytt
Copy link
Contributor

som-snytt commented Feb 1, 2024

-Xlint is from javac, as was -Werror. (Not that WWJD is our lodestone or polestar.)

Modulo naming, I think it's nice to have a simple "here's the recommended (safe) set of warnings that are useful but not drearily painful."

But it's also nice to be able to say, "Shut down ALL the garbage mashers." (Oh, they weren't "trash compactors"! Memory is so fickle.)

Thanks, Seth, I will start calling it arn in a pirate voice.

Edit: on stability, we never figured out how to do that. Lints are unstable and progress, which has caused problems for cross-compiling, especially before nowarn and -Wconf but also after, especially when linting nowarn itself.

@szymon-rd
Copy link
Contributor Author

Close in favor of #19843

@szymon-rd szymon-rd closed this Mar 1, 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.

Add a flag that enables all linting
3 participants