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

User control over cargo warnings #12235

Open
5 of 6 tasks
Tracked by #84
epage opened this issue Jun 6, 2023 · 6 comments
Open
5 of 6 tasks
Tracked by #84

User control over cargo warnings #12235

epage opened this issue Jun 6, 2023 · 6 comments
Labels
A-diagnostics Area: Error and warning messages generated by Cargo itself. C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` E-hard Experience: Hard S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.

Comments

@epage
Copy link
Contributor

epage commented Jun 6, 2023

Problem

Today, cargo maintainers are very cautious as to what warnings are created because they cannot be turned off by the user when they are intended. The problem is we've not had a mechanism to control it. With #12115, we'll now have a mechanism

Items blocked on diagnostic control

Proposed Solution

As package and workspace level lint control, turning existing warnings into diagnostics where possible

Tasks

Non-blocking

  • rustc-like diagnostics for parsing Cargo.lock
  • rustc-like diagnostics for parsing .cargo/config.toml
  • Deciding how to handle edition compatibility groups and groups in general (since refactor: Remove rust_2024_compatibility lint group #13740 skipped it for 2024)
    • Should we reuse rustc groups or not?
  • rustc-like diagnostics for processing Cargo.toml (ie port warnings off of shell.warn for consistency)
  • Diagnostic summary

Open questions

Notes

There will still be lints that are not related to the package but related to the user or / invocation. At a later stage, we can consider user-level lint levels.#

@epage epage added A-diagnostics Area: Error and warning messages generated by Cargo itself. C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` S-triage Status: This issue is waiting on initial triage. labels Jun 6, 2023
@epage
Copy link
Contributor Author

epage commented Jun 6, 2023

From #10160 (comment)

