Keep the IfStmt node even if the condition is invalid

[ogoffart]

Summary

This is quite useful for IDE's or other tools which would like to recover as
much as possible even if there are a few errors in the code, and discarding
the full 'if' bodies is unfortunate.

[milianw]

Is there still an error reported for the invalid condition?

Anyhow, from my POV this is excellent and should fix a bunch of issues I've seen when using clang-c in KDevelop. I never got around to investigating it, but it always was related to conditionals. I'm pretty sure this patch fixes it then.

@ogoffart: For Manuel to see the difference, could you share the before/after screenshots of your code browser? That makes the impact of this patch pretty clear.

[klimek]

+Richard, whom we need to validate AST changes to make sure they're benign.

[rsmith]

lib/Sema/SemaStmt.cpp

508–509

This will create an IfStmt with no ConditionExpr. That is not a valid AST construct, and it's not reasonable to expect all the downstream consumers of the AST to be able to cope with it. It's not hard to find parts of the codebase that will crash when presented with this:

lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp:1133
lib/Analysis/Consumed.cpp:1265
lib/Sema/AnalysisBasedWarnings.cpp:740

... and so on.

As the comment above suggests, you could create some sort of placeholder expression node here instead (either use something like an OpaqueValueExpr, or add a new Expr subclass to represent an erroneous expression -- the latter would probably be useful in many cases where we hit parse errors but can still produce some useful information to tools).

[ogoffart]

Are the analysis run if there is an error? I think the consumer should expect null condition anyway.

But i'll try to add an ErrorExpr anyway.

[ogoffart]

The updated patch uses OpaqueValueExpr

[rsmith]

lib/Sema/SemaStmt.cpp

505–506

Please add a comment here saying that we're creating this node for error recovery. Also, the type of the expression should be BoolTy, not VoidTy. Other than that, this looks fine to me.

[ogoffart]

commited as r249982. (I forgot to add the link to reviews.llvm.org in the commit message. I'll do it next time)

lib/Sema/SemaStmt.cpp

 Sema::ActOnIfStmt(SourceLocation IfLoc, FullExprArg CondVal, Decl *CondVar,
                   Stmt *thenStmt, SourceLocation ElseLoc,
                   Stmt *elseStmt) {
-  // If the condition was invalid, discard the if statement.  We could recover
-  // better by replacing it with a valid expr, but don't do that yet.
-  if (!CondVal.get() && !CondVar) {
-    getCurFunction()->setHasDroppedStmt();
-    return StmtError();
-  }
-
   ExprResult CondResult(CondVal.release());
 
   VarDecl *ConditionVar = nullptr;

     ConditionVar = cast<VarDecl>(CondVar);
     CondResult = CheckConditionVariable(ConditionVar, IfLoc, true);
     CondResult = ActOnFinishFullExpr(CondResult.get(), IfLoc);
-    if (CondResult.isInvalid())
-      return StmtError();
   }
   Expr *ConditionExpr = CondResult.getAs<Expr>();
-  if (!ConditionExpr)
-    return StmtError();
+  if (ConditionExpr) {
+    DiagnoseUnusedExprResult(thenStmt);
 
-  DiagnoseUnusedExprResult(thenStmt);
+    if (!elseStmt) {
+      DiagnoseEmptyStmtBody(ConditionExpr->getLocEnd(), thenStmt,
+                            diag::warn_empty_if_body);
+    }
 
-  if (!elseStmt) {
-    DiagnoseEmptyStmtBody(ConditionExpr->getLocEnd(), thenStmt,
-                          diag::warn_empty_if_body);
+    DiagnoseUnusedExprResult(elseStmt);
+  } else {
+    ConditionExpr = new (Context) OpaqueValueExpr(SourceLocation(),
+                                                  Context.VoidTy, VK_RValue);
   }
 
-  DiagnoseUnusedExprResult(elseStmt);
-
   return new (Context) IfStmt(Context, IfLoc, ConditionVar, ConditionExpr,
                               thenStmt, ElseLoc, elseStmt);
 }

test/Misc/ast-dump-invalid.cpp

 // CHECK-NEXT:         `-CXXUnresolvedConstructExpr {{.*}} <col:10, col:16> 'T'
 // CHECK-NEXT:           |-DeclRefExpr {{.*}} <col:13> 'T' lvalue ParmVar {{.*}} 'i' 'T'
 // CHECK-NEXT:           `-DeclRefExpr {{.*}} <col:16> 'T' lvalue ParmVar {{.*}} 'j' 'T'
+
+
+namespace TestInvalidIf {
+int g(int i) {
+  if (invalid_condition)
+    return 4;
+  else
+    return i;
+}
+}
+// CHECK: NamespaceDecl {{.*}} <{{.*}}> {{.*}} TestInvalidIf
+// CHECK-NEXT: `-FunctionDecl
+// CHECK-NEXT:   |-ParmVarDecl
+// CHECK-NEXT:   `-CompoundStmt
+// CHECK-NEXT:     `-IfStmt {{.*}} <line:25:3, line:28:12>
+// CHECK-NEXT:       |-<<<NULL>>>
+// CHECK-NEXT:       |-OpaqueValueExpr {{.*}} <<invalid sloc>> 'void'
+// CHECK-NEXT:       |-ReturnStmt {{.*}} <line:26:5, col:12>
+// CHECK-NEXT:       | `-IntegerLiteral {{.*}} <col:12> 'int' 4
+// CHECK-NEXT:       `-ReturnStmt {{.*}} <line:28:5, col:12>
+// CHECK-NEXT:         `-ImplicitCastExpr {{.*}} <col:12> 'int' <LValueToRValue>
+// CHECK-NEXT:           `-DeclRefExpr {{.*}} <col:12> 'int' lvalue ParmVar {{.*}} 'i' 'int'
+