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

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added inline comments.


================
Comment at: clang/include/clang/AST/DeclCXX.h:3801
+/// error.
+class UnresolvedUsingIfExistsDecl final : public NamedDecl {
+  UnresolvedUsingIfExistsDecl(DeclContext *DC, SourceLocation Loc,
----------------
aaron.ballman wrote:
> Why is this inheriting from a `NamedDecl` rather than a `UsingDecl`? Given that this is a type of using declaration, I guess I would have expected it to appear as such in the AST hierarchy. For instance, should people using AST matchers be able to match one of these as a using declaration or are they so different semantically that they need to be sibling AST nodes?
This node isn't a kind of using declaration, it is a declaration that gets inserted into the scope via a usual UsingDecl & UsingShadowDecl mechanism that Sema knows to error out on if it is ever used. So presumably existing AST users would still recognize that this is a UsingDecl that adds a single declaration into the current context, but wouldn't really know anything about that declaration. I updated the doc comment above to make that more clear.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:5273
+  namespace empty_namespace {};
+  using empty_namespace::does_not_exist __attribute__((using_if_exists)); // no error!
+
----------------
Mordante wrote:
> Mordante wrote:
> > Can you add an example using `[[clang::using_if_exists]]` or use that instead of this attribute?
> Why is the attribute placed here? I would expect the attribute before the declaration `[[clang::using_if_exists]] using empty_namespace::does_not_exist;`
The attribute is written like that because clang rejects C++ style attributes on using declarations, and only accepts attributes in that position. I think this is the first attribute we actually support on using-declarations, so maybe we should consider supporting it.


================
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))) {
----------------
rsmith wrote:
> 
Huh, didn't know `isa` did that!


================
Comment at: clang/lib/Sema/SemaDecl.cpp:500
+    // Recover with 'int'
+    T = Context.IntTy;
   } else if (AllowDeducedTemplate) {
----------------
rsmith wrote:
> 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?
Hmm, removing this case seems to result in worse diagnostics. For instance, here:
```
using NS::X __attribute__((using_if_exists));
X y;
```

If we remove this check and let `getTypeName()` just return nullptr on `X`, then the parser just emits a generic "unknown type name `X`" diagnostic. This is the only place in that path where lookup is done on X, so ISTM like the place where we should be checking this.

If we don't recover with some type, then the parser will sometimes assume that this is unworkable as a type, and attempt to recover by parsing it as something else, which can lead to duplicate diagnostics. e.g., here: `int i = X();`

I guess we could make `getTypeName()` return an `Optional<ParsedType>` that indicated to callers that they should assume that the name was a type, even if it can't be represented by a type node. That seems a little heavy-handed though. WDYT?


================
Comment at: clang/lib/Sema/SemaDecl.cpp:1177
 
+  if (auto *EmptyD = dyn_cast<UnresolvedUsingIfExistsDecl>(FirstDecl)) {
+    Diag(NameLoc, diag::err_use_of_empty_using_if_exists);
----------------
aaron.ballman wrote:
> `const auto *`
This would lead to a bit of a `const`-goosechase in DiagnoseUseOfDecl. I thought we generally weren't too interested in `const` on AST nodes since they're assumed to be immutable anyways, so it doesn't really show much intent.


================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:3195-3196
+    UnresolvedUsingIfExistsDecl *D) {
+  return UnresolvedUsingIfExistsDecl::Create(
+      SemaRef.Context, Owner, D->getLocation(), D->getDeclName());
+}
----------------
rsmith wrote:
> 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.
Yeah, this is unreachable. I added a `llvm_unreachable`.


================
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();
+    }
----------------
rsmith wrote:
> 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;
> ```
Sure, the update adds that testcase in.


================
Comment at: clang/test/SemaCXX/using-if-exists-attr.cpp:1
+// RUN: %clang_cc1 -std=c++2a -fsyntax-only %s -verify
+
----------------
Mordante wrote:
> Does the test require C++20? If so can you use `-std=c++20` since Clang supports it, same for the other test.
It doesn't require C++20, I just think its good practice to always specify the default in a testcase, since clang sometimes updates the default version and downstream users sometimes can't take those updates, which can lead to test breakages. New patch uses the `20` spelling, though.


================
Comment at: clang/test/SemaCXX/using-if-exists-attr.cpp:9
+
+// FIXME: Should we support the C++ spelling here?
+using NS::x [[clang::using_if_exists]]; // expected-error {{an attribute list cannot appear here}}
----------------
aaron.ballman wrote:
> Mordante wrote:
> > I would prefer to allow the C++ spelling, the square brackets are also supported in C when using `-fdouble-square-bracket-attributes`. (Not too useful in this case since C doesn't support namespaces.) But as mentioned before I prefer the attribute before the `using ` declaration.
> I'm not certain what this FIXME means given that it is using the C++ spelling there.
The C++ spelling is getting rejected with an error message, though. It seems like this was an intentional decision: https://github.com/llvm/llvm-project/blob/3c050a597c599cea035537b8875774dcc48922c3/clang/lib/Parse/ParseDeclCXX.cpp#L715. I'm happy add parsing support for C++ attributes if that's what we want. I think we could also support attributes before the using-declaration if we wanted to (and have the attributes there apply to each using-declarator), which I'm also happy to add. Note that the C++ grammar doesn't support attributes anywhere on using-declarations.


================
Comment at: clang/test/SemaCXX/using-if-exists-attr.cpp:16
+template <class>
+using template_alias UIE = NS::x; // expected-warning {{'using_if_exists' attribute only applies to named declarations, types, and value declarations}}
+
----------------
aaron.ballman wrote:
> erik.pilkington wrote:
> > ldionne wrote:
> > > I assume the attribute is ignored on these declarations? Why is this not an error instead, since it's clearly a programmer mistake?
> > Yeah, its just ignored. This is how clang behaves generally with any mis-applied attribute. An error seems more appropriate to me too honestly, but that seems like a separate policy change. Maybe @aaron.ballman knows more about the history/justification for this behaviour?
> It depends entirely on how you write the `Subjects` line in `Attr.td` as to whether this will warn or err. By default, we warn because most attributes should be things you can ignore. But if the semantics of the attribute suggest that an error is a better approach, you can slap an `ErrorDiag` after the subject list to cause errors if the attribute is written on the wrong subject.
Ok sure, I agree with Louis that the error makes sense here so I added ErrorDiag.


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

https://reviews.llvm.org/D90188