Page MenuHomePhabricator

[AST] Preserve the type in RecoveryExprs for broken function calls.
ClosedPublic

Authored by hokein on Apr 30 2020, 2:22 AM.

Details

Summary

RecoveryExprs are modeled as dependent type to prevent bogus diagnostics
and crashes in clang.

This patch allows to preseve the type for broken calls when the
RecoveryEprs have a known type, e.g. a broken non-overloaded call, a
overloaded call when the all candidates have the same return type, so
that more features (code completion still work on "take2args(x).^") still
work.

However, adding the type is risky, which may result in more clang code being
affected leading to new crashes and hurt diagnostic, and it requires large
effort to minimize the affect (update all sites in clang to handle errorDepend
case), so we add a new flag (off by default) to allow us to develop/test
them incrementally.

This patch also has some trivial fixes to suppress diagnostics (to prevent regressions).

Tested:

  • all existing tests are passed (when both "-frecovery-ast", "-frecovery-ast-type" flags are flipped on);

Diff Detail

Event Timeline

hokein created this revision.Apr 30 2020, 2:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 30 2020, 2:22 AM
hokein edited the summary of this revision. (Show Details)Apr 30 2020, 2:34 AM
hokein edited the summary of this revision. (Show Details)Apr 30 2020, 2:37 AM
hokein updated this revision to Diff 261167.Apr 30 2020, 2:48 AM
hokein marked 3 inline comments as done.

update

hokein added a subscriber: rsmith.
hokein added inline comments.
clang/lib/Sema/SemaOverload.cpp
12779

I steal this function from D61722.

