-
Notifications
You must be signed in to change notification settings - Fork 13.6k
Rollup of 7 pull requests #145236
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
Rollup of 7 pull requests #145236
Conversation
UEFI networking APIs do support vectored read/write. While the types for UDP4, UDP6, TCP4 and TCP6 are defined separately, they are essentially the same C struct. So we can map IoSlice and IoSliceMut to have the same binary representation. Since all UEFI networking types for read/write are DSTs, `IoSlice` and `IoSliceMut` will need to be copied to the end of the transmit/receive structures. So having the same binary representation just allows us to do a single memcpy instead of having to loop and set the DST. Signed-off-by: Ayush Singh <[email protected]>
…ibutes explicitly
…caused a move error When encountering a move error involving a closure because the captured value isn't `Copy`, and the obligation comes from a bound on a type parameter that requires `Fn` or `FnMut`, we point at it and explain that an `FnOnce` wouldn't cause the move error. ``` error[E0507]: cannot move out of `foo`, a captured variable in an `Fn` closure --> f111.rs:15:25 | 14 | fn do_stuff(foo: Option<Foo>) { | --- ----------- move occurs because `foo` has type `Option<Foo>`, which does not implement the `Copy` trait | | | captured outer variable 15 | require_fn_trait(|| async { | -- ^^^^^ `foo` is moved here | | | captured by this `Fn` closure 16 | if foo.map_or(false, |f| f.foo()) { | --- variable moved due to use in coroutine | help: `Fn` and `FnMut` closures require captured values to be able to be consumed multiple times, but an `FnOnce` consume them only once --> f111.rs:12:53 | 12 | fn require_fn_trait<F: Future<Output = ()>>(_: impl Fn() -> F) {} | ^^^^^^^^^ help: consider cloning the value if the performance cost is acceptable | 16 | if foo.clone().map_or(false, |f| f.foo()) { | ++++++++ ```
Do not point at macro invocation which expands to an inference error. Avoid the following: ``` error[E0308]: mismatched types --> $DIR/does-not-have-iter-interpolated.rs:12:5 | LL | quote!($($nonrep)*); | ^^^^^^^^^^^^^^^^^^^ | | | expected `HasIterator`, found `ThereIsNoIteratorInRepetition` | expected due to this | here the type of `has_iter` is inferred to be `ThereIsNoIteratorInRepetition` ```
…nieu Constify remaining traits/impls for `const_ops` Tracking issue: rust-lang#143802 This is split into two commits for ease of reviewability: 1. Updates the `forward_ref_*` macros to accept multiple attributes (in anticipation of needing `rust_const_unstable` attributes) and also *require* attributes in these macros. Since the default attribute only helps for the initial implementations, it means it's easy to get wrong for future implementations, as shown for the saturating implementations which were incorrect before. 2. Actually constify the traits/impls. A few random other notes on the implementation specifically: * I unindented the attributes that were passed to the `forward_ref_*` macro calls because in some places rustfmt wanted them to be unindented, and in others it was allowed because they were themselves inside of macro bodies. I chose the consistent indenting even though I (personally) think it looks worse. ---- As far as the actual changes go, this constifies the following additional traits: * `Neg` * `Not` * `BitAnd` * `BitOr` * `BitXor` * `Shl` * `Shr` * `AddAssign` * `SubAssign` * `MulAssign` * `DivAssign` * `RemAssign` * `BitAndAssign` * `BitOrAssign` * `BitXorAssign` * `ShlAssign` * `ShrAssign` In terms of constified implementations of these traits, it adds the reference-forwarded versions of all the arithmetic operators, which are defined by the macros in `library/core/src/internal_macros.rs`. I'm not going to fully enumerate these because we'd be here all day, but sufficed to say, it effectively allows adding an `&` to one or both sides of an operator for primitives. Additionally, I constified the implementations for `Wrapping`, `Saturating`, and `NonZero` as well, since all of them forward to already-const-stable methods. (potentially via intrinsics, to avoid extra overhead) There are three "non-primitive" types which implement these traits, listed below. Note that I put "non-primitive" in quotes since I'm including `Wrapping`, `Saturating`, and `NonZero`, which are just wrappers over primitives. * `Duration` (arithmetic operations) * `SystemTime` (arithmetic operations) * `Ipv4Addr` (bit operations) * `Ipv6Addr` (bit operations) Additionally, because the methods on `SystemTime` needed to make these operations const were not marked const, a separate tracking issue for const-stabilising those methods is rust-lang#144517. Stuff left out of this PR: * `Assume` (this could trivially be made const, but since the docs indicate this is still under heavy design, I figured I'd leave it out) * `Instant` (this could be made const, but cannot reasonably be constructed at constant time, so, isn't useful) * `SystemTime` (will submit separate PR) * SIMD types (I'm tackling these all at once later; see rust-lang/portable-simd#467) <!-- TRIAGEBOT_START --> <!-- TRIAGEBOT_CONCERN-ISSUE_START --> > [!NOTE] > # Concerns (0 active) > > - ~~[May break Clippy](rust-lang#143949 (comment) resolved in [this comment](rust-lang#143949 (comment)) > > *Managed by ```@rustbot`—see`` [help](https://forge.rust-lang.org/triagebot/concern.html) for details.* <!-- TRIAGEBOT_CONCERN-ISSUE_END --> <!-- TRIAGEBOT_END -->
…=Amanieu document assumptions about `Clone` and `Eq` traits Most standard library collections break if `Clone` has a non-standard implementation which violates `x.clone() == x`. [Here](https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=b7fc6dfa8410cbb673eb8d38393d81de) the resulting broken behaviour of different collections is shown. I originally created an issue at rust-lang/hashbrown#629, but the conclusion there was that `x.clone()` resulting in an object that compares equal to the original one is probably a very universal assumption. However, this assumption is (to my knowledge) not documented anywhere. I propose to make this assumption explicit in the `Clone` trait documentation. The property that seems the most reasonable to me is the following: When implementing both `Clone` and `PartialEq`, then ```text x == x -> x.clone() == x ``` is expected to hold. This way, when also implementing `Eq`, it automatically follows that `x.clone() == x` has to hold, which should be enough for the collections to not break. At the same time, the property also works for the "normal" elements of a type with `PartialEq`. For the wording, I tried to follow the [`Hash` and `Eq`](https://doc.rust-lang.org/std/hash/trait.Hash.html#hash-and-eq) documentation. As I am fairly new to Rust, it might well be that this property cannot be generally expected – it seems reasonable to me, but any counter-examples or critique, both content- and wording-wise, would be very welcome. If the property turns out to be too general, I would suggest to at least document the assumption of `x.clone() == x` for the collections somehow. An additional thought of mine: If it is indeed generally expected that `x == x -> x.clone() == x`, then, for the sake of completeness, one could also define that `x != x -> y != y for y = x.clone()` should hold, i.e., that an object that did not compare equal to itself before cloning, should also not compare equal to itself afterwards.
…olasbishop std: sys: io: io_slice: Add UEFI types UEFI networking APIs do support vectored read/write. While the types for UDP4, UDP6, TCP4 and TCP6 are defined separately, they are essentially the same C struct. So we can map IoSlice and IoSliceMut to have the same binary representation. Since all UEFI networking types for read/write are DSTs, `IoSlice` and `IoSliceMut` will need to be copied to the end of the transmit/receive structures. So having the same binary representation just allows us to do a single memcpy instead of having to loop and set the DST. cc ``@nicholasbishop``
Point at the `Fn()` or `FnMut()` bound that coerced a closure, which caused a move error When encountering a move error involving a closure because the captured value isn't `Copy`, and the obligation comes from a bound on a type parameter that requires `Fn` or `FnMut`, we point at it and explain that an `FnOnce` wouldn't cause the move error. ``` error[E0507]: cannot move out of `foo`, a captured variable in an `Fn` closure --> f111.rs:15:25 | 14 | fn do_stuff(foo: Option<Foo>) { | --- ----------- move occurs because `foo` has type `Option<Foo>`, which does not implement the `Copy` trait | | | captured outer variable 15 | require_fn_trait(|| async { | -- ^^^^^ `foo` is moved here | | | captured by this `Fn` closure 16 | if foo.map_or(false, |f| f.foo()) { | --- variable moved due to use in coroutine | help: `Fn` and `FnMut` closures require captured values to be able to be consumed multiple times, but an `FnOnce` consume them only once --> f111.rs:12:53 | 12 | fn require_fn_trait<F: Future<Output = ()>>(_: impl Fn() -> F) {} | ^^^^^^^^^ help: consider cloning the value if the performance cost is acceptable | 16 | if foo.clone().map_or(false, |f| f.foo()) { | ++++++++ ``` Fix rust-lang#68119, by pointing at `Fn` and `FnMut` bounds involved in move errors.
…ig-method-invoke-inside-parse-functions, r=Kobzol Make config method invoke inside parse use dwn_ctx This PR is part of a series of config refactorings. It removes calls from config methods to solid functions defined in `config.rs`. After this, we will remove the default dependencies in the config. r? ```@Kobzol```
…oshtriplett Tweak spans providing type context on errors when involving macros Do not point at macro invocation multiple times when we try to add span labels mentioning what type each expression has, which is unnecessary when the error is at a macro invocation.
@bors r+ rollup=never p=5 |
☀️ Test successful - checks-actions |
📌 Perf builds for each rolled up PR:
previous master: 21a19c297d In the case of a perf regression, run the following command for each PR you suspect might be the cause: |
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing 21a19c2 (parent) -> 53af067 (this PR) Test differencesShow 2112 test diffsStage 1
Stage 2
Additionally, 2110 doctest diffs were found. These are ignored, as they are noisy. Job group index
Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 53af067bb0b4edf9b5394e5f9b60942974b9fbc2 --output-dir test-dashboard And then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
Finished benchmarking commit (53af067): comparison URL. Overall result: ❌ regressions - please read the text belowOur benchmarks found a performance regression caused by this PR. Next Steps:
@rustbot label: +perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -1.4%, secondary 1.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 2.1%, secondary 5.9%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 464.264s -> 463.403s (-0.19%) |
@rust-timer build 69636ab |
This comment has been minimized.
This comment has been minimized.
@rust-timer build d5ad7be |
@rust-timer ping |
@rust-timer build d5ad7be |
Oh, you actually queued multiple commits on one PR. That's not really supported 😆 You have to do it on separate PRs, there's always max. one concurrent try build per PR. |
Finished benchmarking commit (69636ab): comparison URL. Overall result: ❌ regressions - please read the text belowInstruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -1.4%, secondary 1.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 2.4%, secondary 6.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 464.264s -> 466.204s (0.42%) |
Looks like perf regressions are from #143949. |
Successful merges:
const_ops
#143949 (Constify remaining traits/impls forconst_ops
)Clone
andEq
traits #144330 (document assumptions aboutClone
andEq
traits)Fn()
orFnMut()
bound that coerced a closure, which caused a move error #144558 (Point at theFn()
orFnMut()
bound that coerced a closure, which caused a move error)assert!
s #145228 (Remove unnecessary parentheses inassert!
s)r? @ghost
@rustbot modify labels: rollup
Create a similar rollup