Just had a talk with @Muscraft. This is mostly my idea but it would be great if we have this kind of error overhaul. It will help things like JSON format message (#8283) or developing Cargo's own lint rules with -Zlints (#12115). It would also open a door to diagonostics translation (rust-lang/rust#100717). Probably the modularization of Cargo could benefit from it as well.

@epage
Copy link
Contributor Author

epage commented Jun 6, 2023

imo we need structured diagnostics which we pass to the diagnostic system which will then be turned into errors or something else

@epage
Copy link
Contributor Author

epage commented Jun 6, 2023

An obvious way to implement this is to shove the lint config in and our of Config as we enter and leave the relevant contexts. This is a bit more brittle and lowers our chances for doing some operations with threading as Config needs to be mutable everywhere

One thought for how we could implement this

  • Rename Config to Context (we might want to do this anyways)
  • Refactor Context to allow decorating it (wrapping it)
  • Whenever we start down a workspace or package specific code path, we decorate the Context with that workspace or package's lint configuration

@weihanglo weihanglo added S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted. E-hard Experience: Hard and removed S-triage Status: This issue is waiting on initial triage. labels Jun 11, 2023
epage added a commit to epage/cargo that referenced this issue Jul 26, 2023
To keep this simple, we are leveraging `unicase` for case-insensitive
comparisons which I've been using for years on other projects.

This also opens the door for us to add cross-platform compatibility
hazard warnings about multiple paths that would write to the same
location on a case insensitive file system.  I held off on that because
I assume we would want rust-lang#12235 first.

This does mean we can't test the "no manifest" case anymore because the
one case (no pun intended) I knew of for hitting it is now gone.
epage added a commit to epage/cargo that referenced this issue Jul 26, 2023
To keep things simple, especially in getting a `Hash` implementation
correct, I'm leveraging `unicase` for case-insensitive
comparisons which is an existing dependency and I've been using for
years on other projects.

This also opens the door for us to add cross-platform compatibility
hazard warnings about multiple paths that would write to the same
location on a case insensitive file system.  I held off on that because
I assume we would want rust-lang#12235 first.

This does mean we can't test the "no manifest" case anymore because the
one case (no pun intended) I knew of for hitting it is now gone.
bors added a commit that referenced this issue Jul 29, 2023
fix(package): Recognize and normalize `cargo.toml`

### What does this PR try to resolve?

This solution is a blend of conservative and easy
- Normalizes `cargo.toml` to `Cargo.toml` on publish
  - Ensuring we publish the `prepare_for_publish` version and include `Cargo.toml.orig`
  - Avoids dealing with trying to push existing users to `Cargo.toml`
- All other cases of `Cargo.toml` are warnings
  - We could either normalize or turn this into an error in the future
  - When helping users with case previously, we've only handle the `cargo.toml` case
  - We already should have a fallback in case a manifest isn't detected
  - I didn't want to put in too much effort to make the code more complicated to handle this

As a side effect, if a Linux user has `cargo.toml` and `Cargo.toml`, we'll only put one of them in the `.crate` file.  We can extend this out to also include a warning for portability for case insensitive filesystems but I left that for after #12235.

### How should we test and review this PR?

A PR at a time will show how the behavior changed as the source was edited

This does add a direct dependency on `unicase` to help keep case-insensitive comparisons easy / clear and to avoid riskier areas for bugs like writing an appropriate `Hash` implementation.  `unicase` is an existing transitive dependency of cargo.

### Additional information

Fixes #12384

[Discussion on Zulip](https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/.60cargo.2Etoml.60.20on.20case.20insensitive.20filesystems)
bors added a commit that referenced this issue Jan 10, 2024
feat: Add `rustc` style errors for manifest parsing

#12235 is tracking user control over warnings. One part of that is making `cargo`'s diagnostic system output messages in the style of `rustc`. To make it so that `cargo` doesn't have to manage a formatter and renderer, I decided to use [`annotate-snippets`](https://crates.io/crates/annotate-snippets), which matches `rustc`'s style well (at one time it was meant to be the formatted for `rustc`).

To get this work kicked off, it was suggested to me that we should start with styling manifest parsing errors, and that is what his PR does.

What manifest parsing errors look like after this change:
![image](https://github.com/rust-lang/cargo/assets/23045215/0eb388b9-8d72-48ad-84a9-585160995078)

---

Note: `cargo` does not currently match `rustc` in color (#12740), `rustc`  specifies their colors [here](https://github.com/rust-lang/rust/blob/740cea81d6e34fc03c4495890e29f1c5ecb40b96/compiler/rustc_errors/src/lib.rs#L1755-L1768) and [here](https://github.com/rust-lang/rust/blob/740cea81d6e34fc03c4495890e29f1c5ecb40b96/compiler/rustc_errors/src/emitter.rs#L2689-L2723). I used `annotate-snippets` default colors which match what `rustc` currently uses for colors. I did this as a stopgap while I work to unify the colors used between `rustc` and `cargo`.

---

Note: the error messages come directly from `toml` and can be quite long. It would be nice if we could put some of the message below the highlight but this would require changes to `toml`.
Example:
```
error: invalid type: integer `3`
 --> Cargo.toml:7:7
  |
7 | bar = 3
  |       ^ expected a version string like "0.9.8" or a detailed dependency like { version = "0.9.8" }
  |
```
bors added a commit that referenced this issue Jan 11, 2024
feat: Add `rustc` style errors for manifest parsing

#12235 is tracking user control over warnings. One part of that is making `cargo`'s diagnostic system output messages in the style of `rustc`. To make it so that `cargo` doesn't have to manage a formatter and renderer, I decided to use [`annotate-snippets`](https://crates.io/crates/annotate-snippets), which matches `rustc`'s style well (at one time it was meant to be the formatted for `rustc`).

To get this work kicked off, it was suggested to me that we should start with styling manifest parsing errors, and that is what his PR does.

What manifest parsing errors look like after this change:
![image](https://github.com/rust-lang/cargo/assets/23045215/0eb388b9-8d72-48ad-84a9-585160995078)

---

Note: `cargo` does not currently match `rustc` in color (#12740), `rustc`  specifies their colors [here](https://github.com/rust-lang/rust/blob/740cea81d6e34fc03c4495890e29f1c5ecb40b96/compiler/rustc_errors/src/lib.rs#L1755-L1768) and [here](https://github.com/rust-lang/rust/blob/740cea81d6e34fc03c4495890e29f1c5ecb40b96/compiler/rustc_errors/src/emitter.rs#L2689-L2723). I used `annotate-snippets` default colors which match what `rustc` currently uses for colors. I did this as a stopgap while I work to unify the colors used between `rustc` and `cargo`.

---

Note: the error messages come directly from `toml` and can be quite long. It would be nice if we could put some of the message below the highlight but this would require changes to `toml`.
Example:
```
error: invalid type: integer `3`
 --> Cargo.toml:7:7
  |
7 | bar = 3
  |       ^ expected a version string like "0.9.8" or a detailed dependency like { version = "0.9.8" }
  |
```
bors added a commit that referenced this issue Mar 4, 2024
fix(toml): Don't warn on unset Edition if only 2015 is compatible

### What does this PR try to resolve?

The warning doesn't help towards the stated goal if the only MSRV-compatible Edition is 2015 (plus, if you're MSRV is that old, you likely know better).

This also fixes a problem in #13505 where we were suggesting to set a field that might require an MSRV bump which may be unactionable to silence and we avoid warnings without an actionable way of silencing until #12235 gives us a general means.

### How should we test and review this PR?

### Additional information

This was discussed in
- #13505
- https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/warn.20on.20missing.20.60edition.60.20field.20in.20Cargo.2Etoml
bors added a commit that referenced this issue Mar 15, 2024
feat(tree): Control `--charset` via auto-detecting config value

### What does this PR try to resolve?

This tries to generalize `cargo tree --charset` so any part of cargo's output can use it.  For example,
- `cargo search` could use this for fancier tables
- `cargo add` could use this for fancier feature lists
- #12235 could use this for fancy rendering like miette does (CC `@Muscraft` )
- Progress bars could use fancier characters <-- this is what I'm personally working towards

This builds on the idea from #12889 that we can use fancier terminal features so long as we have good auto-detection and provide users an escape hatch until the auto-detection is improved or in case they disagree.

As a side benefit
- `cargo tree` will auto-detect when the prior default of `--charset utf8` is a good choice
- Users can control `cargo tree --charset` via `.cargo/config.toml`

### How should we test and review this PR?

This is gradually introduced through the individual commits.

### Additional information
bors added a commit that referenced this issue Mar 23, 2024
feat: Add a basic linting system

This PR adds a basic linting system, the first step towards [User control over cargo warnings](#12235). To demonstrate the system, a lint for #12826 is added. It supports controlling cargo lints via the `[lints.cargo]` table. Lints can be controlled either by a group or individually.

This is meant to lay down some fundamental infrastructure for the whole linting system and is not meant to be fully featured. Over time, features will be added that will help us reach a much more solid state.
@epage
Copy link
Contributor Author

epage commented Mar 25, 2024

One thought I had from #13621 was more about the linting system as a whole.

Should we use cargo::rust_2024_compatibility group or should we key off of rust::rust_2024_compatibility group?

@weihanglo
Copy link
Member

Should we use cargo::rust_2024_compatibility group or should we key off of rust::rust_2024_compatibility group?

What is the advantage of that? I can think of a better coherent view of lints. But if external tools don't know about cargo lints, then the behavior might be inconsistent. (Just an imaginary situation, don't know if they really exist.)

From another angle, it looks like Cargo goes beyond Cargo and starts overriding other tools' defaults. This might need a cross-team discussion.

@epage
Copy link
Contributor Author

epage commented Mar 25, 2024

Take unused_lints. iirc rust::unused_lints affects both rustc and clippy.

So the question would be

  • Is rust::unused_lints global or specialized for rustc linters
  • If not global, does that mean cargo::unused_lints should affect all Cargo.toml linters (e.g. if we moved Cargo lints from clippy-driver to cargo clippy)

Or put another way, does a user say "I want to see 2024 Edition compatibility warnings" or do they say "I want to see Rust 2024 Edition compatibility warnings separately from Cargo 2024 Edition compatibility warnings". The Edition is the same concept, so it seems weird to split it by tool. However, rustfmt does have a distinct Edition so maybe we should keep it more loose generally?

bors added a commit that referenced this issue Apr 19, 2024
Unused dependencies cleanup

The implementation of #12826 was used as a demonstration for #12235, in #13621. The demonstration implementation was far from ideal and was lacking a few features. This PR makes the implementation "feature complete", and fixes known bugs with the initial implementation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Error and warning messages generated by Cargo itself. C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` E-hard Experience: Hard S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.
Projects
None yet
Development

No branches or pull requests

2 participants