This is an archive of the discontinued LLVM Phabricator instance.

[Parse] Use empty RecoveryExpr when if/while/do/switch conditions fail to parse
ClosedPublic

Authored by sammccall on Nov 12 2021, 4:03 AM.

Details

Summary

This allows the body to be parsed.
An special-case that would replace a missing if condition with OpaqueValueExpr
was removed as it's now redundant (unless recovery-expr is disabled).

For loops are not handled at this point, as the parsing is more complicated.

Diff Detail

Unit TestsFailed

Event Timeline

sammccall created this revision.Nov 12 2021, 4:03 AM
sammccall requested review of this revision.Nov 12 2021, 4:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 12 2021, 4:03 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
sammccall updated this revision to Diff 386797.Nov 12 2021, 4:06 AM

More conservative in changes to ActOnIfStmt

this looks great, some nits and questions.

clang/lib/Parse/ParseStmt.cpp
1194

what's the purpose of introducing MissingOK? It seems unclear to me, my understanding is

  • before the patch, ActionOnCondition always returned a valid ConditionResult for an empty ConditionResult. I assume this was conceptually a "MissingOK-true" case.
  • now in the patch, the MissingOK is propagated to ActOnCondition, which determines the returning result; ​The MissingOK is set to false in ParseSwitchStatement, ParseDoStatement below.

The only case I can think of is that we might fail to create a recoveryExpr (when the recovery-ast flag is off) for the condition.

1811

we should keep the Cond.isInvalid() branch, because the above typo-correction code path (Line1800) can hit it.

clang/lib/Sema/SemaExpr.cpp
19181

while here, we always return a valid condition (conceptually MissingOK is true), but in the patch, the default value of MissingOK parameter is false, this seems we are changing the default behavior for all call sides of ActOnCondition.

19183

nit: return MissingOK? ConditionResult() : ConditionError(), is easier to understand.

clang/test/AST/loop-recovery.cpp
2

file name: loop-recovery.cpp => ast-dump-loop-recovery.cpp

sammccall marked 3 inline comments as done.Dec 8 2021, 7:41 AM
sammccall added inline comments.
clang/lib/Parse/ParseStmt.cpp
1194

Your understanding is right: previously we were returning a "valid but empty" ConditionResult for a while loop with no condition.

We want functions like ParseParenExprOrCondition to be able to recover in this case, without having them "recover" legitimately missing for loop conditions.

It would be possible to have ParseParenExprOrCondition conditionally recover based on MissingOK, but since we're already using the tristate ConditionResult, using its error state seems cleaner.

The only case I can think of is that we might fail to create a recoveryExpr (when the recovery-ast flag is off) for the condition.

I'm not sure what you're saying here - what is that a case of?
Is there something you'd like me to do here?

clang/lib/Sema/SemaExpr.cpp
19181

That's right, MissingOK=false is a better (safer) default.

I've checked all callsites: in most we're passing it explicitly, in one the default is correct (if statement), and in the remaining cases the condition is synthetic and should never be null.

clang/test/AST/loop-recovery.cpp
2

Why? This directory is all tests of the AST, usually that's tested by building and then dumping it.

An ast-dump- prefix makes sense if what we're testing is the AST dumping functionality (e.g. its formatting) rather than *using* that functionality to test the AST itself.

sammccall updated this revision to Diff 392776.Dec 8 2021, 7:41 AM
sammccall marked an inline comment as done.

address comments

since we're now preserving more invalid code, we should check whether const-evaluator is cable of handling these newly-added invalid case.

I have played around it, it seems that if, do/while, while cases are already handled well. switch case need some work:

  • the ASTs among different cases (switch (;), switch(;;), switch(!!;)) are subtle
  • the follow case will result an value-dependent violation, the fix would be to handle value-dependent condition in EvaluateSwitch (clang/lib/AST/ExprConstant.cpp)
constexpr int s() {
  switch(!!) {
  }
  return 0;
}
void a() {
  constexpr int k = s();
}
clang/lib/Parse/ParseExprCXX.cpp
1965–1966

nit: update the doc comment, though the comment is already stale (missing CK).

clang/lib/Parse/ParseStmt.cpp
1194

I'm not sure what you're saying here - what is that a case of?
Is there something you'd like me to do here?

sorry, no action needed.

clang/test/AST/loop-recovery.cpp
41

can you add an init-statement switch case? e.g. switch (;), switch(;;), switch(!!;)

clang/test/Parser/cxx0x-attributes.cpp
155

This looks like a bogus diagnostic, but I think it is ok, as this is a poor-recovery case for clang already -- IIUC, the do-while statement range claims to the } on Line56.

this is a case which can be improved by our bracket-matching repair :)

sammccall marked 6 inline comments as done.Dec 30 2021, 6:14 AM

since we're now preserving more invalid code, we should check whether const-evaluator is cable of handling these newly-added invalid case.

  • the follow case will result an value-dependent violation, the fix would be to handle value-dependent condition in EvaluateSwitch (clang/lib/AST/ExprConstant.cpp)

Great catch, thanks!
I added constant-evaluation to ensure we don't crash for all the varieties of broken loops I could think of.
They're in SemaCXX/constexpr-function-recovery-crash.cpp which seems to be very similar. Maybe slightly misplaced as really we're testing AST/ExprConstant.

clang/test/Parser/cxx0x-attributes.cpp
155

Right. My understanding is the diagnostic is bogus if you put both missing )s before the semicolon, but clang is noticing them/implicitly inserting them before the }.

The code is horribly mangled here in any case, I'm not sure what diagnostic I'd want as a user really.

sammccall updated this revision to Diff 396662.Dec 30 2021, 6:15 AM
sammccall marked an inline comment as done.

Add constant-evaluation for broken switches, and a bunch of evaluation tests.
Address other comments.

Anything left to do here?
I think it would be nice to get this into 14 release.

hokein accepted this revision.Jan 10 2022, 12:10 AM

thanks, this looks good (sorry, I missed this.)

This revision is now accepted and ready to land.Jan 10 2022, 12:10 AM
This revision was landed with ongoing or failed builds.Jan 10 2022, 1:49 AM
This revision was automatically updated to reflect the committed changes.