[PATCH] D133029: [Sema] Allow to diagnose the references to std::vector<T> with incomplete T

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a subscriber: aaron.ballman.
ilya-biryukov added a comment.

In D133029#3766058 <https://reviews.llvm.org/D133029#3766058>, @shafik wrote:

> This old cfe-dev thread might be relevant: https://lists.llvm.org/pipermail/cfe-dev/2018-June/058338.html

Thanks, the thread is definitely relevant.

> It appears that folks take advantage of this working a lot. It sounds like your saying w/ constexpr std::vector this would be ill-formed in a constant expression context now. Which won't effect most of the existing code I think?

Most of the code is not affected, but some code is and large codebases will definitely see quite some breakages.
Note that this warning diagnoses is very pedantic in terms of implementing the rules from the standard. It fires in many places than don't actually break in libc++ and C++20 mode. On the flip side, it is really easy to implement.

In D133029#3778230 <https://reviews.llvm.org/D133029#3778230>, @dblaikie wrote:

> any chance of generalizing this beyond a special case for `std::vector` - an attribute we could add to any class template (perhaps particular template parameters to the class template) to document the requirement? (is std::vector the only type in the standard library that has this property, or only the one people trip over most often)

It should be easy to generalize, e.g. we could introduce an attribute to mark classes that want this behavior, something along the lines of:

  // Option 1. Less general, easy, small overhead.
  template <class T> struct [[member_access_requires_complete(T)]] my_array {};
  // Option 2. More general, might be high-overhead depending on the actual expression.
  template <class T> struct [[member_access_requires(sizeof (T))]] my_array {}; // checks for validity of expression on every member access?

I am not sure this carries it weight for that particular use-case and there are other things to consider, e.g. how to report error messages.
What is definitely useful and cheap is adding these warnings for more STL containers that have this requirement.

We discussed this with @aaron.ballman in the last Clang C++ Language WG meeting and one idea that popped up is implementing a warning in Clang, but never actually surfacing it in the compiler, only in `clang-tidy`. I would be very comfortable making a `clang-tidy` warning like this as the bar is much lower than the compiler warnings.
My proposal would be to allow making warnings that are disabled by default and even with `-Weverything`, the only way to enable them would be to pass `-W<warning-group>` explicitly or run a corresponding `clang-tidy` check. Alternatively, there will be no way to enable the warning in the compiler at all and the clang-tidy would be the only way to surface it.

Obviously, these use-cases should be rare as most things can be implemented directly in `clang-tidy`, but in that particular case we need access to the compiler state in the middle of processing the TU, something that `clang-tidy` APIs do not provide. We should also be careful to make sure any warnings like this do not hurt performance of the compiler.

@aaron.ballman does that look reasonable? I am about to try prototyping this. Are there any open questions we might want to agree on beforehand?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133029