Add syntax fixup for while loops

[palango]

Part of #12777

This is a first iteration to gather some feedback. In particular I'm not sure if the curly braces should be added here, but I couldn't get the test to work without them. Any hints welcome!

[flodiebold]

Thanks for looking into this!

We should only add the curly braces if they're missing, similar to how we do it for if. You can check whether the loop body is missing with it.loop_body().is_none(). I'd add two more test cases: while {} and while foo. (Possibly we actually parse while {} as having a condition, but no loop body? In that case adding {} is also fine though.)

[palango]

@flodiebold Thanks for the hints!

I added the test cases you asked for, but there's something weird happening in fixup_while_3 (run with cargo test -p hir-expand while_3 -- --nocapture). See lines 3 and 4, which reference the same code:

running 1 test
Found while
Found condition: Some(BlockExpr(BlockExpr { syntax: [email protected] }))
Found loop body: Some(BlockExpr { syntax: [email protected] })
thread 'fixup::tests::fixup_while_3' panicked at 'assertion failed: `(left == right)`
  left: `[SyntaxError("expected a block", 15..15)]`,
 right: `[]`: parse has syntax errors. parse tree:
[email protected]
  [email protected]
    [email protected] "fn"
    [email protected]
      [email protected] "foo"
    [email protected]
      [email protected] "("
      [email protected] ")"
    [email protected]
      [email protected]
        [email protected] "{"
        [email protected]
          [email protected] "while"
          [email protected]
            [email protected]
              [email protected] "{"
              [email protected] "}"
        [email protected] "}"
', crates/hir-expand/src/fixup.rs:452:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
test fixup::tests::fixup_while_3 ... FAILED

failures:

failures:
    fixup::tests::fixup_while_3

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 18 filtered out; finished in 0.00s

It seems like the curly braces are parsed both as the condition and the loop body.

[flodiebold]

Oh... yeah, currently for while {} we treat the block as both the condition and the loop body. We shouldn't do that. I think we'll need to replace the autogenerated condition method by something like the following:

    pub fn condition(&self) -> Option<Expr> {
        let mut exprs = support::children(self.syntax());
        let first = exprs.next();
        let second = exprs.next();
        if let Some(ast::Expr::BlockExpr(_)) = first {
            second.and(first)
        } else {
            first
        }
    }

i.e. if the first expression is a block expression, only consider it as the condition if there's also a second one (i.e. the loop body). This would live in an impl ast::WhileExpr in node_ext.rs, and we'd need to modify the generate_nodes function in sourcegen_ast.rs to skip this method, similar to how we already skip the HasLoopBody trait. I think we'll actually have a similar problem with for...

@lnicola @Veykril any better ideas?

[Veykril]

Hmm, I would expect with while {} that the {} is the condition, since that is the first expression, and as such that we have no body (hypthetically speaking, ignoring the current implementation). Though I guess it could make sense to special case blocks in this regard, not sure? And yes for loops will have the same problem.

[lnicola]

Yeah, but it's less common to use blocks for the condition (oh, hello there!), isn't it? In practice I suspect it doesn't matter.

CC #11565, #11561.

[flodiebold]

It certainly doesn't matter for this, but it might matter for some other features like completion? In any case, I think treating the {} as the condition wouldn't make this significantly easier anyway.

[Veykril]

Ye, that's the thing. It's barely used so assuming the condition is what's missing helps more in the general case I'd say let's special case it.

In any case, we'll have to add iterable (for loops) and condition (while loops) here https://github.com/Veykril/rust-analyzer/blob/c8ff70e924efe3adba39ac5dc2a93dbd94bde5a4/crates/syntax/src/tests/sourcegen_ast.rs#L671-L685

This will also require condition for IfExpr and MatchGuard to be implemented manually then. (We should revisit the source gen script for this at some point, it's getting a bit messy I think)

[palango]

Some of the things you've been discussing are over my head for now and should probably be split out in a new issue.

Would it be ok to remove the test case in question and wait until the underlying issue is fixed?

[Veykril]

#12890 should address the syntax tree problems, that is we no longer should be treating a single expression as both the condition and the body

[palango]

#12890 should address the syntax tree problems, that is we no longer should be treating a single expression as both the condition and the body

Thanks for the quick fix, that solved the problem.

[Veykril]

Thanks!
@bors r+

[bors]

📌 Commit c16e4f2 has been approved by Veykril

It is now in the queue for this repository.

[bors]

⌛ Testing commit c16e4f2 with merge 113f1db...

[bors]

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 113f1db to master...