This is an archive of the discontinued LLVM Phabricator instance.

Add support for attribute 'using_if_exists'
ClosedPublic

Authored by ldionne on Oct 26 2020, 1:18 PM.

Details

Summary

This attribute applies to a using declaration, and permits importing a declaration without knowing if that declaration exists. This is useful for libc++ c wrapper headers that re-export declarations in std::.

As far as implementation goes, when some declarations are found, this attribute has no effect. When no declaration is found, Sema creates a single UsingShadowDecl that targets an UnresolvedUsingIfExistsDecl. If the UnresolvedUsingIfExistsDecl is referred to later, then an error is emitted. If the using-declaration is dependent, then Sema creates an UnresolvedUsingValueDecl/UnresolvedUsingTypenameDecl (as usual), but on instantiation they generate a UsingDecl that refers to an UnresolvedUsingIfExistsDecl.

This was proposed by Louis Dionne here: http://lists.llvm.org/pipermail/cfe-dev/2020-June/066038.html
rdar://69313357

Thanks for taking a look!

Diff Detail

Event Timeline

erik.pilkington requested review of this revision.Oct 26 2020, 1:18 PM
ldionne added inline comments.Oct 27 2020, 12:04 PM
clang/test/SemaCXX/using-if-exists-attr.cpp
16 ↗(On Diff #300716)

I assume the attribute is ignored on these declarations? Why is this not an error instead, since it's clearly a programmer mistake?

clang/test/SemaCXX/using-if-exists-attr.cpp
16 ↗(On Diff #300716)

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?

A few questions. I'm not familiar enough with the code to accept the patch.

clang/include/clang/Basic/AttrDocs.td
5266

using_if_exists please add two backticks before and after the attribute name so it renders nicer.

5273

Can you add an example using [[clang::using_if_exists]] or use that instead of this attribute?

5273

Why is the attribute placed here? I would expect the attribute before the declaration [[clang::using_if_exists]] using empty_namespace::does_not_exist;

clang/lib/Sema/SemaDecl.cpp
497

Why do you want recover instead of returning a nullptr?

clang/test/SemaCXX/using-if-exists-attr.cpp
1 ↗(On Diff #300716)

Does the test require C++20? If so can you use -std=c++20 since Clang supports it, same for the other test.

9 ↗(On Diff #300716)

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.

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.

clang/lib/AST/DeclBase.cpp
816

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.

clang/lib/Sema/SemaDecl.cpp
437–438

Can you use getUnderlyingDecl for this?

439–440
497

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?

clang/lib/Sema/SemaDeclCXX.cpp
12119

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.

clang/lib/Sema/SemaExpr.cpp
3146–3150

I think this should instead be done by DiagnoseUseOfDecl / CanUseDecl. All the code paths we care about already go through that.

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
3195–3196

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.

clang/lib/Sema/TreeTransform.h
14115–14123

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;
aaron.ballman added inline comments.Nov 2 2020, 6:02 AM
clang/test/SemaCXX/using-if-exists-attr.cpp
16 ↗(On Diff #300716)

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.

aaron.ballman added inline comments.Nov 2 2020, 6:02 AM
clang/include/clang/AST/DeclCXX.h
3801

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?

clang/include/clang/Basic/DiagnosticSemaKinds.td
554

Please add single quotes around the attribute name.

clang/lib/Sema/SemaDecl.cpp
1172

const auto *

clang/lib/Sema/SemaExpr.cpp
3146

const auto *

clang/lib/Sema/SemaExprMember.cpp
1169 ↗(On Diff #300716)

const auto *

clang/test/SemaCXX/using-if-exists-attr.cpp
9 ↗(On Diff #300716)

I'm not certain what this FIXME means given that it is using the C++ spelling there.

erik.pilkington marked 20 inline comments as done.

Address review comments, thanks!

clang/include/clang/AST/DeclCXX.h
3801

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.

clang/include/clang/Basic/AttrDocs.td
5273

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.

clang/lib/Sema/SemaDecl.cpp
439–440

Huh, didn't know isa did that!

497

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?

1172

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.

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
3195–3196

Yeah, this is unreachable. I added a llvm_unreachable.

clang/lib/Sema/TreeTransform.h
14115–14123

Sure, the update adds that testcase in.

clang/test/SemaCXX/using-if-exists-attr.cpp
1 ↗(On Diff #300716)

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.

9 ↗(On Diff #300716)

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.

16 ↗(On Diff #300716)

Ok sure, I agree with Louis that the error makes sense here so I added ErrorDiag.

aaron.ballman added inline comments.Nov 4 2020, 6:49 AM
clang/include/clang/AST/DeclCXX.h
3801

So given code like this:

using empty_namespace::does_not_exist __attribute__((using_if_exists));

would you expect this AST matcher to match or not?

usingDecl()

(I hope the answer is "yes" because otherwise the behavior is rather inexplicable to me.)

clang/include/clang/Basic/AttrDocs.td
5273

I think this is the first attribute we actually support on using-declarations, so maybe we should consider supporting it.

This isn't the first attribute we *should* be supporting on using declarations. Any attribute that appertains to a NamedDecl should apply as should the annotate attribute.

However:

[[clang::whatever]] using foo::bar; // Correct to reject, #1
using foo::bar [[clang::whatever]]; // Correct to reject, #2

#1 is rejected because it's a declaration-statement and those cannot have a leading attribute-specifier-seq (http://eel.is/c++draft/stmt.stmt#stmt.pre-1). #2 is rejected because the using-declaration cannot have a trailing attribute-specifier-seq (http://eel.is/c++draft/namespace.udecl#nt:using-declaration).

This seems like a case where we may want to explore an extension to C++ that we propose to WG21.

clang/lib/Sema/SemaDecl.cpp
1172

Ah, then don't bother with the goose chase. We don't require const correctness because of these kinds of goose chases, but we usually strive for const correctness when it's cheap to do so.

erik.pilkington marked 3 inline comments as done.Nov 4 2020, 8:05 AM
erik.pilkington added inline comments.
clang/include/clang/AST/DeclCXX.h
3801

Yeah, the answer is "yes". The AST representation for a using_if_exists declaration that failed to lookup anything would look something like this:

|-UsingDecl 0x7fa42b84a3a0 <t.cpp:20:1, col:9> col:9 ::X
|-UsingShadowDecl 0x7fa42b84a3f8 <col:9> col:9 implicit UnresolvedUsingIfExists 0x7fa42b84a370 'X'

So its still using the typical UsingDecl machinery, its just that the declaration thats being imported is different.

clang/include/clang/Basic/AttrDocs.td
5273

This isn't the first attribute we *should* be supporting on using declarations. Any attribute that appertains to a NamedDecl should apply as should the annotate attribute.

Yeah, agreed. Its just that Sema was failing to call ProcessDeclAttributeList for UsingDecls, so no attributes were actually getting applied in practice.

Okay, if our policy is to only parse C++-style attributes in places that the C++ grammar allows them then I agree that this would be an issue for the standards committee.

aaron.ballman added inline comments.Nov 4 2020, 8:30 AM
clang/include/clang/AST/DeclCXX.h
3801

Awesome, thank you for clarifying!

Mordante added inline comments.Nov 4 2020, 9:40 AM
clang/include/clang/Basic/AttrDocs.td
5273

I would be in favour to implement this as an extension and propose it to the committee. I wonder whether it's not allowed because the committee objected or nobody wrote a paper. (Attributes are already allowed on using namespace directives http://eel.is/c++draft/namespace.udir.)

aaron.ballman added inline comments.Nov 4 2020, 10:07 AM
clang/include/clang/Basic/AttrDocs.td
5273

I wouldn't be opposed to adding an extension for parsing C++-style attributes on parts of a using declaration so long as we diagnosed them as an extension to alert people to portability issues. I would say that the attribute should be written as a prefix attribute (similar to the using-directive grammar) and after the using-declarator. This would allow you to do:

[[]] using foo::bar;
using foo::bar [[]];
using foo::bar [[]], baz::quux;
rsmith added inline comments.Nov 10 2020, 3:05 PM
clang/include/clang/Basic/AttrDocs.td
5273

FWIW, this matches my intuition for attribute placement.

erik.pilkington marked 2 inline comments as done.

Add support for C++11-style attributes on using-declarations.

Add support for C++11-style attributes on using-declarations.

FWIW, it'd be a bit easier if those changes were split off into their own patch, as they're orthogonal to using_if_exists.

clang/include/clang/Basic/AttrDocs.td
5351

Do you think the docs should call out that using the C++ spelling on a using declaration is currently a Clang extension? It's a bit of an odd place to put that information, but I don't know of where else to mention that this attribute is nonportable in a few different ways.

clang/include/clang/Basic/DiagnosticParseKinds.td
689 ↗(On Diff #305193)

I'm not certain what @rsmith thinks, but I think this should be ExtWarn and have the DefaultIgnore removed -- this is a conforming extension of something that's nonportable even in the absence of any attributes in the attribute list. e.g., [[]] using foo::bar;`` is not portable to parse.

690 ↗(On Diff #305193)

I think that should say ISO C++ instead of just C++ to make it clear that this is a standards thing, not a C++-as-it-really-is thing.

clang/include/clang/Serialization/ASTBitCodes.h
1364

Does this mean you need to bump VERSION_MAJOR as well?

clang/test/Parser/cxx0x-attributes.cpp
160 ↗(On Diff #305193)

Can you also add a test for using foo [[]], bar []]]; syntax?

clang/test/SemaCXX/using-if-exists-attr.cpp
9–17 ↗(On Diff #305193)

These test cases look like they belong in a Parser test not a SemaCXX test.

erik.pilkington marked 6 inline comments as done.

Split off parsing support for C++ attributes.

FWIW, it'd be a bit easier if those changes were split off into their own patch, as they're orthogonal to using_if_exists.

Ok, I moved this part to: https://reviews.llvm.org/D91630

clang/include/clang/Basic/AttrDocs.td
5351

Sure, I'll mention it. I'm also switching back to the GCC spelling here since we're warning on the C++ spelling by default.

clang/include/clang/Basic/DiagnosticParseKinds.td
689 ↗(On Diff #305193)

Sure, I'll update the docs to prefer using the GNU spelling if we're going to warn by default on this spelling.

clang/include/clang/Serialization/ASTBitCodes.h
1364

Hmm, I don't think so, it looks like the git hash is taken into account based on the comment in ASTBitCodes.h:

/// Version 4 of AST files also requires that the version control branch and
/// revision match exactly, since there is no backward compatibility of
/// AST files at this time.
const unsigned VERSION_MAJOR = 11;

FWIW it seems like nobody else who adds new AST nodes is bumping this version.

clang/test/Parser/cxx0x-attributes.cpp
160 ↗(On Diff #305193)

Sure, I did this in the parser patch.

clang/test/SemaCXX/using-if-exists-attr.cpp
9–17 ↗(On Diff #305193)

Sure, I moved this file over to Parser.

aaron.ballman added inline comments.Nov 23 2020, 7:25 AM
clang/lib/Sema/SemaDecl.cpp
437
1173
clang/lib/Sema/SemaExpr.cpp
3175

auto *VD = cast<ValueDecl>(D);

clang/lib/Serialization/ASTReaderDecl.cpp
1704–1707

I'm not super familiar with this code, but is this change actually necessary? I would have expected this to be handled via VisitNamedDecl by virtue of UnresolvedUsingIfExistsDecl inheriting from NamedDecl.

erik.pilkington marked 4 inline comments as done.

Address review comments.

clang/lib/Serialization/ASTReaderDecl.cpp
1704–1707

I think its necessary -- DeclVisitor doesn't automatically visit the base classes. Every other Visit* function here manually visits the base classes too FWIW.

aaron.ballman accepted this revision.Dec 4 2020, 9:30 AM

LGTM aside from a minor nit, thank you!

clang/lib/Sema/SemaDeclCXX.cpp
11681

You can use isa_and_nonnull<>() instead of testing for null yourself.

This revision is now accepted and ready to land.Dec 4 2020, 9:30 AM
Quuxplusone added inline comments.
clang/test/SemaCXX/using-if-exists.cpp
12

Do you also have a test for using NS::NotNS::x UIE;?
(Both of these NotNS tests produce hard errors; [[using_if_exists]] does the silent-ignore thing only when the tippymost identifier is nonexistent, not if some intermediate name is nonexistent. This seems reasonable enough.)

61

An inheritance case not yet covered here errors out with error: no matching constructor for initialization of 'Derived<Base>':

struct Base { Base(int); };
template<class T> struct Derived : T {
    using T::T [[clang::using_if_exists]];
};
Derived<Base> d(1);

Does this make sense? Should this attribute perhaps give a hard error if you try to use it to inherit a constructor overload set like this, instead of (apparently) just no-opping the construct?

80

I notice there's a hard error: 'using_if_exists' attribute cannot be applied to types on

using B::operator int UIE;

Any thoughts on how to disambiguate that grammar?

108

This is a very good test, but it feels to me as if

struct B { static int f(int); };
struct C { };
template<class... Ts> struct D : Ts... {
    using Ts::f... [[clang::using_if_exists]];
};
int main() { D<B, C>::f(1); }

ought to be at least as well-formed as

struct B { static int f(int); };
struct C { static void f(); };
template<class... Ts> struct D : Ts... {
    using Ts::f... [[clang::using_if_exists]];
};
int main() { D<B, C>::f(1); }

I guess what's going on here is that an "unresolved using decl" is considered a separate kind of entity, i.e. using Ts::f... will work if all Ts::f... are variables, or if all of them are functions, or if all of them are unresolved, but it doesn't work if you have a mix of variables and functions, or variables and unresolveds, or functions and unresolveds. Is that basically correct, and intended?

200

IMHO this test would be clearer if it used obviously fake names, e.g. using ::fake_FILE, using ::fake_printf, etc.

erik.pilkington marked 4 inline comments as done.

Address review comments.

clang/test/SemaCXX/using-if-exists.cpp
12

Yeah, this is only about the tippymost name :). New patch adds a test for non-existent names in the middle, which was previously untested.

61

Hmm, yeah, I don't think that inheriting constructors really make sense with this attribute. New patch adds an error.

80

Hmm, that's an interesting case. I'm not sure what the right approach is. I think it would be a bit awkward to decide where to attach the attribute depending on what kind of entities the attribute could potentially apply to. FWIW this isn't a semantic restriction, since you can always just use the prefix syntax: UIE using B::operator int;. Maybe @aaron.ballman has some thoughts here?

108

I guess what's going on here is that an "unresolved using decl" is considered a separate kind of entity, i.e. using Ts::f... will work if all Ts::f... are variables, or if all of them are functions, or if all of them are unresolved, but it doesn't work if you have a mix of variables and functions, or variables and unresolveds, or functions and unresolveds. Is that basically correct, and intended?

Yeah, exactly. My mental model for how this attribute works is that an unresolved using-declaration introduces an entity that isn't a function, value, or type, and is incompatible with all of those. Sometimes, that isn't what you want for overloaded functions, like Louis ran into an issue while testing this where remove(const char *) in <cstdio> being unresolved lead to a compile error on the declaration of remove(iter, iter, val) in <algorithm>. That seems to basically be the same issue you're seeing in your first test case.

Personally, my inclination is to conservatively just disallow any interplay with an unresolved using-declaration and a function, and if we run into a lot of problems in practice then we can come up with a more nuanced approach for this case without breaking backwards compatibility. WDYT?

aaron.ballman added inline comments.Apr 6 2021, 2:26 PM
clang/test/SemaCXX/using-if-exists.cpp
80

Does anyone have any remaining concerns or a desire to take another pass through this patch? If not, then I'll commit this later this week on Aaron's LGTM.

ldionne commandeered this revision.May 31 2021, 8:26 AM
ldionne edited reviewers, added: erik.pilkington; removed: ldionne.
ldionne updated this revision to Diff 348810.May 31 2021, 8:27 AM

Rebase onto main.

Herald added a project: Restricted Project. · View Herald TranscriptMay 31 2021, 8:27 AM
ldionne updated this revision to Diff 348952.Jun 1 2021, 5:49 AM

Rebase onto main to trigger CI.

ldionne updated this revision to Diff 349010.Jun 1 2021, 10:21 AM

Run clang-format.

ldionne updated this revision to Diff 349087.Jun 1 2021, 1:30 PM

Fix CI issues (hopefully - at least everything runs locally).

This revision was automatically updated to reflect the committed changes.