fix(lints): Prevent inheritance from bring exposed for published packages
#13843 demonstrated a regression caused by #13801, where we started to keep `[lints]` and `[workspace.lints]` separate, and not truly resolve `[lints]`. This was a nice thing to have and made it easier to tell when a lint came from a workspace. The downside of doing so is the lints table would not get resolved when vendoring or publishing.
To fix this issue, I reverted the change for keeping `[lints]` and `[workspace.lints]` separate and modified how cargo's linting system figures out where a lint is coming from. Due to this change, we no longer specify that a lint was set by `[workspace.lints]`, only `[lints]`. It is true that a lint level is set by `[lints]` always, as it would've had to specify the lint outright or specify that it was inheriting it, seeing that, I do not think this is a regression in diagnostic quality. I still manage to keep the ability to render a lint's location in the workspace's manifest when running ` analyze_cargo_lints_table`, which I am pleased about.
perf(toml): Avoid inferring when targets are known
### What does this PR try to resolve?
We read the file system to infer two different data points
- Implicit targets
- Implicit `path` values for targets
I took a shortcut for this case and recognize the scenario where we can
bypass both and do so.
I went with a bypass, rather than this being integrating into the
inferring code because the inferring code is complex and I didn't want
to add to it further in isolating the inferring to only when its needed.
The validation gets duplicated because having it in the middle of the resolve code provides a better user experience and it would be messy to add the conditionals to get all of that working. I at least worked to keep the duplicated validation close to each other.
### How should we test and review this PR?
### Additional information
Clean package perf improvements
### What does this PR try to resolve?
I've noticed that `cargo clean -p` execution time scales poorly with size of target directory; in my case (~250GB target directory on M1 Mac) running `cargo clean -p` takes circa 35 seconds. Notably, it's the file listing that takes that time, not deleting the package itself. That is, when running `cargo clean -p SOME_PACKAGE` twice in a row, both executions take roughly the same time.
I've tracked it down to the fact that we seem quite happy to use `glob::glob` function, which iterates over contents of target dir. It also was a bit sub-optimal when it came to doing that, for which I've already filled a PR in https://github.com/rust-lang/glob/pull/144 - that PR alone takes down cleaning time down to ~14 seconds. While it is a good improvement for a relatively straightforward change, this PR tries to take it even further. With glob PR applied + changes from this PR, my test case goes down to ~6 seconds. I'm pretty sure that we could squeeze this further, but I'd rather do so in a follow-up PR.
Notably, this PR doesn't help with *just* super-large target directories. `cargo clean -p serde` on cargo repo (with ~7Gb target directory size) went down from ~380ms to ~100ms for me. Not too shabby.
### How should we test and review this PR?
I've mostly tested it manually, running `cargo clean` against multiple different repos.
### Additional information
TODO:
- [x] [c770700](c770700885) is not quite correct; we need to consider that it changes how progress reporting works; as is, we're gonna report all progress relatively quickly and stall at the end (when we're actually iterating over directories, globbing, removing files, that kind of jazz). I'll address that.
We read the file system to infer two different data points
- Implicit targets
- Implicit `path` values for targets
I took a shortcut for this case and recognize the scenario where we can
bypass both and do so.
I went with a bypass, rather than this being integrating into the
inferring code because the inferring code is complex and I didn't want
to add to it further in isolating the inferring to only when its needed.
Turns out, we allow these fields, just in limited ways, so we need to be
consistent.
I limited when this applies to reduce noise from the user solving there
problem because they are unlikely to keep the field and switch it to the
opposite value
Starting with this commit we deduplicate calls to rm_rf_prefix_list by crate name and not by directory; this can lead to more calls to rm_rf_prefix_list (especially in presence of multiple -p arguments),
but it is also more transparent in terms of progress reporting (we're just storing away whether a given directory + glob pair has already been removed)
fix(toml): On 2024 Edition, disallow ignored `default-features` when inheriting
### What does this PR try to resolve?
This is part of rust-lang/rust#123754
This is a follow up to #11409 which tweaked how we do inheritance of default-features, including warning when `default-features = false` is ignored. This turns those warnings into an error.
### How should we test and review this PR?
### Additional information
fix(lints): Remove ability to specify `-` in lint name
In a recent Cargo Team meeting, it was discussed whether our lint should use `-` or `_` and if we should rewrite the wrong form to the correct one. It was decided that Cargo would use `_` for lint names and would not convert `-` to `_` automatically; instead, we would warn about an "unknown lint" and mention the similarly named lint with `_`, if found.
The decision to ise `_` was made because it is the canonical representation, as well as [RFC #344](https://rust-lang.github.io/rfcs/0344-conventions-galore.html#lints) specifies:
> Use snake case in the same way you would for function names.
This PR implements these changes.
Note: This adds an `unknown_lints` lint, that tries to mirror [the lint `rustc` has with the same name](https://doc.rust-lang.org/rustc/lints/listing/warn-by-default.html#unknown-lints).
fix(resolver): Treat unset MSRV as compatible
### What does this PR try to resolve?
Have the resolver treat no-MSRV as `rust-version = "*"`, like `cargo add` does for version-requirement selection
### How should we test and review this PR?
We last tweaked this logic in #13066.
However, we noticed this was inconsistent with `cargo add` in automatically selecting version requirements.
It looks like this is a revert of #13066, taking us back to the behavior in #12950.
In #12950 there was a concern about the proliferation of no-MSRV and whether we should de-prioritize those to make the chance of success more likely.
There are no right answes here, only which wrong answer is ok enough.
- Do we treat lack of rust version as `rust-version = "*"` as some people expect or do we try to be smart?
- If a user adds or removes `rust-version`, how should that affect the priority?
One piece of new information is that the RFC for this has us trying to fill the no-MSRV gap with
`rust-version = some-value-representing-the-current-toolchain>`.
See also https://github.com/rust-lang/cargo/issues/9930#issuecomment-1977471059
r? `@Eh2406`
### Additional information
When inheriting a simple dep, we preserved the package dep's `public`
field but lost it when inheriting a detailed dep.
This was done by consolidating the code paths. This also reduces the
risk of us doing this again in the future.
fix(lint): Warn not Error on unsupported lint tool
In a recent Cargo Team meeting, it was decided to lessen the error on an unsupported tool in `[lints]` to a warning. This PR implements that change.
refactor: Move diagnostic printing to Shell
As I have been working on cargo's diagnostic system, I have disliked copying around the code for a new `Renderer`, and then emitting the diagnostic:
```rust
let renderer = Renderer::styled().term_width(
gctx.shell()
.err_width()
.diagnostic_terminal_width()
.unwrap_or(annotate_snippets::renderer::DEFAULT_TERM_WIDTH),
);
writeln!(gctx.shell().err(), "{}", renderer.render(message))?;
```
Moving to a method on `Shell` helps mitigate the risk of updating duplicate code and missing one. It should also make it nearly impossible to get into scenarios where `gctx.shell()` is borrowed twice, leading to panics. This problem was one I ran into early on as I tried to write messages to `stderr` like so:
```rust
writeln!(
gctx.shell().err(),
"{}",
Renderer::styled()
.term_width(
gctx.shell()
.err_width()
.diagnostic_terminal_width()
.unwrap_or(annotate_snippets::renderer::DEFAULT_TERM_WIDTH),
)
.render(message)
)?;
```
bors
Scott Schafer
Eddie Webbinaro
Piotr Osiewicz
Ed Page
Urgau
Weihang Lo
heisen-li