clang/test/SemaCXX/enable_if.cpp
417 ↗(On Diff #261157)

this diagnostic is changed slightly (without -frecovery-ast-type). I think this is acceptable.

before this patch:

/tmp/t5.cpp:7:10: error: no matching function for call to 'templated'
  return templated<N>();  // expected-error {{no matching function for call to 'templated'}} 
         ^~~~~~~~~~~~
/tmp/t5.cpp:13:5: note: in instantiation of function template specialization 'enable_if_diags::callTemplated<0>' requested here
    callTemplated<0>(); // expected-note {{in call to 'callTemplated()'}} expected-note@-6 {{subexpression not valid in a constant expression}}
    ^
/tmp/t5.cpp:2:32: note: candidate disabled: <no message provided>
template <int N> constexpr int templated() __attribute__((enable_if(N, ""))) {
                               ^                                    ~
/tmp/t5.cpp:13:5: error: constexpr variable 'B' must be initialized by a constant expression
    callTemplated<0>(); // expected-note {{in call to 'callTemplated()'}} expected-note@-6 {{subexpression not valid in a constant expression}}
    ^~~~~~~~~~~~~~~~~~
2 errors generated.

vs after this patch:

/tmp/t5.cpp:7:10: error: no matching function for call to 'templated'
  return templated<N>();  // expected-error {{no matching function for call to 'templated'}} 
         ^~~~~~~~~~~~
/tmp/t5.cpp:13:5: note: in instantiation of function template specialization 'enable_if_diags::callTemplated<0>' requested here
    callTemplated<0>(); // expected-note {{in call to 'callTemplated()'}} expected-note@-6 {{subexpression not valid in a constant expression}}
    ^
/tmp/t5.cpp:2:32: note: candidate disabled: <no message provided>
template <int N> constexpr int templated() __attribute__((enable_if(N, ""))) {
                               ^                                    ~
/tmp/t5.cpp:12:15: error: constexpr variable 'B' must be initialized by a constant expression
constexpr int B = 10 +  // expected-error {{constexpr variable 'B' must be initialized by a constant expression}}
              ^   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/tmp/t5.cpp:7:10: note: subexpression not valid in a constant expression
  return templated<N>();  // expected-error {{no matching function for call to 'templated'}} 
         ^
/tmp/t5.cpp:13:5: note: in call to 'callTemplated()'
    callTemplated<0>(); // expected-note {{in call to 'callTemplated()'}} expected-note@-6 {{subexpression not valid in a constant expression}}
    ^
2 errors generated.
clang/test/SemaTemplate/instantiate-function-params.cpp
1 ↗(On Diff #261157)

this test is very tricky, and hard for human to understand. It was added to test clang not crashing.

Different compilers (gcc, msvc) emit different diagnostics, with this patch, clang emits 3 more errs no type named 'type'..., it seems reasonable, and gcc also emits them.

sammccall marked an inline comment as done.Apr 30 2020, 5:27 AM
sammccall added inline comments.
clang/include/clang/AST/Expr.h
6099–6100

Comment is stale.

6099–6100

We should mention something about optional type preservation.

clang/lib/AST/ComputeDependence.cpp
494

Dropping type-dependence seems like the point here and is risky, doesn't dropping value- and instantiation-dependence introduce unneccesary extra risk?

Probably correct and maybe useful, but splitting it into a separate change and maybe delaying it might make it easier to get the principal change to stick.

clang/lib/AST/Expr.cpp
4687

Why is this in ifndef?
Actually, why is the loop in ifndef? surely the compiler can inline and remove an empty loop over an arrayref?

clang/lib/Sema/SemaDecl.cpp
16429

why is this needed/does it help?
I would have thought in the cases where we now don't consider the type dependent, the *type* wouldn't contain errors (rather the expression owning the type would).

Is this just for decltype?

clang/lib/Sema/SemaOverload.cpp
12904–12908

nit: try to recover
(or just "Overload resolution failed", what the code is doing is obvious enough here)

clang/test/CodeCompletion/member-access.cpp
276

I think there's an interesting test of the "viable" case where you have a const/non-const overload set with different return types:

class Collection {
  const char *find(int) const;
  char* find(int) const;
};
void test1(const Collection &C, Collection &NC) {
 C.find(); // missing arg, type const char*
 NC.find(); // missing arg, is type NC or is it unresolved?
}

(Not sure if it's best written as a codecompletion test, I guess AST dump is better)

clang/test/Index/getcursor-recovery.cpp
6

nit: unresolved overloaded call?

19

nit: a valid foo(int, int) call?
(just to draw the contrast)

clang/test/SemaCXX/enable_if.cpp
417 ↗(On Diff #261157)

Yeah, this is noisier but seems OK.
Can you take a look at the blame for the split-line test and see if there was any special code to support it that can be cleaned up?

clang/test/SemaCXX/recovery-expr-type.cpp
12 ↗(On Diff #261167)

what if we make this constexpr and try to static-assert on it, use it as the size of an array etc?

clang/test/SemaTemplate/instantiate-function-params.cpp
1 ↗(On Diff #261157)

Ugh, I remember this test. It's partially reduced from boost IIRC.

hokein updated this revision to Diff 261762.May 4 2020, 2:43 AM
hokein marked 13 inline comments as done.

address comments.

hokein added inline comments.May 4 2020, 2:44 AM
clang/lib/AST/ComputeDependence.cpp
494

Dropping type-dependence seems like the point here and is risky, doesn't dropping value- and instantiation-dependence introduce unneccesary extra risk?

maybe, but you are right, we should be conservative.

separating this patch seems good to me:

  1. create a new patch like what we do here,
  2. this patch just getting rid of the type bit (still set the value/instantiation bits)

with 1, we will get rid of the value/instantiation-depend bits (for the default Ctx.DependentType case), and we get closer to our goal.

clang/lib/AST/Expr.cpp
4687

removed.

clang/lib/Sema/SemaDecl.cpp
16429

yeah, decltype is the only case we have been discovered so far.

clang/test/CodeCompletion/member-access.cpp
276

oh, this is a good point, this patch doesn't capture the type for member functions, it is in a different code path (probably we should handle that in BuildCallToMemberFunction).

I'd prefer to handle it in a separate patch.

clang/test/SemaCXX/enable_if.cpp
417 ↗(On Diff #261157)

git blame showed https://github.com/llvm/llvm-project/commit/c7d3e609fbf05d1a1236f99efd1e2fd344554f4b, but I didn't see any special code that we need clean up.

clang/test/SemaCXX/recovery-expr-type.cpp
12 ↗(On Diff #261167)

added tests

sammccall accepted this revision.May 4 2020, 6:24 AM
sammccall added inline comments.
clang/lib/AST/ComputeDependence.cpp
488

This comment is hard to understand in isolation: it describes what the patch is doing (compared to the baseline, and to some alternative), rather than what the code is doing.

I'd suggest something like:

Mark the expression as value- and instantiation- dependent to reuse existing suppressions for dependent code, e.g. avoiding constant-evaluation.
clang/lib/AST/Expr.cpp
3310

This can move into the later patch I think - we're always instantiation-dependent where it matters.

clang/lib/Sema/SemaDecl.cpp
12815

again - can move into later patch

clang/lib/Sema/SemaDeclCXX.cpp
15887 ↗(On Diff #261762)

can defer this too

clang/test/CodeCompletion/member-access.cpp
283

nit: CHECK-RECOVERY or so?

This revision is now accepted and ready to land.May 4 2020, 6:24 AM
hokein updated this revision to Diff 262044.May 5 2020, 2:40 AM
hokein marked 4 inline comments as done.

address comments.

hokein updated this revision to Diff 262872.May 8 2020, 7:25 AM

rebase to master (not the D78350)

hokein updated this revision to Diff 262874.May 8 2020, 7:36 AM
hokein marked an inline comment as done.

add a missing test file during the rebase.

hokein marked an inline comment as done.May 8 2020, 7:45 AM
hokein added inline comments.
clang/test/SemaCXX/enable_if.cpp
417 ↗(On Diff #261157)

actually we don't need this change if we land this patch before the enable-recovery-expr patch, defer this change to D78350.

clang/test/SemaTemplate/instantiate-function-params.cpp
1 ↗(On Diff #261157)

the same to this file, defer it to D78350.

This revision was automatically updated to reflect the committed changes.