[PATCH] D129748: [Modules] Disable preferred_name attribute in C++20 Modules

Tue Jul 19 01:30:31 PDT 2022

ChuanqiXu added a comment.

In D129748#3660684 <https://reviews.llvm.org/D129748#3660684>, @aaron.ballman wrote:

> The argument type is `TypeArgument`, not `TypdefType`. The guts of the reading code for the preferred name attribute *should* be:

Oh, My bad. I didn't mention it in the above response. In https://github.com/llvm/llvm-project/blob/469044cfd355d34573643a57b5d2a78a9c341327/clang/lib/Sema/SemaDeclAttr.cpp#L1427-L1456, we could find that the type of `PreferredName` must be a type def type to a record type.

> I'm still not quite seeing that this only impacts one attribute. I *think* this would happen for the others listed if the attribute argument was similarly a typedef as with your example using `preferred_name`.

After experimenting the all day, I can't say the similar problem won't definitely happen to other attributes if they using a typedef as the attribute argument.

---

Here is the story of my experiment. The readers could skip this if not interested. `IBOutletCollection` is an Objective-C attributes. And the type argument of `TypeTagForDatatype` is required to be integral type: https://releases.llvm.org/9.0.0/tools/clang/docs/AttributeReference.html#type-tag-for-datatype.  So let's see about `gsl::Owner` and `gsl::Pointer`. They are symmetric so let's see `gsl::Owner`.

First, I tried to following example, which replace [[gsl::Owner]] with `preferred_name`:

  template<class _CharT>
  class basic_string_view;

  typedef basic_string_view<char> string_view;

  template<class _CharT>
  class
  [[gsl::Owner(string_view)]]
  basic_string_view {
  public:
      basic_string_view() 
      {
      }
  };

  inline basic_string_view<char> foo()
  {
    return basic_string_view<char>();
  }

Then it don't meet the ODR violation problem. The reason is in the following AST:

  |-ClassTemplateDecl 0x7914038 <string_view.h:1:1, line:2:7> col:7 basic_string_view
  | |-TemplateTypeParmDecl 0x7913ec0 <line:1:10, col:16> col:16 class depth 0 index 0 _CharT
  | |-CXXRecordDecl 0x7913f88 <line:2:1, col:7> col:7 class basic_string_view
  | | `-OwnerAttr 0x7914670 <line:9:8, col:25> string_view
  | `-ClassTemplateSpecializationDecl 0x7914240 <line:1:1, line:2:7> line:10:1 class basic_string_view definition
  |   |-DefinitionData pass_in_registers empty standard_layout trivially_copyable has_user_declared_ctor can_const_default_init
  |   | |-DefaultConstructor exists non_trivial user_provided defaulted_is_constexpr
  |   | |-CopyConstructor simple trivial has_const_param implicit_has_const_param
  |   | |-MoveConstructor exists simple trivial
  |   | |-CopyAssignment simple trivial has_const_param needs_implicit implicit_has_const_param
  |   | |-MoveAssignment exists simple trivial needs_implicit
  |   | `-Destructor simple irrelevant trivial constexpr
  |   |-TemplateArgument type 'char'
  |   | `-BuiltinType 0x78c7e50 'char'
  |   |-OwnerAttr 0x7914b98 <line:9:8, col:25> string_view
  |   |-CXXRecordDecl 0x7914c68 <line:7:1, line:10:1> col:1 implicit class basic_string_view
  |   |-AccessSpecDecl 0x7914d18 <line:11:1, col:7> col:1 public
  |   |-CXXConstructorDecl 0x7933d20 <line:12:5, line:14:5> line:12:5 used basic_string_view 'void ()'
  |   | `-CompoundStmt 0x7914978 <line:13:5, line:14:5>
  |   |-CXXConstructorDecl 0x7933ed0 <line:10:1> col:1 implicit constexpr basic_string_view 'void (const basic_string_view<char> &)' inline default trivial noexcept-unevaluated 0x7933ed0
  |   | `-ParmVarDecl 0x7933ff0 <col:1> col:1 'const basic_string_view<char> &'
  |   |-CXXConstructorDecl 0x79340a0 <col:1> col:1 implicit constexpr basic_string_view 'void (basic_string_view<char> &&)' inline default trivial noexcept-unevaluated 0x79340a0
  |   | `-ParmVarDecl 0x79341c0 <col:1> col:1 'basic_string_view<char> &&'
  |   `-CXXDestructorDecl 0x79343a0 <col:1> col:1 implicit referenced constexpr ~basic_string_view 'void () noexcept' inline default trivial
  |-TypedefDecl 0x79143e8 <line:4:1, col:33> col:33 referenced string_view 'basic_string_view<char>':'basic_string_view<char>'
  | `-TemplateSpecializationType 0x7914360 'basic_string_view<char>' sugar basic_string_view
  |   |-TemplateArgument type 'char'
  |   | `-BuiltinType 0x78c7e50 'char'
  |   `-RecordType 0x7914340 'basic_string_view<char>'
  |     `-ClassTemplateSpecialization 0x7914240 'basic_string_view'
  |-ClassTemplateDecl 0x79145b0 prev 0x7914038 <line:6:1, line:15:1> line:10:1 basic_string_view
  | |-TemplateTypeParmDecl 0x7914448 <line:6:10, col:16> col:16 class depth 0 index 0 _CharT
  | |-CXXRecordDecl 0x7914518 prev 0x7913f88 <line:7:1, line:15:1> line:10:1 class basic_string_view definition
  | | |-DefinitionData empty standard_layout trivially_copyable has_user_declared_ctor can_const_default_init
  | | | |-DefaultConstructor exists non_trivial user_provided defaulted_is_constexpr
  | | | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
  | | | |-MoveConstructor exists simple trivial needs_implicit
  | | | |-CopyAssignment simple trivial has_const_param needs_implicit implicit_has_const_param
  | | | |-MoveAssignment exists simple trivial needs_implicit
  | | | `-Destructor simple irrelevant trivial constexpr needs_implicit
  | | |-OwnerAttr 0x79146d0 <line:9:8, col:25> string_view
  | | |-CXXRecordDecl 0x7914738 <line:7:1, line:10:1> col:1 implicit referenced class basic_string_view
  | | |-AccessSpecDecl 0x79147e8 <line:11:1, col:7> col:1 public
  | | `-CXXConstructorDecl 0x79148a0 <line:12:5, line:14:5> line:12:5 basic_string_view<_CharT> 'void ()'
  | |   `-CompoundStmt 0x7914978 <line:13:5, line:14:5>
  | `-ClassTemplateSpecialization 0x7914240 'basic_string_view'

