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

comments for lint config #3

Open
suaviloquence opened this issue May 27, 2024 · 3 comments
Open

comments for lint config #3

suaviloquence opened this issue May 27, 2024 · 3 comments

Comments

@suaviloquence
Copy link
Owner

suaviloquence commented May 27, 2024

https://blog.mcarr.one/rust-lint-config/

@epage
Copy link

epage commented May 27, 2024

cli flags

Is CLI support needed for an initial implementation? It adds some unique complications and most lint levels should probably be declarative.

module/item attributes

Can these even be supported? I believe registering custom tools is unstable

Additionally, we want to be able to configure the required semver version bump as well as the lint level. There is the future possibility of configuring this (i.e., tool-specified configuration) in the [lints] table, such as enum_missing = { level = "warn", semver = "minor" }, but this functionality is not available in the [lints] table yet.

We support it syntactically but nothing was using it until rust-lang/cargo#13913

What cargo-semver-checks does differently than other tools is that lints are created declaratively with a Trustfall query over the crate's API, instead of as a Rust constant. This means that once/if cargo exposes the functionality for third-party crates to register cargo lints, it may not be plug-and-play (especially if it expects something with a 'static lifetime, as we need to parse them at runtime).

There is disagreement on whether we'd even support registering lints in the same way as rustc. Overall, the value is a lot lower due to our different model of operating.

I'm not sure how well we can integrate with cargo lints right now, and it seems like I will have to write my own level precedence calculator. However, we definitely want to design to integrate with cargo in the future, and if you have any suggestions for how to do that, I'd love to hear them. Additionally, maintaining interface (like CLI) compatibility with tools like rustc is also a goal as much as we can, even if we don't use the same lint mechanisms under the hood.

I could see copy/pasting the code or maybe even generalizing it so you get to reuse the precedence calculations. Maybe it could be a separate library for you to pull in? Probably best to copy/paste initially to not block yourself and then, if it makes sense, you could contribute to cargo to generalize the API so you could switch away from a copy/pasted version.

@obi1kenobi
Copy link
Contributor

Nice writeup!

Re: CLI support, I think the work you did analyzing the existing design of Rust-ecosystem tool CLIs is invaluable, and you've identified the key challenges there. This is great because it means we don't have to implement CLI support on day 1, and instead can design our approach to allow it to be built and plugged in in the future. So I'd probably recommend not building in on day 1, and instead investing our effort toward having manifest-based config shipped sooner.

Re: precedence, we currently don't have the concept of lint groups nor item- or module-level config, so the precedence calculations should be straightforward: for each lint, use the defaults, overridden by whatever the workspace says (if anything), overridden by whatever the package manifest says (if anything). I'm sure this will need to become more sophisticated in the future, but it's not a problem we have to solve it on day 1 if it's not easy to solve.

@obi1kenobi
Copy link
Contributor

obi1kenobi commented May 28, 2024

module/item attributes

Can these even be supported? I believe registering custom tools is unstable

@epage Technically yes, courtesy of the guarantees of the new diagnostic attribute namespace (never error, only warn, even on unknown attributes). It would be gross, but we could use #[diagnostic::semver_checks] to do anything we need, since it'll be preserved and emitted in rustdoc JSON.

There's discussion about making a similar tool attribute namespace that gets preserved in rustdoc JSON so we don't all end up abusing the diagnostic namespace. Not sure if there's been any movement on it, but the cat is most certainly out of the bag here so I hope tool happens sooner rather than later for the sake of everyone's sanity.

See thread here:
https://twitter.com/ekuber/status/1786064474283344217

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

3 participants