Make `EarlyBinder`'s inner value private #112006

kylematsuda · 2023-05-26T20:26:56Z

Currently, EarlyBinder(T)'s inner value is public, which allows implicitly skipping the binder by indexing into the tuple struct (i.e., x.0). @lcnr suggested making EarlyBinder's inner value private so users are required to explicitly call skip_binder (#105779 (comment)) .

This PR makes the inner value private, adds EarlyBinder::new for constructing a new instance, and replaces uses of x.0 with x.skip_binder() (or similar). It also adds some documentation to EarlyBinder::skip_binder explaining how to skip the binder of &EarlyBinder<T> to get &T now that the inner value is private (since previously we could just do &x.0).

r? @lcnr

rustbot · 2023-05-26T20:27:04Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in rustc_ty_utils::consts.rs

cc @BoxyUwU

bors · 2023-05-27T16:09:51Z

☔ The latest upstream changes (presumably #112016) made this pull request unmergeable. Please resolve the merge conflicts.

jackh726 · 2023-05-28T16:27:15Z

r=me with rebase :)

@bors p=1

…errors

jackh726 · 2023-05-28T17:49:11Z

@bors r+

bors · 2023-05-28T17:49:12Z

📌 Commit c29c212 has been approved by jackh726

It is now in the queue for this repository.

bors · 2023-05-28T18:04:56Z

⌛ Testing commit c29c212 with merge 1c53407...

compiler-errors

some places probably could've been subst_identity rather than skip_binder, but i guess it doesn't matter

src/librustdoc/clean/blanket_impl.rs

compiler-errors · 2023-05-28T18:49:35Z

compiler/rustc_mir_transform/src/shim.rs

+    let mut sig = if let Some(sig_substs) = sig_substs {
+        sig.subst(tcx, &sig_substs)
+    } else {
+        sig.skip_binder()


subst_identity i feel

I would expect this to be no_bound_vars() 🤔 though that may break with polymorphization rn?

This type of pattern was always sketchy to me. Adding the EarlyBinder type makes it clear why.

I'm not sure how polymorphization works; seems like this could be a byproduct of it.

Yeah, this panics with no_bound_vars().unwrap() unfortunately.

From reading the polymorphization section in the rustc_dev_guide, it looks like polymorphization uses ty::Param to represent unused generics. So after polymorphization sig may appear to needs_subst() because of the presence of ty::Params. I'm sure it's more complicated than that in practice, but is that sort of the right way to think about this?

Yeah, this panics with no_bound_vars().unwrap() unfortunately.

From reading the polymorphization section in the rustc_dev_guide, it looks like polymorphization uses ty::Param to represent unused generics. So after polymorphization sig may appear to needs_subst() because of the presence of ty::Params. I'm sure it's more complicated than that in practice, but is that sort of the right way to think about this?

yeah, it is exactly that, I want to move polymorphization to bound vars by changing instances to use ty::Binder and then using bound vars of params (as params are conceptionally just a different representation of bound vars or placeholders, depending on context 😁)

I think using ty::Param for polymorphization will end up causing issues like the above in the future.

that would be an option, I don't like it too much because ty::Erased should be equal to itself and for<T, U> my_function<T, U> should be different from for<T> my_function<T, T>. While we may not necessarily end up with these cases in practice it still feels a bit dangerous

Both for<T, U> my_function<T, U> and for<T> my_function<T, T> should be equal to be able to codegen a single instance, right? Polymorphization already makes sure that it won't erase types that actually matter for codegen.

Both for<T, U> my_function<T, U> and for<T> my_function<T, T> should be equal to be able to codegen a single instance, right?

yes, the issue is that if you polymorphize to bound vars, these two are different, but if you polymorphize to erased, then both end up as my_function<erased, erased> which should be equal to itself

You can't observe the difference at runtime, right? If you did, polymorphization should reject the candidates. Rustc passes unnamed_addr to LLVM, so just comparing the function pointers can return true already if after optimizations the function bodies are identical.

for<T, U> my_function<T, U> and for<T> my_function<T, T> probably won't happen in practice but you could imagine

fn check_type_ids<T, U, V>() -> u8 { if TypeId::of::<T>() == TypeId::of::<U>() { 0 } else if TypeId::of::<T>() == TypeId::of::<U>() { 1 } else { 2 } }

we end up with for<Eq, DontCare> check_type_ids<Eq, DontCare, Eq> and for<DontCare, Eq> check_type_ids<DontCare, Eq, Eq>

fn check_type_ids<Eq, DontCare, Eq>() -> u8 { 0 } fn check_type_ids<DontCare, Eq, Eq>() -> u8 { if TypeId::of::<T>() == TypeId::of::<U>() { 0 } else { 1 } }

this isn't a great example which is why I said "While we may not necessarily end up with these cases in practice it still feels a bit dangerous". I am worried about having yet another global invariant which is difficult to sensibly check

bors · 2023-05-28T20:36:12Z

☀️ Test successful - checks-actions
Approved by: jackh726
Pushing 1c53407 to master...

rust-timer · 2023-05-28T23:59:33Z

Finished benchmarking commit (1c53407): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

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

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

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

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

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

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 646.98s -> 646.44s (-0.08%)

…piler-errors Cleanup some `EarlyBinder::skip_binder()` -> `EarlyBinder::subst_identity()` fix some incorrect `skip_binder()`'s as identified in rust-lang#112006 (review) r? `@compiler-errors` `@lcnr` `@jackh726` (hope it's alright to just tag everyone who commented 😅)

…piler-errors Cleanup some `EarlyBinder::skip_binder()` -> `EarlyBinder::subst_identity()` fix some incorrect `skip_binder()`'s as identified in rust-lang#112006 (review) r? ``@compiler-errors`` ``@lcnr`` ``@jackh726`` (hope it's alright to just tag everyone who commented 😅)

