Use rebind instead of Binder::bind when possible #77685

jackh726 · 2020-10-08T00:04:21Z

These are really only the easy places. I just searched for Binder::bind and replaced where it straightforward.

lcnr

hmm, I personally feel like using old_bound_value.map_bound_ref(|_| result) makes this harder to read 🤔 I personally would prefer using something like ty::Binder::bind_with_depth(result, old_value.depth()). Maybe with depth just being a newtype for now. Not exactly sure how to best implement this for now

compiler/rustc_codegen_llvm/src/intrinsic.rs

compiler/rustc_trait_selection/src/traits/select/mod.rs

jackh726 · 2020-10-08T15:49:24Z

I do agree that the old_bound_value.map_bound_ref(|_| result) pattern is a bit harder to follow. In my other branch where I go all out, I actually have these changes as Binder::rebind(result, old_bound_value.bound_vars()). I would be definitely okay with bound_vars just returned a ZST that only Binder can construct. That "commits" us more down the field of "we're probably going to actually track bound vars at some point" whereas these changes could still make since in the long-term without actually tracking the bound vars.

Either way, I think we're in a transitionary period in terms of Predicate/PredicateAtom; in the long-term, I would imagine we would want something where we can do:

old_bound_value.map_bound_ref(|bound: PredicateAtom| match bound {
   ...
})

In Chalk, for example, we use skip_binders sparingly and only once is it used to "unwrap" the inner value where we later rebind.

I'm fine either way, just let me know which you prefer.

lcnr · 2020-10-08T16:14:17Z

have to think about this a bit more as I ended up confusing myself about the bigger plan here.

I do feel that even this current change only really makes sense if we want to track bound vars 🤔 am I missing an advantage of using old_bound_value.map_bound_ref(|_| result) over ty::Binder::bind(result)?

Using ty::Binder::rebind(result, old_binder.bound_vars()) has the advantage that if we do not want to go this way we can revert this change by removing that method and fixing all errors which seems nice.

I am not yet as familiar as I would want to be with the arguments for and against storing the number of bound vars in binders, is there a somewhat recent summary/discussion for this?

nikomatsakis · 2020-10-12T21:24:33Z

compiler/rustc_infer/src/infer/combine.rs

