Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Lint level control #58

Closed
Tracked by #61
epage opened this issue Aug 9, 2022 · 7 comments
Closed
Tracked by #61

Lint level control #58

epage opened this issue Aug 9, 2022 · 7 comments
Labels
A-cli Area: engine around the lints C-enhancement Category: raise the bar on expectations

Comments

@epage
Copy link
Collaborator

epage commented Aug 9, 2022

Sometimes it's useful to be able to change the alert level of specific lints for a particular item or an entire module. Clippy and rustc lints have already stabilized syntax like #[deny(lint_name)] and #[allow(lint_name)], and our syntax choices would ideally be reasonably compatible with those.

The kinds of controls we'd like to include are:

  • Forbid: change not allowed regardless of version bump
  • Major change: only allowed in major version bumps
  • Minor change: only allowed in minor version bumps
  • Informational: allowed in all version bumps, but nevertheless reported; for example, if human judgment is necessary to determine whether the change is concerning or not
  • Ignored: not reported at all

The lint level control adjustments should allow both raising and lowering the level of a given lint. For example:

  • semver-minor lints should be possible to be upgraded to semver-major
  • semver-major lints should be possible to be downgraded to semver-minor
  • it should be possible to annotate an entire module as downgrading all semver lints to ignored / informational, e.g. in macro-only or explicitly-unstable modules.
@epage
Copy link
Collaborator Author

epage commented Feb 29, 2024

To clarify something, lints should have two "levels":

  • reporting level (forbid, deny, warn, allow)
  • severity level (major, minor, patch)

In that context, wonder if we should have "virtual reporting groups" based on severity levels, so you can say -Apatch and allow every lint currently configured as "patch".

rustc has the concept of "virtual groups". These are lint groups that are made up of every lint that meets a qualification.

So say you have a command-line like -A<some-warning> -Dwarnings -W<some-warning>. warnings is a virtual reporting group of every lint that is at reporting level "warning". If I understand rustc correctly, by the fact that <some-warning> is allow at the time of denying warnings, we won't deny <some-warning> and can turn it back on.

Granted, the [lints] table doesn't support this back and forth but I was wanting to intentionally show a more perverse case to demonstrate the concept.

@obi1kenobi
Copy link
Owner

So say you have a command-line like -A<some-warning> -Dwarnings -W<some-warning>. warnings is a virtual reporting group of every lint that is at reporting level "warning". If I understand rustc correctly, by the fact that <some-warning> is allow at the time of denying warnings, we won't deny <some-warning> and can turn it back on.

Oh, interesting. I would have assumed that at the end of this command, <some-warning> would be set to warn because -W<some-warning> appears last in the args sequence. In my interpretation, -A<some-warning> -Dwarnings -W<some-warning> and -Dwarnings -W<some-warning> would produce an equivalent effect.

When resolving this issue, we should check this edge case in rustc, match its behavior, and ensure we have tests to back this up.

@epage
Copy link
Collaborator Author

epage commented Feb 29, 2024

Its at least my supposition based on the vague descriptions I've seen about -A<some-warning> -Dwarnings. I agree about double checking.

However, depending on the layers of abstraction and the CLI, this specific corner case might not matter (so long as -A<some-warning> -Dwarnings works). Its a question of whether you exclusively use Cargo.toml (which can't represent the above case) or also provide the CLI (which can represent the case).

@obi1kenobi
Copy link
Owner

I was assuming we'd eventually support both: Cargo.toml recommended for most use cases, CLI available as an override. Perhaps not both added at the same time, of course — probably Cargo.toml first.

@obi1kenobi
Copy link
Owner

Just checking: @epage do you consider this resolved with the lints table in package.metadata, or is having a CLI for lint level control also a blocker for merging into cargo?

If the former, I think we can close this issue. If the latter, we can leave it open, or split off the CLI work into a new issue and mark this one as completed.

@epage
Copy link
Collaborator Author

epage commented Dec 2, 2024

I've not seen any reference to virtual groups but that can be handled when someone runs into a need.

@epage epage closed this as completed Dec 2, 2024
@obi1kenobi
Copy link
Owner

Agreed. The most useful virtual group would be something like "minor lints", but everyone that wants that functionality is currently using --release-type minor which is a great workaround. So we aren't prioritizing groups until something else that isn't well-supported comes up.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-cli Area: engine around the lints C-enhancement Category: raise the bar on expectations
Projects
None yet
Development

No branches or pull requests

3 participants