Declaring a lint's configuration in the same file as its implementation

[Centri3]

TL;DR:
I propose we turn this:

declare_clippy_lint! {
	/// Lint description
	#[clippy::version = "1.72.0"]
	pub NEW_LINT,
	nursery,
	"default lint description"
}
impl_lint_pass!(NewLint => [NEW_LINT]);

With some configuration options in lib.rs and conf.rs.
Into this:

declare_clippy_lint! {
	/// Lint description
	#[clippy::version = "1.72.0"]
	pub NEW_LINT,
	nursery,
	"default lint description"
}
impl_clippy_lint!(NewLint => [])
// ^^ Maybe this can also be generated automatically be `#[derive(Paperclip)]`?

#[derive(Paperclip)]
struct NewLint {
	#[paperclip(key)]
	avoid_breaking_exported_api: bool,
	/// lint name: etc
	///
	/// Some lint description
	#[paperclip(key = "excessive-nesting-threshold")]
	threshold: u64,
	#[paperclip(default)]
	nodes: NodeSet,
	// Maybe even something like...?
	#[paperclip(default = new_nodeset)]
	nodes: NodeSet,
}

fn new_nodeset() -> NodeSet {
	todo!()
}

---

Creating a new lint in clippy, that has configuration options, is a bit of a pain. You need to edit conf.rs, then lib.rs, then hope that there's no merge conflicts as other lints are merged. Both of these files are the exact opposite place you wish to look to edit your lint's implementation... Which is ideally the same file of its declaration. So let's change that :D

But how, exactly? Wouldn't you need a central hub for all lints to be added to the LintStore? Not exactly, there is a simple-enough way to achieve this: global constructors. And a second, bit more hacky way that I can't even begin to understand...

Global constructors aren't really a thing in Rust, let alone life-before-main, you either have a lazily initialized static or a static evaluated at compile time. But there's nothing stopping you from doing it anyway! This is what both the inventory and ctor crates do (and if it's made by dtolnay, you know it's good). The second way is a bit more complex: https://github.com/dtolnay/linkme

But... There's a major issue. Compatibility. This is an issue with both of these methods. Even if you create a Vec that is filled with every early/late lint, you still need it to run everywhere. These crates support many platforms, but what about ones they don't? They're small, but this is something to keep in mind.

Note though that, AFAICT, linkme is compatible on all platforms, it just can't be built on them.

But, if that's not a big deal, what will this solve? In short, it'll make Clippy's codebase both cleaner and easier to hack onto, with the cost of slightly higher compile time and not supporting a few platforms. Maybe we can convert this to the standard lib.rs under the hood before releasing, but that might be even hackier than how it works currently... As it stands, doing everything right mentioned at the start, despite Clippy having great documentation nowadays, is still pretty difficult and a bit ugly, and this could make it a ton nicer.

Upsides:

• Easier for new contributors to hack onto
• A cleaner codebase

Downsides:

• Less straightforward implementation
• Compile time
• Decreased platform support (for inventory)

Alternatives

• Generating a majority of lib.rs automatically with the same #[derive(Paperclip)] and cargo dev update_lints

Open questions:

• How would this handle lints with shared configuration options, like avoid-breaking-exported-api or msrv?
• Would this really be worth the effort?

@rustbot label +C-an-interesting-project

[Centri3]

I'm nominating this for discussion at the next meeting, I want to make sure there's consensus on A. how it should work, B. whether this is even worth it and C. whether everyone's in favor of this before a majority of the implementation.

@rustbot label +I-nominated

[Centri3]

@rustbot label -I-nominated (will be discussed after meeting)

[rustbot]

Error: Parsing relabel command in comment failed: ...'-nominated' | error: a label delta at >| ' (will be '...

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

[Centri3]

@rustbot label -I-nominated

[blyxyas]

How would configurations like msrv (used by several lints) be handled? Should they stay just on conf.rs?

[Centri3]

I was thinking the config could be read once using some handler that ideally would use linkme to get every key (or update_lints), then they would get from a borrowed FxHashMap, using the default value if it isn't there already

[blyxyas]

Mmmm... I'm not sure if this is a good idea. Having a centralized list is pretty useful for a lot of things like generating the book or multiple lints sharing configurations, and adding more code would not make it more readable, but more complex.

[Centri3]

It depends. There are many avenues we can take to still have that "centralized list" in the case of generating the book, like generating a .json file for each lint in the procedural macro which are then combined together. This would also probably remove the need for the collect-metadata lint to do it itself, and also update_lints as well.

As an example, are queries worse to use than any regular, non-cached function? No, not at all, but they're surely much more complex.

Adding onto it though, yes, more complex, but a lot easier to work with. It's a tradeoff.

[blyxyas]

Maybe I'm misunderstanding. Could you put a simpler example or explicitly write the configuration in conf.rs and lib.rs? I'm just not sure where does the new_nodeset function and nodes: NodeSet field come from.

[Centri3]

new_nodeset was just a demonstration that it could work with a path to a function, in other cases it would call Default::default, nodes is to show that it can store data that isn't initialized in the config. They aren't necessary here, all that is is threshold.

This is what excessive_nesting currently looks like (which is what that's based on):

rust-clippy/clippy_lints/src/utils/conf.rs

Lines 310 to 313 in 588c1ab

	/// Lint: EXCESSIVE_NESTING.
	///
	/// The maximum amount of nesting a block can reside in
	(excessive_nesting_threshold: u64 = 0),

rust-clippy/clippy_lints/src/lib.rs

Lines 1036 to 1042 in 588c1ab

	let excessive_nesting_threshold = conf.excessive_nesting_threshold;
	store.register_early_pass(move || {
	Box::new(excessive_nesting::ExcessiveNesting {
	excessive_nesting_threshold,
	nodes: rustc_ast::node_id::NodeSet::new(),
	})
	});

As you can see, the nodes field is initialized using new (or default if you want). So that's the same as #[paperclip(default)] here. excessive_nesting_threshold is the threshold key.