[clang] Break statements with unknown named loops should interrupt constant evaluation

[tbaederr]

For this code:

constexpr int f1() {
  a: for (;;) {
    break azzz;
  }
  return 1;
}
static_assert(f1() == 1);

we currently emit:

array.cpp:102:11: error: 'break' label does not name an enclosing loop or 'switch'
  102 |     break azzz;
      |           ^
array.cpp:106:15: error: static assertion expression is not an integral constant expression
  106 | static_assert(f1() == 1);
      |               ^~~~~~~~~
array.cpp:101:15: note: constexpr evaluation hit maximum step limit; possible infinite loop?
  101 |   a: for (;;) {
      |               ^
array.cpp:106:15: note: in call to 'f1()'
  106 | static_assert(f1() == 1);
      |               ^~~~

(No godbolt link as it doesn't work there yet)

We emit an error for the break statement, but when evaluating the function call at compile time, we completely ignore the break statement altogether. We could handle it like a regular break; which would work in this case, but not for nested loops. We should simply interrupt constant evaluation of this function completely since we have already emitted an error for it.

CC @Sirraide

[llvmbot]

@llvm/issue-subscribers-clang-frontend

Author: Timm Baeder (tbaederr)

For this code: ```c++ constexpr int f1() { a: for (;;) { break azzz; } return 1; } static_assert(f1() == 1); ```

we currently emit:

array.cpp:102:11: error: 'break' label does not name an enclosing loop or 'switch'
  102 |     break azzz;
      |           ^
array.cpp:106:15: error: static assertion expression is not an integral constant expression
  106 | static_assert(f1() == 1);
      |               ^~~~~~~~~
array.cpp:101:15: note: constexpr evaluation hit maximum step limit; possible infinite loop?
  101 |   a: for (;;) {
      |               ^
array.cpp:106:15: note: in call to 'f1()'
  106 | static_assert(f1() == 1);
      |               ^~~~

(No godbolt link as it doesn't work there yet)

We emit an error for the break statement, but when evaluating the function call at compile time, we completely ignore the break statement altogether. We could handle it like a regular break; which would work in this case, but not for nested loops. We should simply interrupt constant evaluation of this function completely since we have already emitted an error for it.

CC @Sirraide

[tbaederr]

FunctionDecl 0x7d7eb1276270 <array.cpp:100:1, line:105:1> line:100:15 used constexpr f1 'int ()' implicit-inline
`-CompoundStmt 0x7d7eb12764b8 <col:20, line:105:1>
  |-LabelStmt 0x7d7eb1276450 <line:101:3, line:103:3> 'a'
  | `-ForStmt 0x7d7eb1276410 <line:101:6, line:103:3>
  |   |-<<<NULL>>>
  |   |-<<<NULL>>>
  |   |-<<<NULL>>>
  |   |-<<<NULL>>>
  |   `-CompoundStmt 0x7d7eb12763f8 <line:101:15, line:103:3>
  `-ReturnStmt 0x7d7eb12764a0 <line:104:3, col:10>
    `-IntegerLiteral 0x7d7eb1276478 <col:10> 'int' 1

We're not adding the break statement to the AST at all.

[Sirraide]

We emit an error for the break statement, but when evaluating the function call at compile time, we completely ignore the break statement altogether.

Er, it’s a bit sad that we try to evaluate a function that contains an error; I candidly wasn’t aware that we’d attempt to do that... is there some flag that needs to be set somewhere to stop it? Because evaluating a function that contains an error can’t possibly go well...

[Sirraide]

We're not adding the break statement to the AST at all.

Yeah, an invalid break statement is dropped; that’s also the case for unlabeled break/continue statements.

[tbaederr]

We emit an error for the break statement, but when evaluating the function call at compile time, we completely ignore the break statement altogether.

Er, it’s a bit sad that we try to evaluate a function that contains an error; I candidly wasn’t aware that we’d attempt to do that... is there some flag that needs to be set somewhere to stop it? Because evaluating a function that contains an error can’t possibly go well...

There is setInvalidDecl(), but as I know from #137773, doing that might regress other diagnostics since the invalid function does not take part in later name lookup.

[Sirraide]

Right, but I don’t think there is a good way of doing this as the break statement isn’t in the AST, and we don’t keep other invalid break/continue statements around either. You run into the same diagnostics if you try and write e.g. (https://godbolt.org/z/dGbs446vb):

constexpr int f1() {
  for (;;) {
    return a;
  }
  return 1;
}
static_assert(f1() == 1);

[erichkeane]

Right, but I don’t think there is a good way of doing this as the break statement isn’t in the AST, and we don’t keep other invalid break/continue statements around either. You run into the same diagnostics if you try and write e.g. (https://godbolt.org/z/dGbs446vb):

constexpr int f1() {
for (;;) {
return a;
}
return 1;
}
static_assert(f1() == 1);

This is mildly unfortunate as well, but at least we're consistent? It would be neat/interesting/etc for us to start using RecoveryExpr in these two cases rather than nullptr, then perhaps the constant evaluator can skip out then (this would obviously only work when RecoveryExpr is enabled).

WDYT?

[erichkeane]

One further option that @cor3ntin mentioned privately: I wonder if we can use typo-correction on the names in both the return/break cases and create the return/break/etc with that? It would end up giving us a reasonable constexpr result.

[Sirraide]

This is mildly unfortunate as well, but at least we're consistent? It would be neat/interesting/etc for us to start using RecoveryExpr in these two cases rather than nullptr, then perhaps the constant evaluator can skip out then (this would obviously only work when RecoveryExpr is enabled).

How would that work for labeled break/continue given that labels aren’t expressions?

I wonder if we can use typo-correction on the names in both the return/break cases and create the return/break/etc with that?

I’m not too familiar w/ typo-correction, but how would that work in situations where there’s nothing to correct to, e.g.

constexpr int f1() {
  for (;;) {
    break does_not_exist;
  }
  return 1;
}
static_assert(f1() == 1);

[erichkeane]

This is mildly unfortunate as well, but at least we're consistent? It would be neat/interesting/etc for us to start using RecoveryExpr in these two cases rather than nullptr, then perhaps the constant evaluator can skip out then (this would obviously only work when RecoveryExpr is enabled).

How would that work for labeled break/continue given that labels aren’t expressions?

The BreakStmt/ContinueStmt would be replaced with the RecoveryExpr. The Constexpr evaluator SHOULD give up when it runs into that.

I wonder if we can use typo-correction on the names in both the return/break cases and create the return/break/etc with that?

I’m not too familiar w/ typo-correction, but how would that work in situations where there’s nothing to correct to, e.g.

constexpr int f1() {
for (;;) {
break does_not_exist;
}
return 1;
}
static_assert(f1() == 1);

If there is nothing to correct, we'd switch to the RecoveryExpr as above :)

[Sirraide]

The BreakStmt/ContinueStmt would be replaced with the RecoveryExpr. The Constexpr evaluator SHOULD give up when it runs into that.

Ah, I see; that makes sense