rustbot assigned lcnr May 26, 2023

kylematsuda added 3 commits May 28, 2023 10:44

Replace EarlyBinder(x) with EarlyBinder::new(x)

03534ac

Make EarlyBinder's inner value private; and fix all of the resulting …

c40e9cc

…errors

Add documentation on skip_binder on how to get &T from &EarlyBinder<T>

c29c212

kylematsuda force-pushed the earlybinder-private branch from a69c002 to c29c212 Compare May 28, 2023 17:02

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 28, 2023

compiler-errors reviewed May 28, 2023

View reviewed changes

This comment was marked as duplicate.

Sign in to view

bors added the merged-by-bors This PR was explicitly merged by bors. label May 28, 2023

bors merged commit 1c53407 into rust-lang:master May 28, 2023

rustbot added this to the 1.72.0 milestone May 28, 2023

lcnr removed the WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) label May 31, 2023

kylematsuda deleted the earlybinder-private branch June 1, 2023 23:51

kylematsuda mentioned this pull request Jun 2, 2023

Cleanup some EarlyBinder::skip_binder() -> EarlyBinder::subst_identity() #112220

Merged

celinval mentioned this pull request Jun 21, 2023

Upgrade rust toolchain to nightly-2023-06-20 model-checking/kani#2551

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `EarlyBinder`'s inner value private #112006

Make `EarlyBinder`'s inner value private #112006

kylematsuda commented May 26, 2023

rustbot commented May 26, 2023

bors commented May 27, 2023

jackh726 commented May 28, 2023

jackh726 commented May 28, 2023

bors commented May 28, 2023

bors commented May 28, 2023

compiler-errors left a comment

compiler-errors May 28, 2023

lcnr Jun 1, 2023

jackh726 Jun 1, 2023

kylematsuda Jun 2, 2023

lcnr Jun 5, 2023 •

edited

Loading

lcnr Jun 5, 2023 •

edited

Loading

bjorn3 Jun 5, 2023

lcnr Jun 5, 2023

bjorn3 Jun 5, 2023

lcnr Jun 5, 2023 •

edited

Loading

This comment was marked as duplicate.

bors commented May 28, 2023

rust-timer commented May 28, 2023

Make EarlyBinder's inner value private #112006

Make EarlyBinder's inner value private #112006

Conversation

kylematsuda commented May 26, 2023

rustbot commented May 26, 2023

bors commented May 27, 2023

jackh726 commented May 28, 2023

jackh726 commented May 28, 2023

bors commented May 28, 2023

bors commented May 28, 2023

compiler-errors left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcnr Jun 5, 2023 • edited Loading

Choose a reason for hiding this comment

lcnr Jun 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcnr Jun 5, 2023 • edited Loading

Choose a reason for hiding this comment

This comment was marked as duplicate.

bors commented May 28, 2023

rust-timer commented May 28, 2023

Overall result: ✅ improvements - no action needed

Make `EarlyBinder`'s inner value private #112006

Make `EarlyBinder`'s inner value private #112006

lcnr Jun 5, 2023 •

edited

Loading

lcnr Jun 5, 2023 •

edited

Loading

lcnr Jun 5, 2023 •

edited

Loading