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

Expose lint levels from CLI #14522

Open
Kixunil opened this issue Sep 9, 2024 · 11 comments
Open

Expose lint levels from CLI #14522

Kixunil opened this issue Sep 9, 2024 · 11 comments
Labels
A-lints Area: rustc lint configuration C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` S-needs-info Status: Needs more info, such as a reproduction or more background for a feature request.

Comments

@Kixunil
Copy link

Kixunil commented Sep 9, 2024

Problem

There are cases when it's not desirable configuring the lint levels using Cargo.toml and RUST(DOC)FLAGS is not discoverable and also has the footgun that RUSTFLAGS != RUSTDOCFLAGS. Specifically it's useful to have things like #![warn(missing_docs)] in the lib.rs but deny the warning in the CI because it's annoying to have to document everything if one is just designing the API and manually editing the file (potentially accidentally committing it) is also annoying. Editing Cargo.toml from CI's shell script is fragile.

Proposed Solution

It'd be best to just expose the lint levels from cargo and make sure cargo sets them correctly for rustc and rustdoc. Also please don't forget forbid because that level is also useful and even if it's not super-important it'll be probably just a few more lines once other lint levels are in.

Notes

I've opened this as requested in #12739 (comment)

@Kixunil Kixunil added C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` S-triage Status: This issue is waiting on initial triage. labels Sep 9, 2024
@epage
Copy link
Contributor

epage commented Sep 9, 2024

Specifically it's useful to have things like #![warn(missing_docs)] in the lib.rs but deny the warning in the CI because it's annoying to have to document everything if one is just designing the API and manually editing the file (potentially accidentally committing it) is also annoying.

Is #8424 sufficient for this use case?

@epage epage added S-needs-info Status: Needs more info, such as a reproduction or more background for a feature request. A-lints Area: rustc lint configuration and removed S-triage Status: This issue is waiting on initial triage. labels Sep 9, 2024
@Kixunil
Copy link
Author

Kixunil commented Sep 9, 2024

For that specific use case yes, it is. But there is also at least one different use case.

@epage
Copy link
Contributor

epage commented Sep 9, 2024

Could you elaborate on that? I'm not seeing it mentioned in the issue and we'd need to understand requirements to be able to discuss designs.

@Kixunil
Copy link
Author

Kixunil commented Sep 9, 2024

Basically I need -F unsafe-code to check is a crate contains unsafe because unsafe code needs (expensive) miri test but safe doesn't (and this is nuanced which is why I don't think miri should be doing this decision). --forbid-unsafe would do the job but it looks really ad-hoc "why only forbid?" and "why only unsafe?".

@epage
Copy link
Contributor

epage commented Sep 9, 2024

For me, the question is if -F unsafe-code has enough of a use case to justify a specific or general flag over still pointing to RUSTFLAGS / RUSTDOCFLAGS.

@Kixunil
Copy link
Author

Kixunil commented Sep 9, 2024

Maybe it's other way around. If there's use case for --allow/--warn/--deny then it'd be weird not to add --forbid. It's hard to believe that nobody would need those but maybe it is the case.

@epage
Copy link
Contributor

epage commented Sep 9, 2024

Maybe it's other way around. If there's use case for --allow/--warn/--deny then it'd be weird not to add --forbid. It's hard to believe that nobody would need those but maybe it is the case.

Yes, we would handle all of those together. The point I was raising was whether there is enough of a use case for those general flags to exist in Cargo's CLI.

@kornelski
Copy link
Contributor

I run cargo clippy --fix -- -Wclippy::pedantic. I don't want pedantic in my normal lints table, but when it's combined with --fix that's okay, because I selectively commit only acceptable changes.

The status quo here is mostly okay, but that extra -- needed is inelegant. It'd be nicer if Cargo passed through the --warn lint.

@epage
Copy link
Contributor

epage commented Sep 9, 2024

I wonder if that case would be another use case for a new lint level which I proposed at https://internals.rust-lang.org/t/forbid-deny-warn-allow-and-notice/19986

@Kixunil
Copy link
Author

Kixunil commented Sep 10, 2024

Maybe if you really, really don't want to pollute CLI, a different way to solve these issues would be to have something like --override-cargo-toml override.toml option, the override file could contain anything Cargo.toml and conflicting values would get replaced by the override. That way we could override settings in Cargo.toml without it being fragile. One other use case that comes to mind is to have publish = false by default and using the override when publishing.

@epage
Copy link
Contributor

epage commented Sep 18, 2024

One other use case that comes to mind is to have publish = false by default and using the override when publishing.

For myself, that seems strange for publish to change like that and calls out a good complication to having an "override": it complicates provenance for that crate itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lints Area: rustc lint configuration C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` S-needs-info Status: Needs more info, such as a reproduction or more background for a feature request.
Projects
None yet
Development

No branches or pull requests

3 participants