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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked 13 inline comments as done.
ilya-biryukov added a comment.

In D69330#1746137 <https://reviews.llvm.org/D69330#1746137>, @rsmith wrote:

> 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).


Most updated tests (including those with more "result unused" warnings) are actually not intended to test recovery expressions, they just happen to produce different results now and need to be updated.
The only new dedicated tests here are `clang/test/AST/ast-dump-recovery.cpp` and `clang/test/Index/getcursor-recovery.cpp`.

Could technically move them into the same directory, but wanted to make sure I got your point first. Could you elaborate on what testing strategy you'd prefer? Also eager for suggestions on more things I could test, but not sure what kinds of tests people normally add when adding new expressions. Any advice here is highly appreciated.



================
Comment at: clang/include/clang/Sema/Sema.h:3568-3569
+  /// Corrects typos inside \p SubExprs.
+  ExprResult ActOnSemanticError(SourceLocation Begin, SourceLocation End,
+                                ArrayRef<Expr *> SubExprs);
+
----------------
rsmith wrote:
> 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?
`CreateRecoveryExpr` looks good, the only additional logic in the function is not producing recovery expressions in the C mode. But I would expect this to go away in the future.


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


================
Comment at: clang/lib/Sema/SemaExpr.cpp:17978
+  }
+  return RecoveryExpr::Create(Context, Begin, End, NoTypos);
+}
----------------
rsmith wrote:
> We shouldn't create these in code synthesis contexts (eg, during SFINAE checks in template instantiations).
Good point, thanks. Added the corresponding check. Also made sure that we always typo-correct in arguments before checking whether we can produce recovery expressions.

I don't think we have any code that calls into `CreateRecoveryExpression` (formerly `ActOnSemanticError`) from SFINAE context, all calls are currently in the parser. Not 100% certain this does not happen with `-fdelayed-template-parsing`, though, so ended up returning null instead of asserting we're not in SFINAE context.




================
Comment at: clang/lib/Sema/TreeTransform.h:9470
+ExprResult TreeTransform<Derived>::TransformRecoveryExpr(RecoveryExpr *E) {
+  return E;
+}
----------------
rsmith wrote:
> 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).
Thanks, I just blindly copied what TypoExpr was doing without considering the consequences here.


Added the code to rebuild `RecoveryExpr`.

Any good way to test this?


================
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
----------------
rsmith wrote:
> 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.)
Sorry, that was a leftover from previous revisions. This was here by accident, I reverted this.

A bit more context on what happened here: if we mark any decl as invalid, references to those variables do not parse into `DeclRefExpr`, the corresponding sema function returns `null` instead.
This leads to diagnostics like this being lost. In one of my attempts, I marked more decls as invalid and, in turn, to these tests failing.


================
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}}
 }
----------------
rsmith wrote:
> Consider just adding the missing semicolon, so that this test is only testing the thing it aims to test.
Good point, thanks.


================
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'}}
----------------
rsmith wrote:
> Add a cast-to-`void`, like on the previous line.
Done. Do you think it's also worth suppressing same errors in other files (e.g. `cxx1z-copy-omission.cpp`) in a similar manner?

Or would you prefer adding `-Wno-unused-result` for tests like `cxx1x-copy-omission.cpp`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69330