Page MenuHomePhabricator

[AST] Add RecoveryExpr to retain expressions on semantic errors
AcceptedPublic

Authored by ilya-biryukov on Oct 23 2019, 2:47 AM.

Details

Reviewers
rsmith
Summary

Normally clang avoids creating expressions when it encounters semantic
errors, even if the parser knows which expression to produce.

This works well for the compiler. However, this is not ideal for
source-level tools that have to deal with broken code, e.g. clangd is
not able to provide navigation features even for names that compiler
knows how to resolve.

The new RecoveryExpr aims to capture the minimal set of information
useful for the tools that need to deal with incorrect code:

  • source range of the expression being dropped,
  • subexpressions of the expression.

We aim to make constructing RecoveryExprs as simple as possible to
ensure writing code to avoid dropping expressions is easy.

Producing RecoveryExprs can result in new code paths being taken in the
frontend. In particular, clang can produce some new diagnostics now and
we aim to suppress bogus ones based on Expr::containsErrors.

We deliberately produce RecoveryExprs only in the parser for now to
minimize the code affected by this patch. Producing RecoveryExprs in
Sema potentially allows to preserve more information (e.g. type of an
expression), but also results in more code being affected. E.g.
SFINAE checks will have to take presence of RecoveryExprs into account.

Initial implementation only works in C++ mode, as it relies on compiler
postponing diagnostics on dependent expressions. C and ObjC often do not
do this, so they require more work to make sure we do not produce too
many bogus diagnostics on the new expressions.

See documentation of RecoveryExpr for more details.

This change is based on https://reviews.llvm.org/D61722

Diff Detail

Event Timeline

ilya-biryukov created this revision.Oct 23 2019, 2:47 AM
nridge added a subscriber: nridge.Oct 26 2019, 5:43 PM

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

clang/include/clang/Sema/Sema.h
3703–3704

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?

clang/lib/AST/TextNodeDumper.cpp
736–739 ↗(On Diff #226105)

You shouldn't need to print anything here. The class name is automatically printed for you.

clang/lib/Sema/SemaExceptionSpec.cpp
1287

Should we return CT_Dependent for RecoveryExprClass, since we model it as being dependent?

clang/lib/Sema/SemaExpr.cpp
18042

We shouldn't create these in code synthesis contexts (eg, during SFINAE checks in template instantiations).

clang/lib/Sema/TreeTransform.h
9562

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 DeclRefExprs referring to declarations from the wrong context, etc).

clang/test/SemaCXX/builtin-operator-new-delete.cpp
94 ↗(On Diff #226105)

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

clang/test/SemaOpenCLCXX/address-space-references.cl
15

Consider just adding the missing semicolon, so that this test is only testing the thing it aims to test.

clang/test/SemaTemplate/instantiate-init.cpp
102

Add a cast-to-void, like on the previous line.

ilya-biryukov marked 13 inline comments as done.Tue, Nov 19, 10:26 AM

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.

clang/include/clang/Sema/Sema.h
3703–3704

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.

clang/lib/Sema/SemaExceptionSpec.cpp
1287

Makes total sense, thanks!

clang/lib/Sema/SemaExpr.cpp
18042

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.

clang/lib/Sema/TreeTransform.h
9562

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?

clang/test/SemaCXX/builtin-operator-new-delete.cpp
94 ↗(On Diff #226105)

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.

clang/test/SemaOpenCLCXX/address-space-references.cl
15

Good point, thanks.

clang/test/SemaTemplate/instantiate-init.cpp
102

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?

ilya-biryukov marked 7 inline comments as done.
  • Rename ActOnSemanticError to CreateRecoveryExpr
  • Do not print anything for recovery expr in TextNodeDumper
  • canThrow(RecoveryExpr) now returns CT_Dependent
  • Store Expr* instead of Stmt* in RecoveryExpr
  • Transform subexpressions of RecoveryExpr in TreeTransform
  • Do not produce RecoveryExpr in SFINAE contexts
  • Updated some test cases
rsmith accepted this revision.Tue, Nov 19, 1:16 PM

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?

For the tests where you added expected-warning {{unused}} that are testing things other than parser recovery, I would instead add -Wno-unused or a cast to void. expects unrelated to the intent of tests make them less readable and more fragile.

I think it's fine to use test/AST/ast-dump-recovery.cpp as the primary test for the various forms of recovery that you added here. I've not thought of anything better, at least :)

clang/lib/Sema/TreeTransform.h
9562

Hmm, testing that the old failure mode doesn't happen seems a bit tricky; any test for that is going to be testing second-order effects of the code under test, so will be fragile. But you can test that we're substituting into the RecoveryExpr easily enough: put some expression for which substitution will fail into a context where we'll build a RecoveryExpr inside a context that we'll TreeTransform. For example, maybe:

template<typename T> int *p = &void(T::error); // should produce "cannot take address of void"
int *q = p<int>; // should produce "'int' cannot be used prior to '::'" in instantiation
This revision is now accepted and ready to land.Tue, Nov 19, 1:16 PM
  • Do not correct typos when creating recovery expr
  • Simplify StmtPrinter::VisitRecoveryExpr
  • Add -Wno-used and remove extra expect-warning directives
ilya-biryukov marked an inline comment as done.Wed, Nov 20, 8:09 AM

@rsmith, could you also take a look at D65591?
It's really important to have the containsError() check in this patch that marks decls with undeduced types as invalid. Otherwise, they would have a "dependent auto type" and the constant evaluation code will fail when attempting to get sizes of those, e.g. when they're used inside sizeof().
There's actually a test in clang that crashes if this is not done (clang/test/SemaCXX/lambda-expressions.cpp)

I have also realized doing typo-correction in CreateRecoveryExpr is a bad idea, the contract for typo correction is to run it at particular points (e.g. on full expressions) and it's unclear why recovery expressions should be another case for this context. All code attempting to create recovery expressions should already handle typo correction properly, so there's no need to cross these two features.