[epic] Improve lint config discoverability

[nyurik]

Description

There are almost 50 Clippy configuration parameters. Most of them are likely unused by the users, in part because users do not know about them. I would like to propose a simple method to improve config param visibility: all lints should print a note, just once, in its output if the lint's behavior can be changed by a config parameter. The note should not be printed if the config param is already used. This approach would address some concerns from #9787

Note that the current config parser resolves defaults early on, so the lint doesn't know whether the config value is not set, or if it is set to the same value as default. We may want to introduce an additional flag for this in the future.

Example

span_lint_and_then(
    cx,
    DBG_MACRO,
    macro_call.span,
    "`dbg!` macro is intended as a debugging tool",
    |diag| {
        diag.span_suggestion(
            macro_call.span,
            "ensure to avoid having uses of it in version control",
            suggestion,
            applicability,
        );

        // ######################################
        // #########  Add this new code #########
        // ######################################
        if !self.allow_dbg_in_tests {
            // Improve lint config discoverability
            diag.note_once(
                "this lint can ignore test functions if \
                `allow-dbg-in-tests = true` is added in the `clippy.toml` file",
            );
        }
    },
);

[xFrednet]

I agree that config values are a bit hard to discover for users. Maybe I would restrict the message a bit more to only add the note, if the configuration would have potentially suppressed the emission. My main fear with this addition is that the output becomes overwhelming. We could also add a config value to suppress the config message ^^

---

