[PATCH] D114235: [clang] Extend ParsedAttr to allow custom handling for type attributes

Wed Feb 23 07:06:18 PST 2022

erichkeane added a comment.

In D114235#3340369 <https://reviews.llvm.org/D114235#3340369>, @mboehme wrote:

> Sorry for the delay -- finally back from leave.
>
> High-level, I think I'd like to return to my earlier approach <https://lists.llvm.org/pipermail/cfe-dev/2021-October/thread.html#69087> of adding a new `annotate_type` attribute (see also discussion above).
>
> In D114235#3244054 <https://reviews.llvm.org/D114235#3244054>, @aaron.ballman wrote:
>
>> An `AttributedType` for an annotation attribute would require a new type in the type system,
>
> Not sure I understand what you mean here?
>
> FWIW, the intent would be to consider types with different `annotate_type` attributes to be the same type (and also the same as the un-attributed type). For example, it would not be possible for two overloads to differ merely by `annotate_type` attributes on their parameters.
>
> Or are you saying this would require us to add a new subclass of `Type` to the Clang implementation -- e.g. `AnnotatedType`?

It would essentially be something like that, then you'd have to alter every single place we touch a 'type' and de-annotate it for the purposes of comparison/etc.  This is actually quite a difficult problem.

>> so there could likely be places that need updating to look "through" the attributed type. But unlike with arbitrary plugins, the number of places is hopefully more reasonable.
>
> IIUC, this already happens to some extent for other type attributes -- correct? For example, the nullability attributes (`_Nonnull` and the like). However, these can only be applied to pointer types (e.g. `int * _Nonnull ptr`), while an `annotate_type` attribute could also be applied to other types (e.g. `int [[clang::annotate_type("foo")]] i`). Is this an example of the places you're thinking of that would need to be touched?

_Nonnull has exactly the problems we are talking about; it is put on the attributed-type, which end up getting lost in canonicalization just about immediately.

>>> If the same concerns apply to both of these approaches, do you have any suggestions for alternative ways that I could add annotations to types? Or does this mean that I would have to do the work of making sure that Clang can handle `AttributedType`s in these new places after all?
>>
>> No, I think you basically have to muck about with the type system no matter what. I love the idea of plugin type attributes in theory, but I don't think we're architecturally in a place where we can do that very easily. I suspect a dedicated attribute may be easier, but that's largely a guess. Both approaches strike me as likely to have a long tail of bugs we'll have to fight through.
>
> What would be your recommendation for the best way to approach this? I.e. after I've implemented an `annotate_type` attribute (I already have a draft patch at https://reviews.llvm.org/D111548), how would I best test it for possible issues? I would obviously try to make my tests as comprehensive as possible -- but is there anything else I could do? There obviously isn't any existing code that uses this attribute -- so should I try to locally patch the compiler in some way where it would add the attribute to as many places in the AST as possible, in an attempt to break things?

This is going to be an incredibly heavy lift, I just don't see how you can make it 'last' through the type system.  For example:

  template<typename T>
  struct S {
    using my_type = T;
  };

  using my_attr_type = int [[annotated_type("asdf")]];

  S<int>::my_type;
  S<my_attr_type>::my_type; <-- will have lost the attribute already, since you want them to behave as the same type.
  std::is_same<int, my_attr_type>::value; // Do you want this to be true?

  std::is_same<S<int>, S<my_attr_type>::value; // What about this one?

This problem ends up being generally intractable as far as I can imagine.  The C++ language doesn't have the idea of strong-typedefs, a type is just the type.  You can't have 1 instance of the type have an attribute, and others not without them being DIFFERENT types.  So you'd have to have some level of AnnotatedType in the type system that does its best to ACT like its underlying type (at great development cost to make sure it doesn't get lost, basically everywhere), but actually isn't.  For example, it would have to end up failing the 2nd `is_same` above, which, IMO, would be language breaking unless the 1st `is_same` ALSO stopped being true.

> In D114235#3244088 <https://reviews.llvm.org/D114235#3244088>, @erichkeane wrote:
>
>> Right, yeah, so there are a couple of problems with AttributedType.  First, it gets lost almost as soon as you get out of SemaType about 90% of the time.  Anything that does some level of canonicalization ends up losing it, so the AttributedType information is lost almost immediately.  This is why the current ones all store information in the ExtInfo.  The worst place for this ends up being in the template instantiator, which immediately canonicalizes/desugars types all over the place.
>>
>> However, making AttributedType 'survive' is actually going to be troublesome as well. A lot of the type-checking compares types using == on their pointer values, so that would be broken if they are an AttributedType.
>>
>> So I think the 'first' thing that needs to happen is to make the entire CFE 'AttributedType' maintaining, AND tolerant. I can't think of a good way to do that reliably (my naive thought would be to come up with some way to temporarily (during development) wrap EVERY type in an AttributedType with a random attribute (so that they are all unique!) and do comparisons that way. Additionally, you'd need SOMETHING to validate that the AttributedTypes are the only one that survives (again, during development) to make sure it 'works right'.
>>
>> Additionally, you'll likely  have a lot of work to do in the template engine to make sure that the attributes are inherited correctly through  instantiations, specializations, etc.
>
> Thanks for pointing out those issues!
>
> I think, if possible, I'd like to avoid having to make `AttributedType`
>
> Edit: Sorry, pressed "submit" too soon. Currently editing the comment, will remove this line once I'm done.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114235