Optional else branch - #430
Conversation
|
Thanks for contributing to formality! :) |
bb21155 to
890c228
Compare
This comment has been minimized.
This comment has been minimized.
Guard hit now parses T mandatorily and wraps in Some; guard miss yields
None. Parsing Option <T> as a nonterminal forked into Some/None after
the keyword, making grammars lke 'if c {..} else {..}' ambiguous.
expect_keyword consumed the identifier even when it didn't match, so a $:guard miss corrupted the parse position when followed by an identifier (e.g. a let statement after an else-less if). Latent until now: existing $:where guards are always followed by punctuation. Fix expect_keyword_in the same way for consistency (its only current caller runs on a discarded clone, so no behavior change there).
Grammar: '"'"'if $condition $then_block $:else $else_block'"'"' with else_block: Option<Block>. Codegen and borrowck desugar None to an empty block, so the implicit else path carries the pre-if state to the join. to_rust omits the else when absent. Adds borrowck tests (init/move only in then-branch) and a mir_typeck test. Closes rust-lang#429
Add "if-no-else" rules to borrowck and codegen that reduce Stmt::If {
else_block: None } to an if with an empty else block.
Update tests
890c228 to
b4054a6
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
There was a problem hiding this comment.
Thanks! I am not comfortable with reviewing the parsing part, @nikomatsakis's input will be appreciated. For the rest, it looks good to me, I just have two questions.
| let text0 = self.current_text; | ||
| match self.identifier_like_string() { | ||
| Ok(ident) if &*ident == expected => Ok(()), | ||
| _ => Err(ParseError::at( | ||
| skip_whitespace(text0), | ||
| format!("expected `{}`", expected), | ||
| )), | ||
| _ => { | ||
| self.current_text = text0; | ||
| Err(ParseError::at( | ||
| skip_whitespace(text0), | ||
| format!("expected `{}`", expected), | ||
| )) | ||
| } |
There was a problem hiding this comment.
I don't understand the change here 🤔
why are we adding self.current_text = text0; when we already have let text0 = self.current_text; on top? isn't self.current_text = text0; just a no-op?
There was a problem hiding this comment.
identifier_like_string() is mutating self.current_text -- it moves the cursor past the identifier before we compare it to expected (which happens in the match). So the self.current_text = text0 is to return it to its original state when the match fails.
(I thought to include it so that if it is a desired behavior we do incorporate it if not, I can rework and get it out.)
There was a problem hiding this comment.
It's ok to not have this test in mir_typeck, we already have plenty of optional else branch test in tests/borrowck.
There was a problem hiding this comment.
That's fine, I'll remove it.
|
Thank you @tiif |
There was a problem hiding this comment.
I'm of two minds. On the one hand, I think these simplifications should take place elsewhere in a-mir-formality (i.e., be handled by an explicit desugaring step).
On the other hand, I think it's convenient, and fairly easy to support this kind of case,so maybe we should keep it. But what do you think about this simpler implementation? I'd be happier if it was "hidden" from the IR.
| condition: Expr, | ||
| then_block: Block, | ||
| else_block: Block, | ||
| else_block: Option<Block>, |
There was a problem hiding this comment.
| else_block: Option<Block>, | |
| else_block: ElseBlock, |
You know, we could do this:
#[term]
struct ElseBlock {
block: Block,
}
impl Default for ElseBlock {
fn default() -> Self { Block::empty() }
}and then the rest of the code basically just do else_block.block and doesn't have to know if it was present or not.
4 participants
What does this PR do?
Closes #429
How does it work, what questions do you have?
Change the behavior of an if statement requiring an else statement by making the else statement optional through changing the grammar definition for the If statement.
Create a small implementation for the Block in order to create an empty block for desugaring the empty else block.
Add a else_block variable in the judgement_fn! for the if rule to ensure that the block is not an option but either an empty block or a block in nll. Same thing to the codegen judgement_fn!
Change the macro since with the old macro, after else was consumed the parser tried Option which can succeed with either Some(block) or None, so
if c {} else {}had two valid parses ( the {} could be a separate block statement totally unrelated with else). Now the block is mandatory when else is seen.I've also changed the formality core parser regarding the
expect_keyword()andexpect_keyword_in()functions since they 'ate' the identifier even when not a keyword such thatif {} let z ..broke. So fix to restore position on mismatch. I'm not sure if that is right. Is it?I also did a textual
Option<T>detection in macro, Is it ok?AI disclosure