Page MenuHomePhabricator

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

Authored by hokein on Oct 23 2019, 2:47 AM.

Details

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
3890–3891

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
1366

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

clang/lib/Sema/SemaExpr.cpp
18427

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

clang/lib/Sema/TreeTransform.h
9877

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 ↗(On Diff #226105)

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 ↗(On Diff #226105)

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

ilya-biryukov marked 13 inline comments as done.Nov 19 2019, 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
3890–3891

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
1366

Makes total sense, thanks!

clang/lib/Sema/SemaExpr.cpp
18427

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
9877

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 ↗(On Diff #226105)

Good point, thanks.

clang/test/SemaTemplate/instantiate-init.cpp
102 ↗(On Diff #226105)

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.Nov 19 2019, 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
9877

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.Nov 19 2019, 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.Nov 20 2019, 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.

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

Fix compilation after rebase

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

hokein updated this revision to Diff 250765.Tue, Mar 17, 7:23 AM
hokein added a subscriber: hokein.
  • rebase to master, fix more broken tests
    • preferType in CodeCompletTest
    • openMP diagnostics target_update_from_messages.cpp, target_update_to_messages.cpp
  • RecoveryExpression RValue => LValue
  • Fix a NULL assertion crash on gnu conditional operator ?:
hokein commandeered this revision.Tue, Mar 17, 7:29 AM
hokein added a reviewer: ilya-biryukov.
bmahjour removed a subscriber: bmahjour.Tue, Mar 17, 9:23 AM

all prerequisite patches have been landed, it is time to land this patch now. Would be nice to have a second look before committing it +@sammccall.

Fantastic :-)

@rsmith and others: would appreciate feedback on giving this a feature-flag (to decouple from LangOpts.CPlusPlus):

  • whether it's OK to have one
  • whether it should be cc1-only
  • the name -f[no]recovery-ast
clang/include/clang/AST/Expr.h
5925

AIUI this should be "for now" with the goal of eliminating the use of template dependence concepts, right?

5940

nit: NumStmts -> NumSubExprs?

clang/lib/AST/ComputeDependence.cpp
460

Probably worth echoing with a FIXME: drop type+value+instantiation once Error is sufficient to suppress checks.

clang/lib/Sema/SemaExpr.cpp
18423

I think we should strongly consider a LangOption with an associated flag. (e.g. LangOptions.RecoveryAST, -f[no]recovery-ast).
If we're going to pay tho cost of letting expr creation fail, we might as well use it
Use cases:

  • Control rollout: we can check this in without (yet) flipping the flag on for all tools at once, if desired. If we flip the default and it causes problems for particular tools, we can turn it off for that tool rather than rolling the whole thing back.
  • turn on and off in lit tests to precisely test behavior and avoid dependence on defaults
  • allow incremental work on recovery in !CPlusPlus mode

If this makes sense to you, I'd suggest setting the default to off in this patch (and including some tests that pass -frecovery-ast), and then immediately following up with a patch that flips the default to on-for-C++.
This separation makes like easier for everyone if turning this on breaks something.

A bunch of the updates to existing tests would be deferred until that patch.

clang/lib/Sema/TreeTransform.h
9877

@hokein I don't think this test was added yet.

hokein updated this revision to Diff 251966.Mon, Mar 23, 2:03 AM
hokein marked 7 inline comments as done.

address some comments, add missing tree-transform testcase.

hokein added inline comments.Mon, Mar 23, 2:05 AM
clang/include/clang/AST/Expr.h
5925

yeah, I think so.

clang/lib/Sema/SemaExpr.cpp
18423

+1, I think this is a good idea -- particularly letting us incrementally working on it without breaking existing tools, I was highly suspected that this patch will fail internal tests during build copping.

I will wait for a while for other feedback @rsmith before making the actual change.

clang/lib/Sema/TreeTransform.h
9877

good catch, done.

martong removed a subscriber: martong.Mon, Mar 23, 6:05 AM
hokein updated this revision to Diff 252046.Mon, Mar 23, 8:02 AM
  • add -frecovery-ast cc1 option
  • defer changes for existing tests.
sammccall accepted this revision.Mon, Mar 23, 8:44 AM
sammccall added inline comments.
clang/include/clang/Basic/LangOptions.def
151

this should be COMPATIBLE_LANGOPT I believe - it affects semantics but not in a breaking way.

clang/include/clang/Driver/CC1Options.td
569

We'll want the positive and negative version of this flag because the default will be "sometimes". See fpadding_on_unsigned_fixed_point for such a pair.

This could be deferred to the next patch (as the default is false for now) but seems cleaner to introduce them together.

clang/test/AST/ast-dump-recovery.cpp
2

This is probably a good test to add a second run line with -fno-recovery-ast and FileCheck -check-prefix=DISABLED to test that the flag controls the behaviour.

hokein updated this revision to Diff 252238.Tue, Mar 24, 12:54 AM
hokein marked 2 inline comments as done.

add -fno-recovery-ast, and negative tests.

This revision was automatically updated to reflect the committed changes.
hokein marked an inline comment as done.