Fix scope of else clause, and avoid duplication #14
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.
Hi. I really like this macro. I'm using it heavily. Rust needs lots of early return constructs (Maybe/Error monad, if you like to think of things that way) and this one composes nicely with
?
:-).I'm in the habit of rebinding the same variable name as I go through processing it. This works great in Rust. But there is a problem with
if_chain!
. Because theelse
clause is duplicated into theelse
of each nestedif
, it gets the wrong variable scope: bindings from earlierif let...;
clauses are visible to each copy of theelse
.Different copies of the else clause see different variable bindings! Usually these bindings have different types, so the result doesn't compile. See my new test case
let_rebinding
, which progressively unwraps; that does not compile without my substantive change.But if the
else
clause is generic enough or usesinto
or the rebindings are different values rather than different types, the code can compile and produce (almost certainly) wrong answers.The fix to this is to have only one copy of the
else
clause. This can be achieved by threading the return value of thethen
through as aSome
, and unwrapping that and substituting theelse
forNone
at the end.Intended effect of this change
For your edification and/or amusement I wrote a test case that compiles both before and after, and produces different results. This section of the diff demonstrates it:
ijackson@47fac88#r48559776
After this change:
else
always sees the original binding ofdunno
, ie, zero.if let Some(dunno) ...;
rebinding is not used, as indeed the code layout would seem to suggest.Undesirable side-effect, and possible alternatives
I have discovered that indirecting the return value through a
Some
in this way breaks some autoderef that otherwise occurs. So this can break existing code with new type errors. The fix is simple - the sprinkling of new*
(in thethen
clause). That makes this a clear semver break.A possible alternative would be to try to use
loop
. That can provide nonlocal exits. However,loop
cannot be used as a general facility for building this kind of thing mostly becausebreak ()
is not legal in afor
loop, which means that it is not possible to rethrow an innerbreak
. RFC2046 looks like it would help but wouldn't because it forbids innerbreak
s.Edited: Previously I discussed whether this was a breaking change under the assumption that the only effect on code which compiles today is the intended semantic change. However given this type problem this is a breaking change.
Example from my own code
In my actual code I have to write
if let Some(p) = piece;
so that I don't rebindpiece
.You may think the above is poor style :-), but there are other similar situations. Anything where any of the
if ...;
rebinds a variable from the environment which is used in theelse
.