[PATCH] D79160: [AST] Preserve the type in RecoveryExprs for broken function calls.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang/lib/AST/ComputeDependence.cpp:491
+  auto D =
+      toExprDependence(E->getType()->getDependence()) | ExprDependence::Error;
   for (auto *S : E->subExpressions())
----------------
sammccall wrote:
> 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. 
> 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. 


================
Comment at: clang/lib/AST/Expr.cpp:4656
 #ifndef NDEBUG
+  assert(!T.isNull());
   for (auto *E : SubExprs)
----------------
sammccall wrote:
> 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?
removed.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:16427
   QualType EltTy = Context.getBaseElementType(T);
-  if (!EltTy->isDependentType()) {
+  if (!EltTy->isDependentType() && !EltTy->containsErrors()) {
     if (RequireCompleteSizedType(Loc, EltTy,
----------------
sammccall wrote:
> 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?
yeah, decltype is the only case we have been discovered so far.
 


================
Comment at: clang/test/CodeCompletion/member-access.cpp:276
 
+struct S { int member; };
+S overloaded(int);
----------------
sammccall wrote:
> 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)
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.


================
Comment at: clang/test/SemaCXX/enable_if.cpp:417
 
-constexpr int B = 10 + // the carat for the error should be pointing to the problematic call (on the next line), not here.
-    callTemplated<0>(); // expected-error{{initialized by a constant expression}} expected-error at -3{{no matching function for call to 'templated'}} expected-note{{in instantiation of function template}} expected-note at -10{{candidate disabled}}
+constexpr int B = 10 + // expected-error {{initialized by a constant expression}}
+    callTemplated<0>(); // expected-error at -3{{no matching function for call to 'templated'}} expected-note{{in instantiation of function template}} expected-note at -10{{candidate disabled}} expected-note {{in call to 'callTemplated()'}} expected-note at -3 {{subexpression not valid in a constant expression}}
----------------
sammccall wrote:
> hokein wrote:
> > 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 at -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 at -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 at -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 at -6 {{subexpression not valid in a constant expression}}
> >     ^
> > 2 errors generated.
> > ```
> > 
> > 
> 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?
`git blame` showed https://github.com/llvm/llvm-project/commit/c7d3e609fbf05d1a1236f99efd1e2fd344554f4b, but I didn't see any special code that we need clean up.


================
Comment at: clang/test/SemaCXX/recovery-expr-type.cpp:12
+  // no crash.
+  int s = sizeof(make_indestructible()); // expected-error {{deleted}}
+}
----------------
sammccall wrote:
> what if we make this constexpr and try to static-assert on it, use it as the size of an array etc?
added tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79160