The most significant difference with `preferred_name` is: there is another `OwnerAttr` in `CXXRecordDecl` at `0x7913f88`. So it would read the attributes before it starts to read `ClassTemplateSpecializationDecl`. This is done by https://github.com/llvm/llvm-project/blob/469044cfd355d34573643a57b5d2a78a9c341327/clang/lib/Sema/SemaDeclAttr.cpp#L5051-L5052. When we handle `OwnerAttr`, it would add the attribute for every redeclaration. But `PreferredNameAttr` would only add the attribute for the declaration itself. So here is the problem.

Then I tried to add `PreferredNameAttr` for every redeclaration. But I met two problems.

(1) It would meet an infinite instantiation problem for the following case:

  C++
  template<int A, int B, typename ...T> struct Foo;
  template<typename ...T> using Bar = Foo<1, 2, T...>;
  using Bar2 = Foo<1, 2, int, float>;
  template<int A, int B, typename ...T> struct [[clang::preferred_name(::Bar<T...>)]] Foo {};
  Foo<1, 2, int, float>::nosuch x; // expected-error {{no type named 'nosuch' in 'preferred_name::Bar<int, float>'}}

  ::Foo<1, 2, int, float>::nosuch x; // expected-error {{no type named 'nosuch' in 'preferred_name::Bar<int, float>'}}

But `gsl::Owner` wouldn't meet the problem. And I haven't found the reason.

(2) It would crash when compiling stdmodules. The crash stack is relatively long and I haven't located the reason. So I omit them here.

---

@tahonermann @aaron.ballman As a summary, I admit this revision is something like "I met a problem that I couldn't solve so I tried to skip it instead of solving it". I understand this is generally bad. I have fought with the problem for about 2 weeks. Then I found things goes pretty good after I tried to skip it. Now the stdmodules have these examples: https://github.com/ChuanqiXu9/stdmodules/tree/master/examples. I know this is completely incomplete. But it should be a good direction. And I am trying to use it in actual projects, too. And the blocking issue is the `preferred_name`, which is a printer hinter to me. So I just feel like it may be not too bad to skip it. And clang15 is going to be released recently. It would be good to have something that people could have fun with. I know the current status of C++20 Modules is not mature really. But it would be really helpful if some users are willing to eat dog foods to give feedbacks. It looks like I am the only one tester for C++20 Modules now.

It looks like @erichkeane likes the idea. How do you think about it @tahonermann  @aaron.ballman?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129748