I know that you added a message on Zulip, but let's ping everyone here again @rust-lang/clippy Maybe also @kraktus since they've been quite active lately and might also be interested or have some thoughts (Hope it's okay that I pinged you)

[nyurik]

I think most lints don't start well balanced -- either they don't show as often as developers may want them to, or they are shown too much. If the proposed hints only reduce the number of notifications, it defeats the purpose of discoverability - a way for users to start using new features they may want.

We do not have a way to grow lint usage, so we never know if a lint is not used because it's not helpful, or because users never discovered it.

Having a way to advertise for both increase and decrease of notifications, we allow the lint ecosystem to be more flexible:

• a lint can have multiple "levels" of annoyance, and we can find the most commonly used level by searching github
• we can introduce new lints as defaults from the start without annoying many people because they could show only for the most certain cases - and we will know if people want more by searching for its usage.

Lastly, lets clarify the impact. We are talking about a single line of text. At the bottom of a suggestion that's usually 5-20 lines long. Shown just once in the entire output, even if there are tens of suggestions. Shown next to the line that users tend to read the least - info on how to disable the lint itself. So overall I think this is the least invasive way to help users discover new functionality they often would find useful, compared with the benefits it provides to the developers.

[flip1995]

Every lint gets a message attached that tells the user where the lint level was set, when the lint is first emitted. This is done by rustc, but we might be able to reuse the same machinery for this.

This will only be a one-line help message in the terminal. So as an "easy" first step we can just emit it for every lint that has a config, no matter if the config was already set or not, but only emit it the first time the lint is emitted.

Excluding avoid_breaking_exported_api and the msrv configuration there are about 40+/- lints with a configuration. This is less than 10% of our lints. So just emitting it for all of those once, shouldn't introduce too much output.

I think this is a good idea. I would recommend to do this in clippy_utils::diagnostics (like it is done for the documentation link), so that you don't have to do this with span_lint_and_then every time.

[flip1995]

I think most lints don't start well balanced -- either they don't show as often as developers may want them to, or they are shown too much. If the proposed hints only reduce the number of notifications, it defeats the purpose of discoverability - a way for users to start using new features they may want.

This one we plan to address with nightly lints. So currently all lints that are in nightly will get to stable 8-12 weeks later. We want to triage lints, before we sync them to stable. We will have to implement rustc support for that though.

[nyurik]

@flip1995 that's exactly what I'm using here. Rustc uses the same .noteonce() call as I do. I don't think we would need to put it into utils - it's just one line. I already have it working in my #9865

[flip1995]

I want to put it into utils, so that we don't have to remember to include it into every lint emission of a lint with a config. This will get really hard to maintain. Reviewers would need to remember that this has to be included and we would have to keep the emitted message consistent between lints. We could write an internal lint for it, but I would rather put that effort into solving this problem in clippy_utils.

[nyurik]

@flip1995 I might be misunderstanding your suggestion. Clippy lints currently use span_lint * calls. Do you propose we add another one here, e.g. span_lint_and_config_info, and change all the existing calls to it? It might be limiting because callee would have to choose between span_lint_and_help or span_lint_and_note or span_lint_and_config_info - but what if it is a combination of those, and we would have to introduce span_lint_and_sugg_and_config, span_lint_and_help_and_config, etc?

Note that I am fairly new to clippy codebase, so please do ignore if this is irrelevant or simply a misunderstanding on my part.

      5 span_lint_hir(
     21 span_lint_and_note(
     28 span_lint_hir_and_then(
    103 span_lint_and_help(
    158 span_lint_and_then(
    181 span_lint(
    263 span_lint_and_sugg(

[flip1995]

Sorry, I can see that what I'm suggesting doesn't make sense, if you're not familiar with our diagnostics and how they work. 😅 Curse of Knowledge

So, you might have noticed, that Clippy emits a link to the lint documentation, when emitting a lint. This is handled in clippy_utils::diagnostics and one of the reasons this exists. This is done with the docs_link function:

rust-clippy/clippy_utils/src/diagnostics.rs

Lines 17 to 29 in 51ec465

	fn docs_link(diag: &mut Diagnostic, lint: &'static Lint) {
	if env::var("CLIPPY_DISABLE_DOCS_LINKS").is_err() {
	if let Some(lint) = lint.name_lower().strip_prefix("clippy::") {
	diag.help(&format!(
	"for further information visit https://rust-lang.github.io/rust-clippy/{}/index.html#{lint}",
	&option_env!("RUST_RELEASE_NUM").map_or("master".to_string(), |n| {
	// extract just major + minor version and ignore patch versions
	format!("rust-{}", n.rsplit_once('.').unwrap().1)
	})
	));
	}
	}
	}

This function is then called by each of the functions, that you listed above, see for example span_lint line 50:

rust-clippy/clippy_utils/src/diagnostics.rs

Lines 48 to 53 in 51ec465

	pub fn span_lint<T: LintContext>(cx: &T, lint: &'static Lint, sp: impl Into<MultiSpan>, msg: &str) {
	cx.struct_span_lint(lint, sp, msg, |diag| {
	docs_link(diag, lint);
	diag
	});
	}

We could do the same for the config note. Just create a function that does something like this:

fn config_note(lint, diag) {
    if has_config(lint) { // <- implementing this function will be the hardest part, design-wise
        diag.note_once(...);
    }
}

And then just add a call to this function for every span_lint* function.

[xFrednet]

We could additionally try to add a new section to the book or our lint list, which lists all config values. The metadata collection monster already collects all the information. We would just need to figure out if such a representation is helpful and where to put it :)

[flip1995]

I think the book is a great place for this list of configuration values.

[xFrednet]

For future reference, this issue has been discussed in our meeting today: https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/Meeting.202022-11-29/near/312808259

[flip1995]

Here is a summary:

• Addung extra output is rather not the way to go
• We should extend --explain to include config options
• We should document the config options (in the book)
• We should provide a default config in the book (?)
• Potentially provide example configs

[xFrednet]

@flip1995 Could you explain the difference of the last two points 😅

[flip1995]

The default config would be a toml file with all the configuration values in it, all set to their default value. See how cargo does this: https://doc.rust-lang.org/cargo/reference/config.html#configuration-format

Example configs would be something like: "If you want to use Clippy before releasing a new version, use this config: avoid_breaking_exported_api = false " And then provide a valid toml file for the use cases we can come up with.

[llogiq]

The last one is just a clippy.toml that has all options (if necessary commented out), with useful comments. The previous is having one or more configuration examples in the clippy book.

[BartMassey]

A full discussion of avoid-breaking-public-api, in particular, would be a really welcome addition to the Clippy book. I just found out about this, and — yikes. Wish I'd known about it from the beginning.

[nyurik]

I think that cargo v1.69 adding the fix suggestions is a great example of helping discoverability, something we should follow in the same way

[llogiq]

I like the idea. I still think having a deeper chapter about configuration in the book is still a good idea independently.

I wonder if we could change our config API so it would automatically create the diagnostic along with the config so it's easier not to forget it?

[xFrednet]

We now have a chapter that documents the current configurations better: Lint Configuration Options but I agree that a deeper chapter about the configuration would be nice.

@blyxyas this might be interesting for you, since you're currently reworking the book ❤️

[blyxyas]

The Lint Configuration Options chapter has been re-vamped almost completely, focusing on readability. (#10699) Althought it seems pretty dead (hasn't had any interactions since its creation)

[BartMassey]

I would be pleased to see those changes merged.