Skip to content

Start using pattern types in libcore#136006

Merged
rust-bors[bot] merged 1 commit into
rust-lang:mainfrom
oli-obk:push-tzonluoyuwkq
Apr 14, 2026
Merged

Start using pattern types in libcore#136006
rust-bors[bot] merged 1 commit into
rust-lang:mainfrom
oli-obk:push-tzonluoyuwkq

Conversation

@oli-obk

@oli-obk oli-obk commented Jan 24, 2025 •
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor

View all comments

cc #135996

Replaces the innards of NonNull with *const T is !null.

This does affect LLVM's optimizations, as now reading the field preserves the metadata that the field is not null, and transmuting to another type (e.g. just a raw pointer), will also preserve that information for optimizations. This can cause LLVM opts to do more work, but it's not guaranteed to produce better machine code.

Once we also remove all uses of rustc_layout_scalar_range_start from rustc itself, we can remove the support for that attribute entirely and handle all such needs via pattern types

@rustbot

This comment was marked as outdated.

Sign in to view
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 24, 2025
@oli-obk oli-obk added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 24, 2025
@rust-log-analyzer

This comment has been minimized.

Sign in to view
compiler-errors
compiler-errors approved these changes Jan 24, 2025
View reviewed changes

@compiler-errors compiler-errors left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well r=me when/if it does get unblocked.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
scottmcm
scottmcm reviewed Jan 25, 2025
View reviewed changes
Comment thread library/core/src/num/niche_types.rs Outdated
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rustbot

rustbot commented Jan 27, 2025

Copy link
Copy Markdown
Collaborator

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@Veykril

Veykril commented Jan 27, 2025

Copy link
Copy Markdown
Member

Re rust-analyzer blocker, as it turns out to implement pattern types (to a degree that would unblock this) it comes down to the same architecture changes required as for rust-lang/rust-analyzer#7434 (which I am currently looking into), so that will take a bit.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rustbot

rustbot commented Jan 27, 2025

Copy link
Copy Markdown
Collaborator

Some changes occurred to the CTFE machinery

cc @rust-lang/wg-const-eval

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

@rust-log-analyzer

This comment has been minimized.

Sign in to view
RalfJung
RalfJung reviewed Jan 27, 2025
View reviewed changes
Comment thread compiler/rustc_const_eval/src/interpret/validity.rs Outdated
@oli-obk oli-obk mentioned this pull request Jan 27, 2025
Merged
@rustbot

This comment has been minimized.

Sign in to view
@oli-obk

oli-obk commented Apr 7, 2026

Copy link
Copy Markdown
Contributor Author

Should we split the compiler and library changes?

I already split everything out that I could. I tried splitting it further but box (and thus NonNull) is just so entangled between the compiler and libcore that it's really hard to do in a way that even some of the compiler changes can be landed and tested

@rust-log-analyzer

This comment has been minimized.

Sign in to view
wesleywiser
wesleywiser approved these changes Apr 7, 2026
View reviewed changes

@wesleywiser wesleywiser left a comment •
edited by rustbot
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one small comment, r=me

View changes since this review

Comment thread tests/ui/abi/compatibility.rs Outdated
test_abi_compatible!(zst_unit, Zst, ());
test_abi_compatible!(zst_array, Zst, [u8; 0]);
test_abi_compatible!(nonzero_int, NonZero<i32>, i32);
test_abi_compatible!(nonzero_int, pattern_type!(i32 is 1..=0x7FFF_FFFF), i32);

@wesleywiser wesleywiser Apr 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the pattern types be added as new cases rather than changing the test for NonZero?

@oli-obk oli-obk Apr 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, yea that was me being lazy. I changed minicore to support i32 for NonZero.

@wesleywiser

wesleywiser commented Apr 7, 2026

Copy link
Copy Markdown
Member

r? wesleywiser

@oli-obk

oli-obk commented Apr 8, 2026

Copy link
Copy Markdown
Contributor Author

@bors r=wesleywiser

@rust-bors

rust-bors Bot commented Apr 8, 2026

Copy link
Copy Markdown
Contributor

📌 Commit 2bbf86e has been approved by wesleywiser

It is now in the queue for this repository.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@JonathanBrouwer

JonathanBrouwer commented Apr 8, 2026

Copy link
Copy Markdown
Contributor

@bors r-
CI failed

@JonathanBrouwer

JonathanBrouwer commented Apr 8, 2026

Copy link
Copy Markdown
Contributor

@bors r-
github listen please?

@JonathanBrouwer

JonathanBrouwer commented Apr 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Oh huh this PR never got S-waiting-on-bors (which I was hoping to see removed) even though it was actually in the queue, github moment I guess

scottmcm
scottmcm reviewed Apr 9, 2026
View reviewed changes
Comment thread library/std/src/os/unix/io/tests.rs
scottmcm
scottmcm reviewed Apr 9, 2026
View reviewed changes

@scottmcm scottmcm left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking pretty good 🎉

I'm glad you found a way to get the change down to something pretty small -- when it's more minicore fixes than "real" changes it makes me happier.

EDIT: wow, I missed this had gotten a review. I should go sleep stop looking at code, apparently.

View changes since this review

