[libcxx-commits] [PATCH] D136533: [clang] Fix missing diagnostic of declaration use when accessing TypeDecls through typename access

Tue Nov 1 10:36:22 PDT 2022

ldionne added a comment.

In D136533#3896536 <https://reviews.llvm.org/D136533#3896536>, @mizvekov wrote:

> Now that you mention it, looking at the code, I think we don't diagnose an use of a type alias itself, if that is what you mean?
>
> Ie, clang doesn't, GCC does, MSVC doesn't: https://godbolt.org/z/WEo4enjbz
>
> Would fixing that be a problem for libc++?

It might be a problem, but I would argue we should still do it after fixing any problematic cases. It seems like Clang's current behavior is broken, as it basically ignores the `[[deprecated]]` attribute on aliases?

> Otherwise, typename accesses through the type alias pattern itself should work like from any context, except that if the access happens through a dependent entity, we should be diagnosing it when we instantiate the type alias.
>
> Does what you are talking about fall into any of that? Otherwise, do you have a short example?

Sorry, I'm a bit lost. Let's take it back to the errors we're actually seeing with my reproducer above:

  In file included from <stdin>:1:
  In file included from SDK/usr/include/c++/v1/filesystem:245:
  In file included from SDK/usr/include/c++/v1/__filesystem/directory_entry.h:14:
  In file included from SDK/usr/include/c++/v1/__chrono/time_point.h:13:
  In file included from SDK/usr/include/c++/v1/__chrono/duration.h:14:
  In file included from SDK/usr/include/c++/v1/limits:107:
  In file included from SDK/usr/include/c++/v1/type_traits:421:
  In file included from SDK/usr/include/c++/v1/__functional/invoke.h:17:
  In file included from SDK/usr/include/c++/v1/__type_traits/decay.h:13:
  SDK/usr/include/c++/v1/__type_traits/add_pointer.h:26:50: error: 'type' is unavailable: introduced in macOS 10.15
                  _IsSame<typename remove_cv<_Tp>::type, void>::value>
                                                   ^
  SDK/usr/include/c++/v1/__type_traits/add_pointer.h:33:39: note: in instantiation of default argument for '__add_pointer_impl<std::filesystem::file_status>' required here
      {typedef _LIBCPP_NODEBUG typename __add_pointer_impl<_Tp>::type type;};
                                        ^~~~~~~~~~~~~~~~~~~~~~~
  SDK/usr/include/c++/v1/__type_traits/decay.h:44:40: note: in instantiation of template class 'std::add_pointer<std::filesystem::file_status>' requested here
                                typename add_pointer<_Up>::type,
                                         ^
  SDK/usr/include/c++/v1/__type_traits/decay.h:56:38: note: in instantiation of template class 'std::__decay<std::filesystem::file_status, true>' requested here
      typedef _LIBCPP_NODEBUG typename __decay<_Up, __is_referenceable<_Up>::value>::type type;
                                       ^
  SDK/usr/include/c++/v1/__filesystem/path.h:142:47: note: in instantiation of template class 'std::decay<std::filesystem::file_status>' requested here
  template <class _Source, class _DS = typename decay<_Source>::type,
                                                ^
  SDK/usr/include/c++/v1/__filesystem/path.h:195:31: note: in instantiation of default argument for '__is_pathable_char_array<std::filesystem::file_status>' required here
            bool _IsCharIterT = __is_pathable_char_array<_Tp>::value,
                                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  SDK/usr/include/c++/v1/__filesystem/path.h:445:26: note: in instantiation of default argument for '__is_pathable<std::filesystem::file_status, false>' required here
        typename enable_if<__is_pathable<_SourceOrIter>::value, _Tp>::type;
                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
  SDK/usr/include/c++/v1/__filesystem/path.h:480:36: note: in instantiation of template type alias '_EnableIfPathable' requested here
    template <class _Source, class = _EnableIfPathable<_Source, void> >
                                     ^
  SDK/usr/include/c++/v1/__filesystem/path.h:482:3: note: in instantiation of default argument for 'path<std::filesystem::file_status>' required here
    path(const _Source& __src, format = format::auto_format) {
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  SDK/usr/include/c++/v1/__filesystem/operations.h:99:75: note: while substituting deduced template arguments into function template 'path' [with _Source = file_status, $1 = (no value)]
  inline _LIBCPP_HIDE_FROM_ABI bool exists(const path& __p) { return exists(__status(__p)); }
                                                                            ^
  SDK/usr/include/c++/v1/__filesystem/file_status.h:28:24: note: 'file_status' has been explicitly marked unavailable here
  class _LIBCPP_TYPE_VIS file_status {
                         ^

`file_status`, `exists` and `path::path` are all marked as unavailable. Why do we diagnose the use of an unavailable type wayyy lower in the stack when we instantiate `remove_cv`? I am not seeing what libc++ is doing wrong.

> Otherwise, if current libc++ is fine and building older libc++with newer clang is not supported, is everyone fine with merging this back, as is?
> @thakis ?

See below for what I meant by "unsupported". Another way to think about this would be to say that Clang has the burden of keeping a slightly-older libc++ compiling. If that's the case, then one could argue that this change should be held off until Clang no longer cares about being able to compile that version of libc++. This is not really my decision to make, but I would argue that there's probably value in keeping things working (that would bite people not only on OS X, but anyone using an older libc++ with a newer Clang).

If so, then perhaps it would make sense to wait a few months before we merge this, perhaps at least until LLVM 16 is out so there's an officially-released version of libc++ that doesn't break with this change. But I think that's a decision for the Clang folks to make, not libc++.

In D136533#3897516 <https://reviews.llvm.org/D136533#3897516>, @glandium wrote:

>> you're using the SDK libc++ with a ToT clang, which is technically not a supported combination.
>
> Where is it specified that it's not a supported combination? Why should it not be supported? You don't even get libc++ from the llvm tree unless you explicitly enable it.

I guess I should reword it that way: If you're using an older libc++ with a newer Clang, there is literally no way for that old libc++ to guarantee that it can work with that not-known-yet Clang. It would imply being forward compatible with potentially arbitrary changes in Clang, which is nonsensical. IOW, we can't guard against issues that we don't know exist yet. Does that make sense?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136533