Repository navigation
Rollup of 9 pull requests - #165023
Rollup of 9 pull requests#165023
Conversation
This avoids the unsoundness-hazard of using `&'ll mut` to represent an owning pointer that typically does not live for as long as its enclosing context.
The SimpleCx type contains references tied to the lifetime of the LLVM context, but the SimpleCx itself has a shorter lifetime.
This commit changes the type of LLVM context references from `&'ll Context` to a dedicated newtype struct `Context<'ll>` with an invariant lifetime brand. The current usage of `&'ll Context` is not sound, because covariance in `'ll` allows different contexts to be mixed in ways that the underlying bindings are not prepared to handle. (Most of the time, mixing contexts is unlikely to happen by accident, but switching to a more-sound API is good maintenance hygiene for these complex bindings.)
Lex and parse emoji in lifetimes, and disallow them in the parser with a hard error. Allow emoji to start a lifetime name even if they are not XID_Start. ``` error: lifetimes cannot contain emoji --> $DIR/emoji-in-lifetime.rs:1:22 | LL | fn bad_lifetime_name<'🐛🐛🐛family👨👩👧👦>( | ^^^^^^^^^^^^^^^^^^^^^ ``` Unify lifetime and identifier parsing. Use `bad_unicode_identifiers` for lifetimes.
During work on PR 164772 [1], I've noticed another tiny inconsistency in doc code examples: `HashMap`'s `Entry::insert_entry()`'s doc code example did not have an assertion where `BTreeMap`'s `Entry::insert_entry()`'s docs did. I've added an assertion where it was missing. [1] rust-lang#164772
This function is documented as allowed to continue, which is useful behavior during const eval. Split a new `resumable_breakpoint` intrinsic from `breakpoint` which has the behavior of continuing in Miri and CTFE.
The original `nonzero_ops` feature (rust-lang#84186) was stabilized before the `strict_*` methods on the regular integer types, so the inclusion of the `strict` methods on `NonZero` type was left as future work. Some users (see e.g. rust-lang#84186 (comment)) have recently requested these methods, and it's good to have them for consistency.
To avoid further duplication in `nonzero.rs`, this new set of tests is
generated by macros similar to those used for the primitive integer
types in `coretests/tests/num/{u,}int_macros.rs`. Many of the tests in
`nonzero.rs` are good candidates to be moved to the new
`nonzero_{u,}int_module` macros, but that's left as future work.
…s-tests, r=lqd Add tests for unsoundness involving alias liveness This adds a number of tests demonstrating unsoundness with alias liveness (some on stable w/ NLL, some only with Polonius Alpha). This reopens rust-lang#153215. See tests/ui/borrowck/alias-liveness/projection-hide-one-bound-153215.rs. This adds tests for rust-lang#160979, rust-lang#161038, rust-lang#151861, and rust-lang#163886. r? lqd
…tes, r=Mark-Simulacrum core: Constify `resumable_breakpoint` and re-export it in `std` This function is documented as allowed to continue, which is useful behavior during const eval. Split a new `resumable_breakpoint` intrinsic from `breakpoint` which has the behavior of continuing in Miri and CTFE.
cg_llvm: Use an invariant lifetime brand for LLVM contexts This PR changes the type of LLVM context references from `&'ll Context` to `&'ll Context<'ll>`, introducing an invariant lifetime brand. The current usage of `&'ll Context` is not sound, because covariance in `'ll` allows different contexts to be mixed in ways that the underlying bindings are not prepared to handle. (Most of the time, mixing contexts is unlikely to happen by accident, but switching to a more-sound API is good maintenance hygiene for these complex bindings.) In most code there is no practical difference between `&'ll Context<'ll>` and `&Context<'ll>`, but when storing the context in structures (e.g. SCx) it is helpful to be able to avoid an extra lifetime. --- A big chunk of the migration was done via a regex replace of `Context` with `Context<'ll>`, and then manually chasing down any resultant/remaining errors. I have tried to manually check each of the changes multiple times, forwards and backwards, but this is a pretty tedious process so hopefully I did catch everything that shouldn't be there. I intend to migrate various other related types as well, but just Context on its own is quite a lot to review already. There should be no change to compiler output.
x86-64-linux-gnu seems to have working f128 now There was some progress in llvm/llvm-project#44744 that seems to have helped. At least, I can compile and run Miri's `tests/pass/float.rs` on my system successfully. Let's see what CI says.
…lmann,Veykril Lex lifetimes as identifiers, recover from emoji and emit appropriate error Lex and parse emoji in lifetimes by using the identifier logic, and disallow them in the parser with a hard error. Allow emoji to start a lifetime name even if they are not XID_Start. ``` error: identifiers cannot contain emoji: `'🐛🐛🐛family👨👩👧👦` --> $DIR/emoji-in-lifetime.rs:1:22 | LL | fn bad_lifetime_name<'🐛🐛🐛family👨👩👧👦>( | ^^^^^^^^^^^^^^^^^^^^^ ``` Address rust-lang#141081 (but we could provide more information in the diagnostic, pointing at the specific chars, providing a link to the reference on identifiers and/or some other extra information).
Add `strict_` methods to `NonZero` The original `nonzero_ops` feature (rust-lang#84186) was stabilized before the `strict_*` methods on the regular integer types, so the inclusion of the `strict` methods on `NonZero` type was left as future work. Some users (see e.g. rust-lang#84186 (comment)) have recently requested these methods, and it's good to have them for consistency.
…r=mejrs Disable recovery when attribute parser shouldn't emit Removes some duplicate diagnostics. r? @mejrs
…8472 fd-views: enable test on Miri rust-lang/miri#5453 is fixed so we can enable this test. r? @the8472
…aapoalas docs: Nitpick change in `HashMap`'s `.insert_entry()` doc example During work on PR 164772 [1], I've noticed another tiny inconsistency in doc code examples: `HashMap`'s `Entry::insert_entry()`'s doc code example did not have an assertion where `BTreeMap`'s `Entry::insert_entry()`'s docs did. I've added an assertion where it was missing. [1] rust-lang#164772
rustbot
added
A-attributes
|
@bors force |
|
Unknown command "force". Run |
|
Oi, |
Ah I'm sorry, @jhpratt and I agreed we can do this to eachothers rollups to increase size. I do it to other people's rollups as well, such as @GuillaumeGomez's, and I haven't heard complaints before, so I wasn't aware of any policy. (I know that matthias doesn't like me touching his rollups, so I don't touch his). I won't do it to your rollups in the future, sorry again <3
Because there are now a lot more PRs in the queue that are waiting to be tested. I think it's especially defensible to increase rollup size because we need to optimize throughput more than latency right now, because of the |
This comment has been minimized.
This comment has been minimized.
|
No worries at all :) I was curious about the math. From a quick back of the napkin and assuming the average PR has a 95% pass rate, a 9 PR rollup has 0.95^9=63% chance of passing, a 24 PR rollup has a 29% chance of passing. Expected values of 5.7 and 7.0 PRs merging, respectively, so larger rollups are favorable in general. But this rollup with a known overall ~98% chance of passing (accounting for some failure right at the end of the Apple job) has an expected value of 8.8 PRs, which is highest of the three options. Of course that completely hinges on the 95% average pass rate which could be completely off base, better pass percentages favor large rollups and worse pass percentages favor smaller ones. And it doesn't account for things that get caught early or the spurious failures we've been enjoying... |
|
That's an interesting way of thinking about it, thanks for sharing! One thing that that calculation does not take into account, is that this only calculates the expected amount of PRs merged in the first "merge attempt", and the second "merge attempt" benefits from the information gained of the failed big rollup. Forgive me for completely getting nerdsniped, let's assume there are 24 PRs waiting in queue, of which 9 are known-good. Strategy 1: Merge the 9 (happy to assume 100% chance), then take a chance at merging the remaining 15. Strategy 2: Merge all 24, if that fails try again. This will only fail if at least 2 PRs are bad. The chance of <2 bad PRs is 66.1%I used https://www.omnicalculator.com/statistics/coin-flip-probability and entered:
That is surprisingly close, but strategy 1 still wins! |
|
Oh wait I fucked up the analysis of strategy 2. It should take into account that 9 PRs are known-good. |
|
Yeah, that was just the two of us agreeing privately because I was tired of leaving a comment every time lol. At the time it was largely the two of us who were doing rollups, so it covered most. I don't think it's ever been explicitly stated publicly. |
rust-bors
Bot
added
merged-by-bors
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 16d1ffa (parent) -> 41089d0 (this PR) Test differencesShow 2067 test diffsStage 1
Stage 2
(and 417 additional test diffs) Additionally, 1550 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 41089d07a0a2afc16adfe28a760d94d73d7298a4 --output-dir test-dashboardAnd 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 (41089d0): comparison URL. Overall result: ❌ regressions - no action needed@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 0.7%, secondary -0.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -0.0%, secondary 1.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.1%, secondary -0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 488.102s -> 490.666s (0.53%) |
|
📌 Perf builds for each rolled up PR:
parent commit: 16d1ffac17 In the case of a perf regression, run the following command with the SHAs of each PR you suspect might be the cause: |
|
Continuing the nerdsnipe, with the naive approach (not accounting for high-probability PRs), the maximum expected value is at It would be interesting to compute the stats on this. A sufficiently advanced bors could probably automatically create pretty good rollups. |
|
FYI I proposed a plan towards auto-rollup here: #t-infra/bors > Auto-rollup: The next big improvement of bors? @ 💬 I will likely start implementing this in the near future |
|
Just saying: love the maths, I had a good read. :) Also I love the idea to automate rollups! |
View all comments
Successful merges:
resumable_breakpointand re-export it instd#164608 (core: Constifyresumable_breakpointand re-export it instd)strict_methods toNonZero#164848 (Addstrict_methods toNonZero)HashMap's.insert_entry()doc example #165004 (docs: Nitpick change inHashMap's.insert_entry()doc example)r? @ghost
Create a similar rollup