[RFC] Allowing _Nonnull etc on smart pointers

Clang Frontend
21

Thank you for the summary, I agree with your take on consensus so far.

I prefer using the attribute-based proposal and not inferring based on the name of standard library identifiers (I generally dislike the compiler getting clever about knowing “special” C++ APIs because of how incredibly difficult lookup is in C++ – things like using declarations, inline anonymous namespaces, template specializations, etc make it more effort than for C APIs.) That said, I can live with a name-based lookup if that’s got stronger consensus.

I do have design questions though. Instead of making [[clang::nullable]], should we reuse _Nullable (et al)? Then the rule is “pointers are _Nullable as are any class type marked _Nullable” which seems pretty easy for users to understand and for us to implement.

Also, here are some code snippets with questions…

struct __Nullable Base {
};

struct Derived : Base { // Is this nullable?
};

struct PrivatelyDerived : private Base { // Is this nullable?
};

template <typename Ty>
void func(Ty Foo);

template <>
void func<Base>(_Nullable Base Foo); // Is this okay? (I assume so)

template <typename Ty>
void other_func(_Nullable Ty Foo);

void call_it() {
  other_func(Base{}); // Is this okay? (I assume so)
  other_func(0); // Is this okay? (I assume it's an instantiation error because int is not nullable)
}

struct NotNullable {};

using Errr _Nonnull = NotNullable; // Is this allowed and is Errr nullable?
using Uhhhh = NotNullable _Nonnull; // Is this allowed and is Uhhhh nullable?
// I assume typedef will work the same way as using aliases.
22

To offer some input with a very shallow understanding: I like the idea of designing around the attribute mechanism, and having the compiler implicitly apply the attribute to the well-known classes we care about (unique_ptr, shared_ptr).

I thought perhaps we could use API note files to apply this attribute to the relevant declarations, but API notes do not appear to be general enough for this yet.

23

Absolutely. I was trying to suggest two things:

  • let’s reuse _Nullable for this attribute on classes
  • let’s introduce [[clang::nullable]] etc as an alternate spelling for _Nullable etc in all contexts

The latter is really independent of the main proposal here, and not at all essential. I think of it as kind of a cleanup, but it’s not important.

All your examples are debatable, below are just my 2c:

struct Derived : Base { // Is this nullable?

No, in the name of keeping both contract + implementation simple.
We can always make more things nullable later, but committing to it now is ~irreversable. I don’t feel strongly about this thuogh.

struct PrivatelyDerived : private Base { // Is this nullable?

Definitely not: nullability is public API and API shouldn’t be affected by private inheritance.

template <> void func<Base>(_Nullable Base Foo); // Is this okay? (I assume so)

I think this is fine: Base is nullable and the specialization is OK as _Nullable is just sugar. Semantics are “when calling func with Base, it shouldn’t be null”.

I’d add this example:

template <> void func<_Nullable Base>(_Nullable Base);`

Using _Nullable in specialization args is dubious: you can’t specialize separately for _Nullable and _Nonnull. I don’t think it’s actually worth the trouble to disallow this though.

other_func(Base{}); // Is this okay? (I assume so);

Yes, this looks fine.

other_func(0); // Is this okay? (I assume it’s an instantiation error because int is not nullable)

Instantiation error. (I can see merit in accepting this: “nullable if possible”, but the existing behavior of _Nullable disallows this and we should be consistent).

using Errr _Nonnull = NotNullable; // Is this allowed and is Errr nullable?

I think this is an error: the only decls you can apply _Nullable to are CXXRecordDecl.

Clang does sometimes support putting attributes on declarations that should really be on their types, with a warning:

warning: applying attribute 'address_space' to a declaration is deprecated; apply it to the type instead [-Wdeprecated-attributes]
using y [[clang::address_space(1)]] = int*;
      ~   ^

If we chose to allow this, this example is equivalent to:

using Errr = NotNullable _Nonnull;

and this is an error, because NotNullable isn’t _Nullable.

Agreed on both counts.

24

Thank you for bringing up APINotes, that’s an excellent observation!

This PR is an example of what comes from the compiler having special knowledge about names in the STL – all the folks using their own hand-rolled STL are left in the cold. So IMO, we need something that users can explicitly opt into with markings of their own. It would be nice if API notes worked sufficiently well for us to handle existing STLs rather than have the compiler proper know special names.

25

Ah I misunderstood then. Let’s leave that separate for now so we don’t have to debate the merits of multiple ways of spelling the same thing.

As is typically the case. :wink:

+1, I agree with waiting until later so the design space isn’t closed off too early. Also, a +1 to your other feelings on how my examples should be handled. I agree with your reasoning.

26

Thanks! I’ve updated and sent [clang][nullability] allow _Nonnull etc on gsl::Pointer types by sam-mccall · Pull Request #82705 · llvm/llvm-project · GitHub based on our discussion here.

It implicitly adds the attribute to std::unique_ptr and friends. If we don’t have this consistently, I think we’d need some way for code to feature-test whether unique_ptr<int> _Nullable is going to work or be a compile error.

1 Like