Rollup of 6 pull requests#156555
Closed
JonathanBrouwer wants to merge 69 commits into
Closed
Conversation
The program will suggest using the `then_some` method. changelog: add [`some_filter`]
It currently only depends on two things: - `rustc_ast::AttrId`: this is just a re-export of `rustc_span::AttrId`, so we can import the original instead. - `rustc_ast::AttributeExt`: needed only for the `name` and `id` methods. We can instead pass in the `name` and `id` directly.
Previously when adding a suggestion for using `Cow::into_owned()` instead of `ToOwned::to_owned()`, the compiler would just convert the methods `Span` into a `String` and do checks on that `String`. This PR adds an extra guard to that suggestion by checking if the method is `sym::to_owned_method`.
Add extra symbol check for `.to_owned()` Follow up of rust-lang#154646 Previously when adding a suggestion for using `Cow::into_owned()` instead of `ToOwned::to_owned()`, the compiler would just convert the methods `Span` into a `String` and do checks on that `String`. This PR adds an extra guard to that suggestion by checking if the method is `sym::to_owned_method`. r? @davidtwco since you added the review to the previous PR
*[View all comments](https://triagebot.infra.rust-lang.org/gh-comments/rust-lang/rust-clippy/pull/15793)* This is needed for `SpanlessEq` to work correctly inside a macro expansion. The context selected for some of the lints may be wrong, but most of them don't handle macros correctly in the first place so this won't really be a regression. Without this change `SpanlessEq` would essentially assume the root context which isn't always correct. changelog: none
Make retags an implicit part of typed copies Ever since Stacked Borrows was first implemented in Miri, that was done with `Retag` statements: given a place (usually a local variable), those statements find all references stored inside the place and refresh their tags to ensure the aliasing requirements are upheld. However, this is a somewhat unsatisfying approach for multiple reasons: - It leaves open the [question](rust-lang/unsafe-code-guidelines#371) of where to even put `Retag` statements. Over time, the AddRetag pass settled on one possible answer to this, but it wasn't very canonical. - For assignments of the form `*ptr = expr`, if the assignment involves copying a reference, we probably want to do a retag -- but if we do a `Retag(*ptr)` as the next instruction, it can be non-trivial to argue that this even retags the right value, so we refrained from doing retags in that case. This has [come up](llvm/llvm-project#160913 (comment)) as a potential issue for Rust making better use of LLVM "captures" annotations. (That said, there might be [other ways](rust-lang/unsafe-code-guidelines#593 (comment)) to obtain this desired optimization.) - Normal compilation avoids generating retags, but we still generate LLVM IR with `noalias`. What does that even mean? How do MIR optimization passes interact with retags? These are questions we have to figure out to make better use of aliasing information, but currently we can't even really ask such questions. I think we should resolve all that by making retags part of what happens during a typed copy (a concept and interpreter infrastructure that did not exist yet when retags were initially introduced). Under this proposal, when executing a MIR assignment statement, what conceptually happens is as follows: - We evaluate the LHS to a place. - We evaluate the RHS to a value. This does a typed load from memory if needed, raising UB if memory does not contain a valid representation of the assignment's type. - We walk that value, identify all references inside of it, and retag them. If this happens as part of passing a function argument, this is a protecting retag. - We store (a representation of) the value into the place. However, this semantics doesn't fully work: there's a mandatory MIR pass that turns expressions like `&mut ***ptr` into intermediate deref's. Those must *not* do any retags. So far this happened because the AddRetag pass did not add retags for assignments to deref temporaries, but that information is not recorded in cross-crate MIR. Therefore I instead added a field to `Rvalue::Use` to indicate whether this value should be retagged or not. A non-retagging copy seems like a sufficiently canonical primitive that we should be able to express it. Dealing with the fallout from that is a large chunk of the overall diff. (I also considered adding this field to `StatementKind::Assign` instead, but decided against that as we only actually need it for `Rvalue::Use`. I am not sure if this was the right call...) This neatly answers the question of when retags should occur, and handles cases like `*ptr = expr`. It avoids traversing values twice in Miri. It makes codegen's use of `noalias` sound wrt the actual MIR that it is working on. It also gives us a target semantics to evaluate MIR opts against. However, I did not carefully check all MIR opts -- in particular, GVN needs a thorough look under the new semantics; it currently can turn alias-correct code into alias-incorrect code. (But this PR doesn't make things any worse for normal compilation where the retag indicator is anyway ignored.) Another side-effect of this PR is that `-Zmiri-disable-validation` now also disables alias checking. It'd be nicer to keep them orthogonal but I find this an acceptable price to pay. - [rustc benchmark results](rust-lang#154341 (comment)) - [miri benchmark results](rust-lang#154341 (comment))
Remove unused spans from AttributeKind Recently I noticed some spans in diagnostic attributes were never used. I went through and checked the other variants too.
changelog: none r? ghost
…nishearth Clippy subtree update r? Manishearth
…RalfJung,ShoyuVanilla Rip out rustc_layout_scalar_valid_range_* attribute support And either removes tests for it or replaces the uses with pattern types. primarily fixes rust-lang#135996 fixes rust-lang#147761 fixes rust-lang#133652
…ust-lang#16952) fixes rust-lang/rust-clippy#16950 lint should now also trigger over async fns. changelog: [`needless_return_with_question_mark`]: fix false negative in async fn bodies.
*[View all comments](https://triagebot.infra.rust-lang.org/gh-comments/rust-lang/rust-clippy/pull/15745)* Add a check for some followed by filter. The program will suggest using the `then_some` method. changelog: [`filter_some`]: https://rust-lang.github.io/rust-clippy/master/index.html#filter_some
This proposes replacing calls to `.truncate(0)` for types in the standard library by `.clear()`, as the specialized version might be more efficient.
Update `askama` version to `0.16.0` New features and bugfixes. Full changelog is [here](https://github.com/askama-rs/askama/releases/tag/v0.16.0). r? @Urgau
This PR prepares the clippy testsuite to work with the new Cargo `build-dir` layout tracked in rust-lang/cargo#16807. Context: In rust-lang#155439 we attempt to enable the new Cargo `build-dir` layout in rust-lang/rust and I am working through all of the build failures caused by changing where the files are. Also note that the `ui_test` crate was bumped to include the corresponding fix in oli-obk/ui_test#368 With the changes in this PR the following test passes on my system. ```sh CARGO_UNSTABLE_BUILD_DIR_NEW_LAYOUT=true ./x test --stage 2 src/tools/clippy ``` changelog: none
fix: peeling of block. test: supply corner case tests of implicit_return. fix: pass body.value rather than block.expr. fix: special checking for implicit_return. fix: suggestion. refactor: flatten the code structure. refactor: remove lint submission in helper. refactor: call is_lint_allowed when necessary.
…rn` (rust-lang#16949) *[View all comments](https://triagebot.infra.rust-lang.org/gh-comments/rust-lang/rust-clippy/pull/16949)* changelog: fix [`non_canonical_clone_impl`] incompatibility with [`implicit_return`] Fixes rust-lang/rust-clippy#16945 <!-- TRIAGEBOT_START --> <!-- TRIAGEBOT_CONCERN-ISSUE_START --> > [!NOTE] > # Concerns (0 active) > > - ~~[Using `return *self` is not the canonical implementation of `Clone`](rust-lang/rust-clippy#16949 (comment) resolved in [this comment](rust-lang/rust-clippy#16949 (review)) > > *Managed by `@rustbot`—see [help](https://forge.rust-lang.org/triagebot/concern.html) for details.* <!-- TRIAGEBOT_CONCERN-ISSUE_END --> <!-- TRIAGEBOT_END -->
This proposes replacing calls to `.truncate(0)` for types in the standard library by `.clear()`, as the specialized version might be more efficient. changelog: [`manual_clear`]: new lint Closes rust-lang/rust-clippy#16615
r? @ghost changelog: none
…Manishearth Clippy subtree update r? Manishearth `Cargo.lock` update due to patch version bump in `ui_test`. One day early, as I won't have access to my laptop from tomorrow till Sunday
Add lint againts invalid runtime symbol definitions
This PR adds a deny-by-default lint againts invalid runtime symbol definitions, those runtime symbols are assumed and used by `core`[^1] and `rustc` with a specific definition.
We have had multiple reports of users tripping over `std` symbols (addressed in a future PR):
- [Why does `#[no_mangle] fn open() {}` make `cargo t` hang?](https://users.rust-lang.org/t/why-does-no-mangle-fn-open-make-cargo-t-hang/103423)
- [Pointer becomes misaligned in test with `no_mangle`](https://users.rust-lang.org/t/pointer-becomes-misaligned-in-test-with-no-mangle/126580)
This PR is a second attempt after rust-lang#146505, where T-lang had [some reservations](rust-lang#146505 (comment)) about a blanket lint that does not check the signature, which is now done with this PR, and about linting of `std` runtime symbols when std is not linked, which this PR omits by not including any std runtime symbols (for now).
## `invalid_runtime_symbol_definitions`
*(deny-by-default)*
The `invalid_runtime_symbol_definitions` lint checks the signature of items whose symbol name is a runtime symbols expected by `core`.
### Example
```rust,compile_fail
#[unsafe(no_mangle)]
pub fn memcmp() {} // invalid definition of the `memcmp` runtime symbol
```
```text
error: invalid definition of the runtime `memcmp` symbol used by the standard library
--> a.rs:2:1
|
4 | fn memcmp() {}
| ^^^^^^^^^^^
|
= note: expected `unsafe extern "C" fn(*const c_void, *const c_void, usize) -> i32`
found `fn()`
= help: either fix the signature or remove any attributes like `#[unsafe(no_mangle)]`, `#[unsafe(export_name = "memcmp")]`, or `#[link_name = "memcmp"]`
= note: `#[deny(invalid_runtime_symbol_definitions)]` on by default
```
### Explanation
Up-most care is required when defining runtime symbols assumed and used by the standard library. They must follow the C specification, not use any standard-library facility or undefined behavior may occur.
The symbols currently checked are `memcpy`, `memmove`, `memset`, `memcmp`, `bcmp` and `strlen`.
[^1]: https://doc.rust-lang.org/core/index.html#how-to-use-the-core-library
Privacy: move macros handling to early stage The patch moves effective visibility computation for macros from `rustc_privacy` to `rustc_resolve`. It will enable this optimization: rust-lang#156228. However, I found some problems with macro handling while I was doing this. The current implementation was written ~6 years ago and checks the reachability of a definition from a macro by nominal visibility. In general this is incorrect. For example, in the current implementation modules are not traversed if their nominal visibility is less then the nominal visibility of a module defining macro: https://github.com/rust-lang/rust/blob/29b7590130c83542a095cdf1323ed0f78eec2bb8/compiler/rustc_privacy/src/lib.rs#L618-L626 As a result, in order to compile code like `tests/ui/definition-reachable/auxiliary/field-method-macro.rs`. we have to additionally traverse types of adt fields: https://github.com/rust-lang/rust/blob/29b7590130c83542a095cdf1323ed0f78eec2bb8/compiler/rustc_privacy/src/lib.rs#L628-L638 This is a hack and the proper solution would be to check definitions with `EffectiveVisibilities::is_reachable`. I haven’t done this yet, as it would start to trigger many lints as more items become reachable. I think it’s better to leave the change to another commit. r? @petrochenkov
…laumeGomez rustdoc: Correctness & perf improvements to link-to-definition Rewrite the way we resolve type-dependent paths (incl. method calls) in `SpanMapVisitor`. Instead of trying to (re)find the enclosing body owner each time we encounter such a node, potentially calling `typeck_body` several times per body, keep track of the "active" `BodyId` in the visitor and cache the corresponding `TypeckResults`. The "active" `BodyId` is updated in `visit_nested_body` (add) and in `visit_item` (remove). This fixes a class of ICEs where we tried to look up the definition of type-relative paths located in non-body items nested in bodies, in the overarching body which is never correct. rustdoc@main has a hack to fix this issue for paths specifically found in assoc types in impls. Fixes rust-lang#147882. Fixes rust-lang#147057. Fixes rust-lang#155327. Fixes rust-lang#149089. Fixes rust-lang#150153. Fixes rust-lang#156418. Supersedes rust-lang#147911. --- `--generate-link-to-definition` is not enabled by default, so we can't perf this as is. Instead, I'm benching them against PR rust-lang#156348 in PR rust-lang#156355 using the same parent commit for the try-builds. For context, LTD is *extremely* costly as the perf run for the baseline PR shows: rust-lang#156348 (comment). "Absolute results": rust-lang#156355 (comment). "Relative results": [https://perf.rust-lang.org/compare.html?...](https://perf.rust-lang.org/compare.html?start=68c2bff6cb08a87e59246064a6a9f37098e22c3f&end=78d15de5525370011388c8f63847e873c4de14ed&stat=instructions%3Au). Excerpt of the relative results: Primary: Benchmark | Profile | Scenario | Backend | Target | % Change | Significance Threshold | Significance Factor ---|---|---|---|---|---|---|--- nalgebra-0.33.0 | doc | full | llvm | x64 | -5.82% | 0.20% | 29.09x diesel-2.2.10 | doc | full | llvm | x64 | -3.41% | 0.20% | 17.05x image-0.25.6 | doc | full | llvm | x64 | -2.03% | 0.20% | 10.16x regex-automata-0.4.8 | doc | full | llvm | x64 | -1.94% | 0.20% | 9.71x cargo-0.87.1 | doc | full | llvm | x64 | -1.64% | 0.20% | 8.21x clap_derive-4.5.32 | doc | full | llvm | x64 | -1.63% | 0.20% | 8.14x cranelift-codegen-0.119.0 | doc | full | llvm | x64 | -1.48% | 0.20% | 7.41x syn-2.0.101 | doc | full | llvm | x64 | -1.07% | 0.20% | 5.35x ripgrep-14.1.1 | doc | full | llvm | x64 | -0.96% | 0.20% | 4.78x stm32f4-0.15.1 | doc | full | llvm | x64 | -0.80% | 0.20% | 4.01x serde-1.0.219 | doc | full | llvm | x64 | -0.67% | 0.20% | 3.33x hyper-1.6.0 | doc | full | llvm | x64 | -0.66% | 0.20% | 3.30x eza-0.21.2 | doc | full | llvm | x64 | -0.55% | 0.20% | 2.74x unicode-normalization-0.1.24 | doc | full | llvm | x64 | -0.49% | 0.20% | 2.45x serde_derive-1.0.219 | doc | full | llvm | x64 | -0.45% | 0.20% | 2.26x typenum-1.18.0 | doc | full | llvm | x64 | -0.42% | 0.20% | 2.08x bitmaps-3.2.1 | doc | full | llvm | x64 | -0.32% | 0.20% | 1.61x Secondary: Benchmark | Profile | Scenario | Backend | Target | % Change | Significance Threshold | Significance Factor ---|---|---|---|---|---|---|--- deep-vector | doc | full | llvm | x64 | -23.96% | 0.20% | 119.78x large-workspace | doc | full | llvm | x64 | -2.16% | 0.20% | 10.81x deeply-nested-multi | doc | full | llvm | x64 | -1.35% | 0.20% | 6.75x serde-1.0.219-threads4 | doc | full | llvm | x64 | -0.67% | 0.20% | 3.33x wg-grammar | doc | full | llvm | x64 | -0.32% | 0.20% | 1.62x tt-muncher | opt | full | llvm | x64 | -0.31% | 0.20% | 1.56x nalgebra-0.33.0: Query/Function | Time (%) | Time (s) | Time delta | Executions | Executions delta | Hits | Hits delta ---|---|---|---|---|---|---|--- Totals | 109.57% | 2.185 | -0.138 (-5.9%) | 1614146 | -5602 (-0.3%) | 16983840 | -1436455 (-7.8%) typeck_root | 15.75% | 0.344 | -0.105 (-23.4%) | 1704 | -954 (-35.9%) | 393 | -6758 (-94.5%) ...|...|...|...|...|...|...|...
…nia-e Add `ChildExt::kill_process_group` ACP: rust-lang/libs-team#791 Tracking issue: rust-lang#156537
…Gomez use `deref_patterns` in `rustdoc` instead of `box_patterns` Part of rust-lang#156110.
rustbot
added
A-rustdoc-json
Contributor
Author
|
@bors r+ rollup=never p=5 |
Contributor
rust-bors
Bot
added
S-waiting-on-bors
Collaborator
|
The job Click to see the possible cause of the failure (guessed by this bot) |
rust-bors
Bot
added
S-waiting-on-author
Contributor
|
☔ The latest upstream changes (presumably #138995) made this pull request unmergeable. Please resolve the merge conflicts. This pull request was unapproved. |
rustbot
removed
the
S-waiting-on-author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
20 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Successful merges:
ChildExt::kill_process_group#156539 (AddChildExt::kill_process_group)deref_patternsinrustdocinstead ofbox_patterns#156540 (usederef_patternsinrustdocinstead ofbox_patterns)r? @ghost
Create a similar rollup