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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
