rust-lang / cargo

The Rust package manager
https://doc.rust-lang.org/cargo
Apache License 2.0
12.79k stars 2.42k forks source link

Expose lint levels from CLI #14522

Open Kixunil opened 2 months ago

Kixunil commented 2 months ago

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 https://github.com/rust-lang/cargo/issues/12739#issuecomment-2338147752

epage commented 2 months ago

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?

Kixunil commented 2 months ago

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

epage commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 1 month ago

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.