Avoid invoking the hir_crate query to traverse the HIR
Walking the HIR tree is done using the `hir_crate` query. However, this is unnecessary, since `hir_owner(CRATE_DEF_ID)` provides the same information. Since depending on `hir_crate` forces dependents to always be executed, this leads to unnecessary work.
By splitting HIR and attributes visits, we can avoid an edge to `hir_crate` when trying to visit the HIR tree.
Provide `layout_of` automatically (given tcx + param_env + error handling).
After #88337, there's no longer any uses of `LayoutOf` within `rustc_target` itself, so I realized I could move the trait to `rustc_middle::ty::layout` and redesign it a bit.
This is similar to #88338 (and supersedes it), but at no ergonomic loss, since there's no funky `C: LayoutOf<Ty = Ty>` -> `Ty: TyAbiInterface<C>` generic `impl` chain, and each `LayoutOf` still corresponds to one `impl` (of `LayoutOfHelpers`) for the specific context.
After this PR, this is what's needed to get `trait LayoutOf` (with the `layout_of` method) implemented on some context type:
* `TyCtxt`, via `HasTyCtxt`
* `ParamEnv`, via `HasParamEnv`
* a way to transform `LayoutError`s into the desired error type
* an error type of `!` can be paired with having `cx.layout_of(...)` return `TyAndLayout` *without* `Result<...>` around it, such as used by codegen
* this is done through a new `LayoutOfHelpers` trait (and so is specifying the type of `cx.layout_of(...)`)
When going through this path (and not bypassing it with a manual `impl` of `LayoutOf`), the end result is that only the error case can be customized, the query itself and the success paths are guaranteed to be uniform.
(**EDIT**: just noticed that because of the supertrait relationship, you cannot actually implement `LayoutOf` yourself, the blanket `impl` fully covers all possible context types that could ever implement it)
Part of the motivation for this shape of API is that I've been working on querifying `FnAbi::of_*`, and what I want/need to introduce for that looks a lot like the setup in this PR - in particular, it's harder to express the `FnAbi` methods in `rustc_target`, since they're much more tied to `rustc` concepts.
r? `@nagisa` cc `@oli-obk` `@bjorn3`
Introduce `let...else`
Tracking issue: #87335
The trickiest part for me was enforcing the diverging else block with clear diagnostics. Perhaps the obvious solution is to expand to `let _: ! = ..`, but I decided against this because, when a "mismatched type" error is found in typeck, there is no way to trace where in the HIR the expected type originated, AFAICT. In order to pass down this information, I believe we should introduce `Expectation::LetElseNever(HirId)` or maybe add `HirId` to `Expectation::HasType`, but I left that as a future enhancement. For now, I simply assert that the block is `!` with a custom `ObligationCauseCode`, and I think this is clear enough, at least to start. The downside here is that the error points at the entire block rather than the specific expression with the wrong type. I left a todo to this effect.
Overall, I believe this PR is feature-complete with regard to the RFC.
Warn when [T; N].into_iter() is ambiguous in the new edition.
Fixes https://github.com/rust-lang/rust/issues/88475
In https://github.com/rust-lang/rust/issues/88475, a situation was found where `[T; N].into_iter()` becomes *ambiguous* in the new edition. This is different than the case where `(&[T; N]).into_iter()` resolves differently, which was the only case handled by the `array_into_iter` lint. This is almost identical to the new-traits-in-the-prelude problem. Effectively, due to the array-into-iter hack disappearing in Rust 2021, we effectively added `IntoIterator` to the 'prelude' in Rust 2021 specifically for arrays.
This modifies the prelude collisions lint to detect that case and emit a `array_into_iter` lint in that case.
rustc_target: `TyAndLayout::field` should never error.
This refactor (making `TyAndLayout::field` return `TyAndLayout` without any `Result` around it) is based on a simple observation, regarding `TyAndLayout::field`:
If `cx.layout_of(ty)` succeeds (for some `cx` and `ty`), then `.field(cx, i)` on the resulting `TyAndLayout` should *always* succeed in computing `cx.layout_of(field_ty)` (where `field_ty` is the type of the `i`th field of `ty`).
The reason for this is that no matter which field is chosen, `cx.layout_of(field_ty)` *will have already been computed*, as part of computing `cx.layout_of(ty)`, as we cannot determine the layout of *any* type without considering the layouts of *all* of its fields.
And so it should be fine to turn any errors into ICEs, since they likely indicate a `cx` mismatch, or some other edge case that is due to a compiler bug (as opposed to ever being an user-facing error).
<hr/>
Each commit should probably be reviewed separately, though note that there's some `where` clauses (in `rustc_target::abi::call::*`) that change in most commits.
cc `@nagisa` `@oli-obk`
Refactor `named_asm_labels` to a HIR lint
As discussed on #88169, the `named_asm_labels` lint could be moved to a HIR lint. That allows future lints or custom plugins or clippy lints to more easily access the `asm!` macro's data and create better error messages with the lints.
Use custom wrap-around type instead of RangeInclusive
Two reasons:
1. More memory is allocated than necessary for `valid_range` in `Scalar`. The range is not used as an iterator and `exhausted` is never used.
2. `contains`, `count` etc. methods in `RangeInclusive` are doing very unhelpful(and dangerous!) things when used as a wrap-around range. - In general this PR wants to limit potentially confusing methods, that have a low probability of working.
Doing a local perf run, every metric shows improvement except for instructions.
Max-rss seem to have a very consistent improvement.
Sorry - newbie here, probably doing something wrong.
Remove `Session.used_attrs` and move logic to `CheckAttrVisitor`
Instead of updating global state to mark attributes as used,
we now explicitly emit a warning when an attribute is used in
an unsupported position. As a side effect, we are to emit more
detailed warning messages (instead of just a generic "unused" message).
`Session.check_name` is removed, since its only purpose was to mark
the attribute as used. All of the callers are modified to use
`Attribute.has_name`
Additionally, `AttributeType::AssumedUsed` is removed - an 'assumed
used' attribute is implemented by simply not performing any checks
in `CheckAttrVisitor` for a particular attribute.
We no longer emit unused attribute warnings for the `#[rustc_dummy]`
attribute - it's an internal attribute used for tests, so it doesn't
mark sense to treat it as 'unused'.
With this commit, a large source of global untracked state is removed.
Improve wording of the `drop_bounds` lint
This PR addresses #86653. The issue is sort of a false positive of the `drop_bounds` lint, but I would argue that the best solution for #86653 is simply a rewording of the warning message and lint description, because even if the lint is _technically_ wrong, it still forces the programmer to think about what they are doing, and they can always use `#[allow(drop_bounds)]` if they think that they really need the `Drop` bound.
There are two issues with the current warning message and lint description:
- First, it says that `Drop` bounds are "useless", which is technically incorrect because they actually do have the effect of allowing you e.g. to call methods that also have a `Drop` bound on their generic arguments for some reason. I have changed the wording to emphasize not that the bound is "useless", but that it is most likely not what was intended.
- Second, it claims that `std::mem::needs_drop` detects whether a type has a destructor. But I think this is also technically wrong: The `Drop` bound says whether the type has a destructor or not, whereas `std::mem::needs_drop` also takes nested types with destructors into account, even if the top-level type does not itself have one (although I'm not 100% sure about the exact terminology here, i.e. whether the "drop glue" of the top-level type counts as a destructor or not).
cc `@jonhoo,` does this solve the issue for you?
r? `@GuillaumeGomez`
Instead of updating global state to mark attributes as used,
we now explicitly emit a warning when an attribute is used in
an unsupported position. As a side effect, we are to emit more
detailed warning messages (instead of just a generic "unused" message).
`Session.check_name` is removed, since its only purpose was to mark
the attribute as used. All of the callers are modified to use
`Attribute.has_name`
Additionally, `AttributeType::AssumedUsed` is removed - an 'assumed
used' attribute is implemented by simply not performing any checks
in `CheckAttrVisitor` for a particular attribute.
We no longer emit unused attribute warnings for the `#[rustc_dummy]`
attribute - it's an internal attribute used for tests, so it doesn't
mark sense to treat it as 'unused'.
With this commit, a large source of global untracked state is removed.
Force warn improvements
As part of stablization of the `--force-warn` option (#86516) I've made the following changes:
* Error when the `warnings` lint group is based to the `--force-warn` option
* Tests have been updated to make it easier to understand the semantics of `--force-warn`
r? `@nikomatsakis`
Improve non_fmt_panics suggestion based on trait impls.
This improves the non_fmt_panics lint suggestions by checking first which trait (Display or Debug) are actually implemented on the type.
Fixes https://github.com/rust-lang/rust/issues/87313
Fixes https://github.com/rust-lang/rust/issues/87999
Before:
```
help: add a "{}" format string to Display the message
|
2 | panic!("{}", Some(1));
| +++++
help: or use std::panic::panic_any instead
|
2 | std::panic::panic_any(Some(1));
| ~~~~~~~~~~~~~~~~~~~~~
```
After:
```
help: add a "{:?}" format string to use the Debug implementation of `Option<i32>`
|
2 | panic!("{:?}", Some(1));
| +++++++
help: or use std::panic::panic_any instead
|
2 | std::panic::panic_any(Some(1));
| ~~~~~~~~~~~~~~~~~~~~~
```
r? `@estebank`
Detect fake spans in non_fmt_panic lint.
This addresses https://github.com/rust-lang/rust/issues/87621
Some proc_macros claim that the user wrote all of the tokens it outputs, by applying a span from the input to all of the produced tokens. That can result in confusing suggestions, as in #87621. This is a simple patch that avoids suggesting anything for `panic!("{}")` if the span of `"{}"` and `panic!(..)` are identical, which is normally not possible.
Uplift the invalid_atomic_ordering lint from clippy to rustc
This is mostly just a rebase of https://github.com/rust-lang/rust/pull/79654; I've copy/pasted the text from that PR below.
r? `@lcnr` since you reviewed the last one, but feel free to reassign.
---
This is an implementation of https://github.com/rust-lang/compiler-team/issues/390.
As mentioned, in general this turns an unconditional runtime panic into a (compile time) lint failure. It has no false positives, and the only false negatives I'm aware of are if `Ordering` isn't specified directly and is comes from an argument/constant/whatever.
As a result of it having no false positives, and the alternative always being strictly wrong, it's on as deny by default. This seems right.
In the [zulip stream](https://rust-lang.zulipchat.com/#narrow/stream/233931-t-compiler.2Fmajor-changes/topic/Uplift.20the.20.60invalid_atomic_ordering.60.20lint.20from.20clippy/near/218483957) `@joshtriplett` suggested that lang team should FCP this before landing it. Perhaps libs team cares too?
---
Some notes on the code for reviewers / others below
## Changes from clippy
The code is changed from [the implementation in clippy](68cf94f6a6/clippy_lints/src/atomic_ordering.rs) in the following ways:
1. Uses `Symbols` and `rustc_diagnostic_item`s instead of string literals.
- It's possible I should have just invoked Symbol::intern for some of these instead? Seems better to use symbol, but it did require adding several.
2. The functions are moved to static methods inside the lint struct, as a way to namespace them.
- There's a lot of other code in that file — which I picked as the location for this lint because `@jyn514` told me that seemed reasonable.
3. Supports unstable AtomicU128/AtomicI128.
- I did this because it was almost easier to support them than not — not supporting them would have (ideally) required finding a way not to give them a `rustc_diagnostic_item`, which would have complicated an already big macro.
- These don't have tests since I wasn't sure if/how I should make tests conditional on whether or not the target has the atomic... This is to a certain extent an issue of 64bit atomics too, but 128-bit atomics are much less common. Regardless, the existing tests should be *more* than thorough enough here.
4. Minor changes like:
- grammar tweaks ("loads cannot have `Release` **and** `AcqRel` ordering" => "loads cannot have `Release` **or** `AcqRel` ordering")
- function renames (`match_ordering_def_path` => `matches_ordering_def_path`),
- avoiding clippy-specific helper methods that don't exist in rustc_lint and didn't seem worth adding for this case (for example `cx.struct_span_lint` vs clippy's `span_lint_and_help` helper).
## Potential issues
(This is just about the code in this PR, not conceptual issues with the lint or anything)
1. I'm not sure if I should have used a diagnostic item for `Ordering` and its variants (I couldn't figure out how really, so if I should do this some pointers would be appreciated).
- It seems possible that failing to do this might possibly mean there are more cases this lint would miss, but I don't really know how `match_def_path` works and if it has any pitfalls like that, so maybe not.
2. I *think* I deprecated the lint in clippy (CC `@flip1995` who asked to be notified about clippy changes in the future in [this comment](https://github.com/rust-lang/rust/pull/75671#issuecomment-718731659)) but I'm not sure if I need to do anything else there.
- I'm kind of hoping CI will catch if I missed anything, since `x.py test src/tools/clippy` fails with a lot of errors with and without my changes (and is probably a nonsense command regardless). Running `cargo test` from src/tools/clippy also fails with unrelated errors that seem like refactorings that didnt update clippy? So, honestly no clue.
3. I wasn't sure if the description/example I gave good. Hopefully it is. The example is less thorough than the one from clippy here: https://rust-lang.github.io/rust-clippy/master/index.html#invalid_atomic_ordering. Let me know if/how I should change it if it needs changing.
4. It pulls in the `if_chain` crate. This crate was already used in clippy, and seems like it's used elsewhere in rustc, but I'm willing to rewrite it to not use this if needed (I'd prefer not to, all things being equal).
- Deprecate clippy::invalid_atomic_ordering
- Use rustc_diagnostic_item for the orderings in the invalid_atomic_ordering lint
- Reduce code duplication
- Give up on making enum variants diagnostic items and just look for
`Ordering` instead
I ran into tons of trouble with this because apparently the change to
store HIR attrs in a side table also gave the DefIds of the
constructor instead of the variant itself. So I had to change
`matches_ordering` to also check the grandparent of the defid as well.
- Rename `atomic_ordering_x` symbols to just the name of the variant
- Fix typos in checks - there were a few places that said "may not be
Release" in the diagnostic but actually checked for SeqCst in the lint.
- Make constant items const
- Use fewer diagnostic items
- Only look at arguments after making sure the method matches
This prevents an ICE when there aren't enough arguments.
- Ignore trait methods
- Only check Ctors instead of going through `qpath_res`
The functions take values, so this couldn't ever be anything else.
- Add if_chain to allowed dependencies
- Fix grammar
- Remove unnecessary allow
