Return early from a MIR Visitor
#65572
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rust-highfive
added
the
S-waiting-on-review
ecstatic-morse
changed the title
VisitorVisitor
cramertj
reviewed
Oct 18, 2019
This comment has been minimized.
This comment has been minimized.
ecstatic-morse
force-pushed
the
mir-visitor-break
branch
from
f4b360d to
121a445
Compare
This comment has been minimized.
This comment has been minimized.
ecstatic-morse
force-pushed
the
mir-visitor-break
branch
2 times, most recently
from
9d95568 to
44126d8
Compare
Centril
reviewed
Oct 19, 2019
| body: &'mir Body<'tcx>, | ||
| } | ||
|
|
||
| impl Visitor<'tcx, McfResult> for IsMinConstFn<'mir, 'tcx> { |
There was a problem hiding this comment.
I actually liked the fact that we didn't use a visitor for this code cause visitors are pretty fragile when it comes to adding new things to a construct. In particular, if the visitor gains new provided methods then chances are they won't be modified here and so we may accidentally allow new things on stable... how do we prevent this? cc @oli-obk
There was a problem hiding this comment.
I think the problem that you're describing is mostly solved with exhaustive matching on Rvalue, StatementKind etc. This type of defensive coding is a requirement for both when using Visitor and when manually recursing. Can you give an example where this kind of breakage would occur in one but not the other?
The downside of manual recursion is that it's easier to forget to call visit_operand or visit_place on something. The super methods put this code in one place, making it easier to audit for correctness. You don't have to, for example, match all the fields of a TerminatorKind::Call just so you can call your equivalent of visit_* for each one.
There was a problem hiding this comment.
I think the problem that you're describing is mostly solved with exhaustive matching on
Rvalue,StatementKindetc.
Sure, that works well when you are already matching on a specific enum and another variant is added, but what if a new enum or struct is added altogether?
Can you give an example where this kind of breakage would occur in one but not the other?
I suspect it is more likely with manual recursion that we would notice that a new field has been added for example...
The downside of manual recursion is that it's easier to forget to call
visit_operandorvisit_placeon something.
...but this is a fair point; maybe I'm overly paranoid. :)
There was a problem hiding this comment.
I think a better critique of Visitor APIs is that they encourage keeping a lot of ancillary state in the implementer instead of explicitly passing it via function parameters. This is exacerbated by by the fact that Visitor takes &mut self. Also, because the order of traversal is defined in the super methods, you need to check elsewhere to ensure that things are called in the order you expect. You can see both of these downsides in the way span is updated in the POC.
There was a problem hiding this comment.
Here's a suggestion: Let's modify the visitor documentation to suggest that the developer first add the new visitor method without a provided method; this will make errors all over the place and the developer will need to consider them all. Once they have been considered, the method can become provided.
There was a problem hiding this comment.
@ecstatic-morse I agree with that critique as well. :) I think SYB in the style of lens is neater for that reason, but we don't have generic closures unfortunately.
|
☔ The latest upstream changes (presumably #65570) made this pull request unmergeable. Please resolve the merge conflicts. |
ecstatic-morse
force-pushed
the
mir-visitor-break
branch
from
44126d8 to
a062593
Compare
ecstatic-morse
changed the title
VisitorVisitor
|
☔ The latest upstream changes (presumably #65804) made this pull request unmergeable. Please resolve the merge conflicts. |
ecstatic-morse
force-pushed
the
mir-visitor-break
branch
from
a062593 to
ef7bd53
Compare
| @@ -120,181 +120,182 @@ macro_rules! yrt { // try backwards | |||
|
|
|||
| macro_rules! make_mir_visitor { | |||
| ($visitor_trait_name:ident, $($mutability:ident)?) => { | |||
| pub trait $visitor_trait_name<'tcx> { | |||
| pub trait $visitor_trait_name<'tcx, R: TryHarder<Ok = ()> = ()> { | |||
There was a problem hiding this comment.
Can we use Result<T, E> with an associated error type that is sometimes ! instead? I guess that requires Ok wrapping, but seems overall simpler, and we could continue to use ??
There was a problem hiding this comment.
That would require modifying all implementers of Visitor right? I didn't want to do this until there was some interest in it. I agree that it would be nicer though.
There was a problem hiding this comment.
Yes, it would require that. We could of course do it not with an associated type but with a type parameter, just as you did here
trait Visitor<'tcx, E = !> { ... }that may be better anyway
|
The perf queue is pretty empty, so @bors try @rust-timer queue |
|
Awaiting bors try build completion |
bors
added a commit
that referenced
this pull request
Nov 2, 2019
Return early from a MIR `Visitor` This allows functions to return a `Result` from the `visit_*` methods on a MIR `Visitor`. Returning an `Err` will stop visitation. This PR is an exploration of an idea I floated [on Zulip](https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Early.20return.20from.20a.20MIR.20.60Visitor.60.3F). It shouldn't land as is. The diff for the `is_min_const_fn`change is easier to read if whitespace is ignored. r? @nikomatsakis
|
☀️ Try build successful - checks-azure |
|
Queued ca31dd2 with parent f39205b, future comparison URL. |
|
Finished benchmarking try commit ca31dd2, comparison URL. |
|
I was worried that the early return path would not be optimized out of the I'm inclined to close this, even though it would make some code more legible. WDYT @nikomatsakis? It is curious that the minimum instruction count is unchanged for the regressed benchmarks. |
|
@ecstatic-morse hmm, it's a bit disappointing to see those perf results, hmm? I wonder if that's an optimization waiting to be done. Maybe we should try to compare the perf effect of matching on |
|
I guess I'm indifferent. I'm not eager to lose 2%, but I do think early return is nice. It feels disappointing that we don't optimize this well. At minimum I would want to open an issue with a small example that maybe shows the problem and see if we can optimize it. |
|
There's a long-standing issue that destructing-and-rebuilding a I'm not sure if there's a tweak to the desugaring or |
This comment has been minimized.
This comment has been minimized.
JohnCSimon
removed
the
S-waiting-on-review
ecstatic-morse
force-pushed
the
mir-visitor-break
branch
from
ef7bd53 to
20ce01d
Compare
|
@bors try Bors is a bit flaky because of some chocolatey reasons, we can keep working as usual, we just don't get much merged right now. That issue will indeed cause a perf run to not show any improvement as far as I can tell. |
bors
added a commit
that referenced
this pull request
Dec 1, 2019
Return early from a MIR `Visitor` This allows functions to return a `Result` from the `visit_*` methods on a MIR `Visitor`. Returning an `Err` will stop visitation. This PR is an exploration of an idea I floated [on Zulip](https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Early.20return.20from.20a.20MIR.20.60Visitor.60.3F). It shouldn't land as is. The diff for the `is_min_const_fn`change is easier to read if whitespace is ignored. r? @nikomatsakis
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
@bors try- |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
ecstatic-morse
force-pushed
the
mir-visitor-break
branch
from
b0be6e0 to
0fca5d8
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
ecstatic-morse
force-pushed
the
mir-visitor-break
branch
from
0fca5d8 to
8464664
Compare
|
@bors try |
bors
added a commit
that referenced
this pull request
Dec 2, 2019
Return early from a MIR `Visitor` This allows functions to return a `Result` from the `visit_*` methods on a MIR `Visitor`. Returning an `Err` will stop visitation. This PR is an exploration of an idea I floated [on Zulip](https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Early.20return.20from.20a.20MIR.20.60Visitor.60.3F). It shouldn't land as is. The diff for the `is_min_const_fn`change is easier to read if whitespace is ignored. r? @nikomatsakis
|
☀️ Try build successful - checks-azure |
oli-obk
reviewed
Dec 3, 2019
| )), | ||
| // binops are fine on integers | ||
| Rvalue::BinaryOp(_, lhs, _) | Rvalue::CheckedBinaryOp(_, lhs, _) => { | ||
| // FIXME: do we need to type check `rhs`? |
There was a problem hiding this comment.
I don't think there are any binops where the rhs differs from the lhs and where the rhs is anything but an integer. So to answer the question: no
oli-obk
reviewed
Dec 3, 2019
| TerminatorKind::SwitchInt { discr, switch_ty: _, values: _, targets: _ } => { | ||
| check_operand(tcx, discr, span, def_id, body) | ||
| match base { | ||
| PlaceBase::Static(box Static { kind: StaticKind::Static, .. }) => { |
There was a problem hiding this comment.
I thought these were gone on master
|
Ping from Triage: any updates @ecstatic-morse @oli-obk? |
ecstatic-morse
added
S-blocked
|
Maybe @ecstatic-morse we should close this PR in the meantime, and move to an issue or something? (Or just close) |
ecstatic-morse
added
S-blocked-closed
and removed
S-blocked
jyn514
added
S-blocked
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This allows functions to return a
Resultfrom thevisit_*methods on a MIRVisitor. Returning anErrwill stop visitation. This PR is an exploration of an idea I floated on Zulip. It shouldn't land as is.The diff for the
is_min_const_fnchange is easier to read if whitespace is ignored.r? @nikomatsakis