This is an archive of the discontinued LLVM Phabricator instance.

[AST] Suppress the spammy "attempt to use a deleted fucntion" diagnostic.
ClosedPublic

Authored by hokein on Apr 14 2020, 4:58 AM.

Details

Summary

This patch fixes the regression diagnostic, which was introduced in
https://reviews.llvm.org/D77395.

Diff Detail

Event Timeline

hokein created this revision.Apr 14 2020, 4:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 14 2020, 4:58 AM
hokein updated this revision to Diff 257302.Apr 14 2020, 6:12 AM

fix the broken tests.

Sorry to go back and forth on this, but I'm not sure whether these diagnostics are actually spam to be suppressed. I think @adamcz mentioned these today as reasonable diagnostics we're enabling.

@rsmith do you have an opinion here?

clang/lib/Sema/SemaDeclCXX.cpp
15006

This deserves a comment, like "if initializing the variable failed, don't also diagnose problems with the desctructor, they're likely related".

Sorry to go back and forth on this, but I'm not sure whether these diagnostics are actually spam to be suppressed. I think @adamcz mentioned these today as reasonable diagnostics we're enabling.

@rsmith do you have an opinion here?

My initial reaction was certainly that it wasn't obvious why an initialization error would necessarily have anything to do with a destruction error. But this line of thinking helped me: suppose we would encounter both an (unrecoverable) initialization error *and* an error during destruction. How likely is it that they have a common cause? My guess would be way more than half the time. Given that we know we're on an error recovery path, and that we've already produced an unrecoverable error during initialization, skipping the destruction checks is probably the better choice. Even if we're wrong, the worst case is that the programmer fixes the initialization error and is then presented with a destruction error that they didn't see before, which doesn't seem all that bad of an outcome.

(Generally I don't think we need to be sure that errors would be correlated to suppress latter errors as being follow-on diagnostics from earlier errors, and should lean towards suppressing diagnostics in order to make the second and subsequent diagnostic as likely as possible to be meaningful and useful.)

clang/lib/Sema/SemaDecl.cpp
11985–11986

Should we attach a RecoveryExpr initializer in this case?

11998–12001

Should we attach a RecoveryExpr initializer in this case?

12268

Should we attach a RecoveryExpr initializer in this case too?

12562–12563

I don't think this is accurate (we didn't "leave var uninitialized"). Maybe something like: "If default-init fails, attach an initializer that's marked invalid to track that initialization was attempted and failed."

clang/test/CXX/class.access/p4.cpp
1–6

I'm not really happy about improving our quality of diagnostics only under -frecovery-ast. Do we really need that flag? The way I see it, we can divide the users of Clang up into:

  • We want valid code (eg, as a compiler): if we hit an error, it doesn't matter too much whether we build RecoveryExprs or not, since we're on a path to bailing out anyway. If RecoveryExprs allow us to improve our diagnostics, we should build them. (Exception: if we want valid code or to get a simple pass/fail as early as possible, maybe not.)
  • We want to accept invalid code (eg, tooling): if we hit an error, we probably want to retain as much information as we reasonably can about the non-erroneous parts of the program.

So I think at least the default should be to build RecoveryExprs, and maybe we can remove the flag entirely?

Sorry to go back and forth on this, but I'm not sure whether these diagnostics are actually spam to be suppressed. I think @adamcz mentioned these today as reasonable diagnostics we're enabling.

@rsmith do you have an opinion here?

My initial reaction was certainly that it wasn't obvious why an initialization error would necessarily have anything to do with a destruction error. But this line of thinking helped me: suppose we would encounter both an (unrecoverable) initialization error *and* an error during destruction. How likely is it that they have a common cause? My guess would be way more than half the time. Given that we know we're on an error recovery path, and that we've already produced an unrecoverable error during initialization, skipping the destruction checks is probably the better choice. Even if we're wrong, the worst case is that the programmer fixes the initialization error and is then presented with a destruction error that they didn't see before, which doesn't seem all that bad of an outcome.