Comment thread compiler/rustc_codegen_ssa/src/base.rs
let dst_ty = dst.layout.ty;
match (src_ty.kind(), dst_ty.kind()) {
(&ty::Pat(s, sp), &ty::Pat(d, dp))
if let (PatternKind::NotNull, PatternKind::NotNull) = (*sp, *dp) =>

@scottmcm scottmcm Apr 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unsure: does the specific kind of pattern really matter here? Would just

Suggested change
if let (PatternKind::NotNull, PatternKind::NotNull) = (*sp, *dp) =>
if sp == dp =>

be ok?

@oli-obk oli-obk Apr 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, if we ever end up with anything but a NotNull, I'm worried, so I like running into the bug! case at the end. Maybe in the future we will support other things, but I'm not sure they should go down the same code path here

Comment thread compiler/rustc_mir_transform/src/elaborate_box_derefs.rs
// While we can't project into `NonNull<_>` in a basic block
// due to MCP#807, this is debug info where it's fine.
// While we can't project into a pattern type in a basic block,
// this is debug info where it's fine.

@scottmcm scottmcm Apr 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's less about soundness and more that debug info doesn't need to worry about losing the validity restriction information. We don't want to load an i32 from an i32 is 1.. because that loses the !range metadata, but in debug info whatever.

@rustbot

rustbot commented Apr 9, 2026

Copy link
Copy Markdown
Collaborator

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@oli-obk

oli-obk commented Apr 13, 2026

Copy link
Copy Markdown
Contributor Author

@bors r=wesleywiser

@rust-bors

rust-bors Bot commented Apr 13, 2026

Copy link
Copy Markdown
Contributor

📌 Commit 834137a has been approved by wesleywiser

It is now in the queue for this repository.

@rust-bors

This comment has been minimized.

Sign in to view
@rust-bors

rust-bors Bot commented Apr 14, 2026

Copy link
Copy Markdown
Contributor

☀️ Test successful - CI
Approved by: wesleywiser
Duration: 3h 11m 5s
Pushing 338dff3 to main...

@github-actions

github-actions Bot commented Apr 14, 2026

Copy link
Copy Markdown
Contributor
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 17584a1 (parent) -> 338dff3 (this PR)

Test differences

Show 131 test diffs

131 doctest diffs were found. These are ignored, as they are noisy.

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard 338dff3e3a375cb4a3c68be825058c582262655a --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

  1. x86_64-msvc-ext1: 1h 41m -> 2h 18m (+35.8%)
  2. pr-check-1: 42m 2s -> 29m 10s (-30.6%)
  3. dist-apple-various: 1h 23m -> 1h 46m (+27.8%)
  4. i686-gnu-nopt-1: 2h 23m -> 2h (-15.9%)
  5. armhf-gnu: 1h 30m -> 1h 17m (-14.8%)
  6. aarch64-apple: 3h 14m -> 2h 47m (-13.5%)
  7. pr-check-2: 44m 5s -> 38m 8s (-13.5%)
  8. x86_64-gnu-nopt: 2h 13m -> 2h 31m (+13.5%)
  9. x86_64-gnu: 2h 24m -> 2h 5m (-13.3%)
  10. x86_64-rust-for-linux: 54m 55s -> 48m 21s (-12.0%)
How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

This was referenced Apr 14, 2026
Open
Open
Merged
@rust-timer

rust-timer commented Apr 14, 2026

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (338dff3): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

@rustbot label: -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 0.2% [0.2%, 0.2%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -0.3% [-0.3%, -0.3%] 1
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (primary -2.3%, secondary 0.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 5.9% [5.9%, 5.9%] 1
Improvements ✅
(primary)		 -2.3% [-4.1%, -0.4%] 3
Improvements ✅
(secondary)		 -2.6% [-4.2%, -1.0%] 2
All ❌✅ (primary) -2.3% [-4.1%, -0.4%] 3

Cycles

Results (secondary 3.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 3.1% [2.0%, 4.3%] 5
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 489.468s -> 489.33s (-0.03%)
Artifact size: 394.17 MiB -> 394.21 MiB (0.01%)

@scottmcm

scottmcm commented Apr 14, 2026

Copy link
Copy Markdown
Member

It landed 🎉 'grats oli!

@yshui yshui mentioned this pull request Apr 15, 2026
Closed
@matthiaskrgr matthiaskrgr mentioned this pull request Apr 24, 2026
Open
@theemathas theemathas mentioned this pull request Jun 6, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RalfJung RalfJung RalfJung left review comments

@programmerjake programmerjake programmerjake left review comments

@scottmcm scottmcm scottmcm left review comments

@tmiasko tmiasko tmiasko left review comments

@wesleywiser wesleywiser wesleywiser approved these changes

+2 more reviewers

@compiler-errors compiler-errors compiler-errors approved these changes

@Paladynee Paladynee Paladynee left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@wesleywiser wesleywiser

Labels

A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-test-infra-minicore Area: `minicore` test auxiliary and `//@ add-core-stubs` A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. O-unix Operating system: Unix-like O-wasi Operating system: Wasi, Webassembly System Interface O-wasm Target: WASM (WebAssembly), http://webassembly.org/ T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-clippy Relevant to the Clippy team. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-rustfmt Relevant to the rustfmt team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

1.97.0

Development

Successfully merging this pull request may close these issues.