Make functions passed to BoundVarReplacer be optional

[jackh726]

This means we can reuse the bound vars when we don't care to change them

[rust-highfive]

r? @varkor

(rust-highfive has picked a reviewer for you, use r? to override)

[jackh726]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit d22289d3ce6cbdb1ba66053d03ef23e96985fc50 with merge 4f15e3f1454acbf63f87a260b0b68a36cb27206a...

[bors]

☀️ Try build successful - checks-actions
Build commit: 4f15e3f1454acbf63f87a260b0b68a36cb27206a (4f15e3f1454acbf63f87a260b0b68a36cb27206a)

[rust-timer]

Queued 4f15e3f1454acbf63f87a260b0b68a36cb27206a with parent 56f74c5, future comparison URL.

[rust-timer]

Finished benchmarking try commit (4f15e3f1454acbf63f87a260b0b68a36cb27206a): 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
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

[jackh726]

Perf is mixed. Maybe slightly clean on instruction counts, but heavily mixed for max-rss and wall-time. I'll leave it up to a reviewer to decide if benefits of "None means no change" outweighs the slight overhead in cognitive complexity.

[varkor]

I wonder whether something like this would be more readable:

fn fold_ty(&mut self, t: Ty<'tcx>) -> Ty<'tcx> {
    match *t.kind() {
        ty::Bound(debruijn, bound_ty) if debruijn == self.current_index => {
            if let Some(fld_t) = self.fld_t.as_mut() {
                let ty = fld_t(bound_ty);
                return ty::fold::shift_vars(self.tcx, &ty, self.current_index.as_u32());
            }
        }
        _ if t.has_vars_bound_at_or_above(self.current_index) => return t.super_fold_with(self),
    }
    t
}

(and similarly for the other methods).

[jackh726]

Ooh, I like that

[varkor]

I don't have a strong opinion on whether this is more or less readable. If you personally find it more readable, then I'm happy. It would be nice to simplify the methods as in my comment above, though.

[jackh726]

I don't have a strong opinion on whether this is more or less readable. If you personally find it more readable, then I'm happy. It would be nice to simplify the methods as in my comment above, though.

I personally think the idea of "None means you don't want to change anything" is nice, even if there isn't really any additional perf benefit (I imagine we probably just don't end up with enough bound tys or consts for it to matter.)

[varkor]

r=me when CI passes.

[jackh726]

@bors r=varkor

[bors]

📌 Commit ba27cae has been approved by varkor

[bors]

⌛ Testing commit ba27cae with merge 3b8e3d885585effc745b57c774aabf56bff6c4a9...

[bors]

💔 Test failed - checks-actions

[rust-log-analyzer]

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[jackh726]

Looks spurious @bors retry

[bors]

⌛ Testing commit ba27cae with merge 04ae501...

[bors]

☀️ Test successful - checks-actions
Approved by: varkor
Pushing 04ae501 to master...