(Generally I don't think we need to be sure that errors would be correlated to suppress latter errors as being follow-on diagnostics from earlier errors, and should lean towards suppressing diagnostics in order to make the second and subsequent diagnostic as likely as possible to be meaningful and useful.)

Thanks, that's useful. The common cause was our original intuition but I wasn't sure if that was an appropriate reason to suppress.

clang/lib/Sema/SemaDecl.cpp
11998–12001

This is D78116 (which should probably handle the case above, too)

12268

Now we're really slipping into a different set of use-cases for RecoveryExpr...
I assume we're talking about a RecoveryExpr that has no children, at least in the short term, as parsing failures don't return the likely subexpressions found. So it's a pure error marker in the form of an AST node. Quite a lot of ExprError()s could become these...

Actually there's another virtue here: even without subexpressions, the RecoveryExpr can capture the token range. So VarDecl::getSourceRange() will include the malformed initializer, so tools can at least attribute these tokens to the right part of the AST.

clang/test/CXX/class.access/p4.cpp
1–6

Agree that want to flip the default to true, and maybe eventually get rid of it. But:

  • we're still fighting quite a lot of new crash-on-invalids, and can't fix them all in one big patch. We plan to find more by rolling this out to a subset of google-internal IDE users (where we have good monitoring), the flag is needed for this.
  • we expect this to be stable for C++ long before it can be enabled for C, because that requires making the C codepaths safe handle (at least error-)dependence. So there'll be a similar testing/improvement period later.

However I don't like adding -frecovery-ast to big existing tests, we lose coverage of today's production behavior. I think we may need to create new tests to show the effect of these changes, and clean them up after flipping the default.

hokein updated this revision to Diff 257689.Apr 15 2020, 6:09 AM
hokein marked 4 inline comments as done.

address comments:

  • don't modify the existing tests
  • add new tests for -frecovery-ast only.
hokein marked an inline comment as done.Apr 15 2020, 6:13 AM
hokein added inline comments.
clang/lib/Sema/SemaDecl.cpp
11985–11986

I think so, will address in a separate patch.

12268

yeah, I'm not sure how much benefit we can get from the recovery-expr in this case, if the initializer is failed to parse, we don't have any ast nodes.

clang/test/CXX/class.access/p4.cpp
1–6

yeah, it is not perfect, given that we are at intermediate stage. We plan to enable the flag for C++ by default (we did it once, but failed with many crashes), this means we'd eventually focus on '-frecovery-ast' only (at least C++), it seems not too harmful to add the -frecovery-ast flag to exiting tests at the moment. But it is not great..

Another way is to adjust existing tests to support both, but this seems non-trivial, maybe creating new tests for '-frecovery-ast' is a best way to go.

life can be easier if the flag is turned on by default. Currently, I have to maintain a local patch with a long tail of adjusted tests when doing recovery-expr developments, I need to run all tests with&without -frecovery-ast to make sure it doesn't introduce big issures.

rsmith added inline comments.Apr 15 2020, 2:46 PM
clang/lib/Sema/SemaDecl.cpp
12268

What I'm thinking is that we should have some kind of marker in the AST to track that an initializer was provided for the variable but was malformed. Right now, we use the "invalid" bit for that, which means that downstream code that refers to the variable will have poor recovery. If we attach an initialization expression marked with the "contains errors" bit instead, then we can keep the VarDecl valid, and improve the recovery not necessarily for this node but for things downstream of it.

(I guess ultimately it seems reasonable to me to use the same AST representation for "no initializer was provided and implicit default init failed", "an initializer was provided but we couldn't parse / semantically analyze it", and "an initializer was provided but was incompatible with the variable's type" -- except that in the third case we can track the expression that we formed for the initializer.)

I don't think there's any urgency to this, and having both AST models for a while (in different cases) doesn't seem like it's going to cause much pain.

sammccall accepted this revision.Apr 19 2020, 3:54 PM

LG but please fix the sourcerange

clang/lib/Sema/SemaDecl.cpp
12268

Yeah, I think this makes sense. @hokein: even without child AST nodes, this will improve SelectionTree accuracy in clangd: go to definition on some part of an invalid initializer currently goes to the vardecl, and after adding RecoveryExpr here it'll go nowhere instead which is progress.

But the expr needs to have at least the source range, so this isn't a trivial change --> another patch?

12565

This seems like it's going to claim some actual tokens, when the thing it represents doesn't cover any tokens.

I think both start/end source locations should be invalid.

clang/lib/Sema/SemaDeclCXX.cpp
15007

desctructor -> destructor
(sorry, you copied my typo)

This revision is now accepted and ready to land.Apr 19 2020, 3:54 PM
hokein marked an inline comment as done.Apr 20 2020, 6:55 AM
hokein added inline comments.
clang/lib/Sema/SemaDecl.cpp
12565

actually, I think it is still valuable to set the var location to recovery-expr.

Foo [[foo]]; // if there is a valid default ctor, we have a CtorExpr which has the `foo` range; otherwise there is a recoveryExpr with the same range.

OK LG without changes

clang/lib/Sema/SemaDecl.cpp
12565

Yeah, invalid source range is scary too. Let's go with this and see if things break. I think more likely it'll be a mild annoyance like the range of CXXConstructExpr.

(In a perfect world maybe this would have a location but no range, or a SourceRange would have half-open semantics and could represent a point).

This revision was automatically updated to reflect the committed changes.