[PATCH] D69330: [AST] Add RecoveryExpr to retain expressions on semantic errors

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

I would prefer that we have dedicated tests for them rather than scattering the tests throughout the existing test suite (for example, consider adding `-Wno-unused` to the tests producing "result unused" warnings and adding a dedicated test).



================
Comment at: clang/include/clang/Sema/Sema.h:3568-3569
+  /// Corrects typos inside \p SubExprs.
+  ExprResult ActOnSemanticError(SourceLocation Begin, SourceLocation End,
+                                ArrayRef<Expr *> SubExprs);
+
----------------
Generally the `ActOn*` interface is invoked by the parser in response to syntactic constructs, so `ActOnSemanticError` is a surprising function name. Maybe `CreateRecoveryExpr` would be better. Is that too narrow for the intended semantics of this function?


================
Comment at: clang/lib/AST/TextNodeDumper.cpp:736-739
+void TextNodeDumper::VisitRecoveryExpr(const RecoveryExpr *Node) {
+  OS << " "
+     << "<recovery-expr>";
+}
----------------
You shouldn't need to print anything here. The class name is automatically printed for you.


================
Comment at: clang/lib/Sema/SemaExceptionSpec.cpp:1287
     // FIXME: Can any of the above throw?  If so, when?
     return CT_Cannot;
 
----------------
Should we return `CT_Dependent` for `RecoveryExprClass`, since we model it as being dependent?


================
Comment at: clang/lib/Sema/SemaExpr.cpp:17978
+  }
+  return RecoveryExpr::Create(Context, Begin, End, NoTypos);
+}
----------------
We shouldn't create these in code synthesis contexts (eg, during SFINAE checks in template instantiations).


================
Comment at: clang/lib/Sema/TreeTransform.h:9470
+ExprResult TreeTransform<Derived>::TransformRecoveryExpr(RecoveryExpr *E) {
+  return E;
+}
----------------
We should either transform the subexpressions or just return `ExprError()` here. With this approach, we can violate AST invariants (eg, by having the same `Expr` reachable through two different code paths in the same function body, or by retaining `DeclRefExpr`s referring to declarations from the wrong context, etc).


================
Comment at: clang/test/SemaCXX/builtin-operator-new-delete.cpp:94
   void *p = __builtin_operator_new(42, std::align_val_t(2)); // expected-error {{call to '__builtin_operator_new' selects non-usual allocation function}}
-  __builtin_operator_delete(p, std::align_val_t(2));         // expected-error {{call to '__builtin_operator_delete' selects non-usual deallocation function}}
+  __builtin_operator_delete((void*)0, std::align_val_t(2));         // expected-error {{call to '__builtin_operator_delete' selects non-usual deallocation function}}
 #endif
----------------
What happens to the original testcase here? I wouldn't expect the invalid call expression in the initializer of `p` to affect whether we diagnose uses of `p`.

(Nit: remove spaces to realign the comments here.)


================
Comment at: clang/test/SemaOpenCLCXX/address-space-references.cl:15
+  bar(1) // expected-error{{binding reference of type 'const __global unsigned int' to value of type 'int' changes address space}} \
+         // expected-error{{expected ';' after expression}}
 }
----------------
Consider just adding the missing semicolon, so that this test is only testing the thing it aims to test.


================
Comment at: clang/test/SemaTemplate/instantiate-init.cpp:102
     (void)sizeof(array_lengthof(Description<float>::data));
-
+    // expected-warning at +1{{expression result unused}}
     sizeof(array_lengthof( // expected-error{{no matching function for call to 'array_lengthof'}}
----------------
Add a cast-to-`void`, like on the previous line.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69330/new/

https://reviews.llvm.org/D69330