Add a [lints] table to Cargo.toml
#3389
fix: Be explicit that lint configuration is a future possibility
- Loading branch information
commit 354396739f69e10a01916d620990dc5ae6e1c57e
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually think this is probably necessary for this RFC to make sense. I feel this makes more sense in the local (potentially-but-not-necessarily checked-in) config as opposed to the crate config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though with workspace inheritance perhaps it's not that necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you clarify why you feel it makes more sense to put this in
.cargo/config.tomloverCargo.toml?In starting this with
Cargo.toml, I'm looking to address the existing problems with doing this via rustflags in.cargo/config.toml(project configuration being directory dependent). EmbarkStudios/rust-ecosystem#59 is a good example of people using both source code and.cargo/config.tomlfor lints that are project-specific and would benefit from this. This was also spurred by the cargo team itself wanting to use this feature.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense to allow it in both.
The reason being that there are multiple lint workflows: sometimes you lint in CI and want everyone to follow the same lint set. However, some people may wish to opt in to additional lints locally for their own code, and being able to set that is useful.
But I'm fine with starting out in Cargo.toml at least. I would prefer to try and solve it for both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The part I'm missing is why users can't just use rustflags for this in the short term? In the long term, if we add configurable lints, there isn't an alternative and I can see it becoming a higher priority then. This is the main reason I put it as a future possibility, rather than including it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you are cross-compiling, there is no way to set rustflags via
.config/cargo.tomlthat applies to everything (built for host or built for target), so even just for having something that works across proc-macros and normal crates within a workspace, this would be great.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? Lints don't affect the compiled binaries. It seems like pure overhead to me.
While I'd want to be able to set lints globally (for every project on my computer), there is a design issue of config priority which is unclear to me. Currently afaik the only option duplicated in both
config.tomlandCargo.tomlis[profile], and in its case theconfig.tomlhas priority. In particular, a[profile]section in$HOME/.cargo/config.tomlwould override the[profile]in any other Cargo project.That may be reasonable for
[profile], but would likely be incorrect for lints. Most of the time I'd want to use the lints from the specific project I work on, with theconfig.tomldefaults (particularly defined in non-project directories) used only occasionally. So the correct priority looks like$WORKSPACE/.cargo/config.toml>$WORKSPACE/subcrate/Cargo.toml>$WORKSPACE/Cargo.toml>$UPPER_DIR/.cargo/config.toml. This looks a bit convoluted and inconsistent with the behaviour of otherconfig.tomlkeys.Maybe there should be a cargo command-line flag which would control whether
Cargo.tomlorconfig.tomllint settings should be used?Actually, if
cargo build/run/testwould have flags to set lint levels per invocation, that would be sufficient. That would also avoid the question of config priorities.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a user adds additional lints and run
cargo check, they would expect to see the result of those lints. This would only be possible by rebuilding which will require finger printing.For now, I am leaving
[lints]for.cargo/config.tomlas a future possibility rather than removing it or making it required to move this proposal along. Keeping it as a future possibility has no cost. Adding it into the proposal has a larger cost, in design, decision making, and implementation. It would require a very strong motivation to block incremental improvement ([lints]inCargo.toml) on getting a full solution (e.g. including[lints]in.cargo/config.toml).With that context, maybe we can temper how much design work we put into the feature or how much we debate whether we should or shouldn't do it in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I wouldn't want future extensions to block this long-waited feature either.
When you say "rebuilding", do you mean rebuilding the workspace crates or all dependencies? I am most concerned about the latter, even though for large projects there could be no difference. I am not familiar with the inner details of
cargo check.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rebuilding the package when its
[lints]table changes and not its dependencies.When writing up my previous message, I realized this was ambiguous in the RFC and I have since clarified it.