@@ -551,7 +551,8 @@ impl TypeRelation<'tcx> for Generalizer<'_, 'tcx> {
    where
        T: Relate<'tcx>,
    {
-        Ok(ty::Binder::bind(self.relate(a.skip_binder(), b.skip_binder())?))
+        let result = self.relate(a.skip_binder(), b.skip_binder())?;
+        Ok(a.map_bound(|_| result))


I'm not 100% sure this is correct. It seems to assume that a and b have equivalent bound variables and hence that the result will be expressed in terms of those variables. This is probably true -- if it's not, we should probably not be folding over binders but instead replacing them with inference variables and the like -- but I feel like I'd like an assertion or something.

So, from my other PR, I haven't didn't run into this being a problem. I'm not exactly sure what kind of assertion we could make here.

Okay, I am going to revert this here. Once we get to a point that we can have an assertion here, we almost certainly want to switch this over.

nikomatsakis · 2020-10-12T21:41:10Z

So, here is my take.

I agree that map_bound_ref(|_| foo) is ugly and it would be nice to use a pattern that's more easily removed later. I have to think about a better method name but I feel like map_bound_ref is not the one I would want, although the operation that's being performed here really is a map, just one that took place before the method was called.

I think the advantage of this style over ty::Binder::bind, in the absence of storing lists of binders, is just documentation. The idea is that it clearly specifies "the set of bound variables should have been preserved", and it tells you where that set of bound variables comes from. This is a "logical" set today. ty::Binder::bind, in contrast, doesn't give you any such information, so you don't know whether the new value may have produced a different set of bound variables.

This ambiguity is somewhat on full display here: for example, I think that the changes to folder are not obviously correct, and there may be other changes where potentially we're dropping references to bound variables or other things where the semantics would not be 100% the same.

But yeah I think the main advantage I see here is that when I am reading the code, if I see

let x = a.skip_binder();
let y = do_stuff(x);
ty::Binder::bind(y)

It's not as clear to me where the "binder" in ty::Binder::bind "comes from". But if I see:

let x = a.skip_binder();
let y = do_stuff(x);
y.with_binders(&x)

or

let x = a.skip_binder();
let y = do_stuff(x);
x.with_bound_value(y)

or something like that I know that y is referencing the variables from x. The question is what the right name is...

bors · 2020-10-13T01:24:32Z

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

lcnr

👍

there were some rebinds I didn't spend too much time looking into so I am completely sure that all of them are appropriate. I do however feel that even if we get one of them wrong we can detect that once we actually check something there.

lcnr · 2020-10-16T18:24:58Z

compiler/rustc_middle/src/ty/mod.rs

+        match self.kind() {
+            &PredicateKind::ForAll(binder) => binder,
+            &PredicateKind::Atom(atom) => {
+                assert!(!atom.has_escaping_bound_vars());


Suggested change

assert!(!atom.has_escaping_bound_vars());

debug_assert!(!atom.has_escaping_bound_vars());

i expect this method to be faily hot, so it might also make sense to mark this #[inline]

also, is there a reason you did not remove the _tcx argument here? Do you expect us to need it in the future?

Oops, definitely meant to make this a debug_assert. I will hold off on adding #[inline] for now, since other functions don't have it. It's probably worth a followup.

As for _tcx, I can remove that. I don't see any need for it in the future, here.

lcnr · 2020-10-16T18:33:56Z

compiler/rustc_middle/src/ty/sty.rs

+    /// current `Binder`. This should not be used if the new value *changes*
+    /// the bound variables. Note: the (old or new) value itself does not
+    /// necessarily need to *name* all the bound variables.
+    pub fn rebind<U>(&self, value: U) -> Binder<U> {


It might make sense to add a comment here which says that we do not yet track bound variables.

I am also kind of suprised that none of the methods on ty::Binder are marked as inline even though they are very frequently used and really simple. Might be interesting to check if that has a perf impact.

I will add a comment. As I said in other comment, leaving inline to another PR.

compiler/rustc_trait_selection/src/traits/select/mod.rs

compiler/rustc_typeck/src/check/dropck.rs

compiler/rustc_typeck/src/check/method/probe.rs

lcnr · 2020-10-16T19:22:14Z

we have been bitten by perf in the past here, so better safe than sorry

@bors try @rust-timer queue

if perf is clean r=me

rust-timer · 2020-10-16T19:22:15Z

Awaiting bors try build completion

bors · 2020-10-16T19:22:30Z

⌛ Trying commit f6a53b4 with merge 51ded3d7e6cd55a68668a891eb8f1150bd037be7...

bors · 2020-10-16T20:05:15Z

☀️ Try build successful - checks-actions, checks-azure
Build commit: 51ded3d7e6cd55a68668a891eb8f1150bd037be7 (51ded3d7e6cd55a68668a891eb8f1150bd037be7)

rust-timer · 2020-10-16T20:05:17Z

Queued 51ded3d7e6cd55a68668a891eb8f1150bd037be7 with parent a78a62f, future comparison URL.

rust-timer · 2020-10-17T01:08:35Z

Finished benchmarking try commit (51ded3d7e6cd55a68668a891eb8f1150bd037be7): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

lcnr · 2020-10-17T07:30:44Z

looking fairly neutral to me

@bors r+

bors · 2020-10-17T07:30:45Z

📌 Commit f6a53b4 has been approved by lcnr

bors · 2020-10-17T10:28:58Z

⌛ Testing commit f6a53b4 with merge 6f0ea29...

bors · 2020-10-17T12:36:25Z

☀️ Test successful - checks-actions, checks-azure
Approved by: lcnr
Pushing 6f0ea29 to master...

rust-highfive assigned lcnr Oct 8, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 8, 2020

jackh726 force-pushed the binder-map branch from 6a7582a to b5cc403 Compare October 8, 2020 01:34

lcnr reviewed Oct 8, 2020

View reviewed changes

compiler/rustc_codegen_llvm/src/intrinsic.rs Outdated Show resolved Hide resolved

compiler/rustc_trait_selection/src/traits/select/mod.rs Outdated Show resolved Hide resolved

nikomatsakis reviewed Oct 12, 2020

View reviewed changes

jackh726 added 2 commits October 16, 2020 12:58

Use map_bound(_ref) instead of Binder::bind when possible

dd5c9bf

Review comments

11d62aa

jackh726 force-pushed the binder-map branch from ceda94d to 7e830bf Compare October 16, 2020 18:04

map_bound_ref -> rebind

eba1027

jackh726 force-pushed the binder-map branch from 7e830bf to eba1027 Compare October 16, 2020 18:29

lcnr reviewed Oct 16, 2020

View reviewed changes

Review comments

f6a53b4

lcnr approved these changes Oct 16, 2020

View reviewed changes

jackh726 changed the title ~~Use map_bound(_ref) instead of Binder::bind when possible~~ Use rebind instead of Binder::bind when possible Oct 16, 2020

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 Oct 17, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 17, 2020

bors merged commit 6f0ea29 into rust-lang:master Oct 17, 2020

rustbot added this to the 1.49.0 milestone Oct 17, 2020

This was referenced Oct 17, 2020

[WIP] Add const qualifier in FnSig (for const fn pointers) #74553

Closed

Provide appropriate types in turbofish suggestions #76043

Closed

jackh726 deleted the binder-map branch October 17, 2020 17:19

jackh726 restored the binder-map branch December 15, 2020 19:38

jackh726 deleted the binder-map branch March 31, 2021 20:45

jackh726 restored the binder-map branch March 31, 2021 20:45

jackh726 deleted the binder-map branch March 31, 2021 20:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use rebind instead of Binder::bind when possible #77685

Use rebind instead of Binder::bind when possible #77685

jackh726 commented Oct 8, 2020

lcnr left a comment

jackh726 commented Oct 8, 2020

lcnr commented Oct 8, 2020 •

edited

Loading

nikomatsakis Oct 12, 2020

jackh726 Oct 12, 2020

jackh726 Oct 16, 2020

nikomatsakis commented Oct 12, 2020

bors commented Oct 13, 2020

lcnr left a comment

lcnr Oct 16, 2020

lcnr Oct 16, 2020

jackh726 Oct 16, 2020

lcnr Oct 16, 2020

jackh726 Oct 16, 2020

lcnr commented Oct 16, 2020

rust-timer commented Oct 16, 2020

bors commented Oct 16, 2020

bors commented Oct 16, 2020

rust-timer commented Oct 16, 2020

rust-timer commented Oct 17, 2020

lcnr commented Oct 17, 2020

bors commented Oct 17, 2020

bors commented Oct 17, 2020

bors commented Oct 17, 2020

	assert!(!atom.has_escaping_bound_vars());
	debug_assert!(!atom.has_escaping_bound_vars());

Use rebind instead of Binder::bind when possible #77685

Use rebind instead of Binder::bind when possible #77685

Conversation

jackh726 commented Oct 8, 2020

lcnr left a comment

Choose a reason for hiding this comment

jackh726 commented Oct 8, 2020

lcnr commented Oct 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented Oct 12, 2020

bors commented Oct 13, 2020

lcnr 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

Choose a reason for hiding this comment

lcnr commented Oct 16, 2020

rust-timer commented Oct 16, 2020

bors commented Oct 16, 2020

bors commented Oct 16, 2020

rust-timer commented Oct 16, 2020

rust-timer commented Oct 17, 2020

lcnr commented Oct 17, 2020

bors commented Oct 17, 2020

bors commented Oct 17, 2020

bors commented Oct 17, 2020

lcnr commented Oct 8, 2020 •

edited

Loading