[PATCH] D90188: Add support for attribute 'using_if_exists'

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In addition to moving the diagnostic to `DiagnoseUseOfDecl`, you may well get better error recovery if you teach `ClassifyName` about the new kind of declaration, and have it `DiagnoseUseOfDecl` it immediately and return `NameClassification::Error()` -- that way, the parser knows it can't use the kind of the name to disambiguate the parse and should use heuristics to figure out whether a type or non-type was intended instead.



================
Comment at: clang/lib/AST/DeclBase.cpp:816
+    case UnresolvedUsingIfExists:
+      return IDNS_Type | IDNS_Member | IDNS_Ordinary;
+
----------------
Hmm. This is tricky: `IDNS_Type` isn't necessarily appropriate, but we don't know one way or the other. Consider:

```
namespace N {
  using ::stat [[clang::using_if_exists]];
  struct stat;
}
```

Should we accept or reject? If the intent is that the using-declaration brings in only a non-type name, then this should be valid, but if it's intended to also bring in a type, then we'd have a conflict.

I suppose on balance being more restrictive (including `IDNS_Type` here) is probably better.

You don't need `IDNS_Member` here; that's only different from `IDNS_Ordinary` in C.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:438-439
+      NamedDecl *RealRes = *Res;
+      if (auto *ShadowD = dyn_cast<UsingShadowDecl>(*Res))
+        RealRes = ShadowD->getTargetDecl();
+      if (isa<TypeDecl>(RealRes) || isa<ObjCInterfaceDecl>(RealRes) ||
----------------
Can you use `getUnderlyingDecl` for this?


================
Comment at: clang/lib/Sema/SemaDecl.cpp:440-441
+        RealRes = ShadowD->getTargetDecl();
+      if (isa<TypeDecl>(RealRes) || isa<ObjCInterfaceDecl>(RealRes) ||
+          isa<UnresolvedUsingIfExistsDecl>(RealRes) ||
+          (AllowDeducedTemplate && getAsTypeTemplateDecl(RealRes))) {
----------------



================
Comment at: clang/lib/Sema/SemaDecl.cpp:500
+    // Recover with 'int'
+    T = Context.IntTy;
   } else if (AllowDeducedTemplate) {
----------------
Mordante wrote:
> Why do you want recover instead of returning a `nullptr`?
I agree. Producing `IntTy` here is liable to result in follow-on diagnostics. It seems better to tell downstream code that you found a non-type. Can you remove this special case entirely?


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:12119
 
-  if (HasTypenameKeyword) {
-    // If we asked for a typename and got a non-type decl, error out.
-    if (!R.getAsSingle<TypeDecl>()) {
-      Diag(IdentLoc, diag::err_using_typename_non_type);
-      for (LookupResult::iterator I = R.begin(), E = R.end(); I != E; ++I)
-        Diag((*I)->getUnderlyingDecl()->getLocation(),
-             diag::note_using_decl_target);
-      return BuildInvalid();
+  if (!IsUsingIfExists || !R.empty()) {
+    if (HasTypenameKeyword) {
----------------
It might be simpler to create the `UnresolvedUsingIfExistsDecl` earlier (before this `if`) and add it to the lookup result for the checks here and below. That would avoid some of the special-case logic here.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:3146-3150
+    if (auto *EmptyD = dyn_cast<UnresolvedUsingIfExistsDecl>(D)) {
+      Diag(Loc, diag::err_use_of_empty_using_if_exists);
+      Diag(EmptyD->getLocation(), diag::note_empty_using_if_exists_here);
+      return ExprError();
+    }
----------------
I think this should instead be done by `DiagnoseUseOfDecl` / `CanUseDecl`. All the code paths we care about already go through that.


================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:3195-3196
+    UnresolvedUsingIfExistsDecl *D) {
+  return UnresolvedUsingIfExistsDecl::Create(
+      SemaRef.Context, Owner, D->getLocation(), D->getDeclName());
+}
----------------
Is this reachable? I'd expect the only way we can see these is via the list of shadow declarations of the `UsingDecl`, so we should never try to instantiate one by itself.


================
Comment at: clang/lib/Sema/TreeTransform.h:14115-14123
+    NamedDecl *Target = Using->shadow_begin()->getTargetDecl();
+    if (auto *EmptyD = dyn_cast<UnresolvedUsingIfExistsDecl>(Target)) {
+      getSema().Diag(Loc, diag::err_use_of_empty_using_if_exists);
+      getSema().Diag(EmptyD->getLocation(),
+                     diag::note_empty_using_if_exists_here);
+      return QualType();
+    }
----------------
We should `DiagnoseUseOfDecl` along this code path rather than adding an ad-hoc check for this one case. That would also fix another, related bug:

```
struct A { struct __attribute__((deprecated)) B {}; };
// warns that B is deprecated
struct C : A { using typename A::B; B b; };
// fails to warn that B is deprecated
template<typename T> struct D : A { using typename A::B; B b; };
D<A> da;
```


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

https://reviews.llvm.org/D90188