[Clang] Remove unnecessary Attr.isArgIdent checks.

[fhahn]

Summary

The MatrixType, ExtVectorType, VectorSize and AddressSpace attributes
have arguments defined as ExprArguments in Attr.td. So their arguments
should never be ArgIdents and the logic to handle this case can be
removed.

The logic has been replaced by an assertion to ensure the arguments
are always ArgExpressions

[fhahn]

Also remove Attr.isArgIdent check from AddressSpaceTypeAttribute

[erichkeane]

I'm not sure how well Attr.td's constraints are enforced on type attributes, as these often happen before parsing is completely done. I'd imagine this code was put into place at least the 1st time for good reason, but I'm curious as to why we wouldn't have tests that cover that (or, as you assert, it could simply be that this is simply dead code).

I'm generally OK with this (the asserts are unnecessary), but would like @aaron.ballman to double check my expectations here.

clang/lib/Sema/SemaType.cpp

7661

I think the asserts aren't necessary, isArgExpr does a ArgsUnion::get<Expr*>, which already asserts.

[fhahn]

In D94092#2479684, @erichkeane wrote:

I'm not sure how well Attr.td's constraints are enforced on type attributes, as these often happen before parsing is completely done. I'd imagine this code was put into place at least the 1st time for good reason, but I'm curious as to why we wouldn't have tests that cover that (or, as you assert, it could simply be that this is simply dead code).

I'm generally OK with this (the asserts are unnecessary), but would like @aaron.ballman to double check my expectations here.

It would be great if we could get confirmation!

I tried a few different things to construct matrix_type attributes with ArgIdents, but failed. The patch also adjusts the code for a bunch of attributes. So if there are indeed cases where ArgIdents can show up, we will get some examples for unit tests.

[fhahn]

clang/lib/Sema/SemaType.cpp

7661

Good point, I removed the assertions. It's even more compact now :)

[RKSimon]

clang/lib/Sema/SemaType.cpp

7684

(style) SizeExpr

7965–7966

these comments seem unnecessary now?

[erichkeane]

In D94092#2479733, @fhahn wrote:

In D94092#2479684, @erichkeane wrote:

I'm not sure how well Attr.td's constraints are enforced on type attributes, as these often happen before parsing is completely done. I'd imagine this code was put into place at least the 1st time for good reason, but I'm curious as to why we wouldn't have tests that cover that (or, as you assert, it could simply be that this is simply dead code).

I'm generally OK with this (the asserts are unnecessary), but would like @aaron.ballman to double check my expectations here.

It would be great if we could get confirmation!

I tried a few different things to construct matrix_type attributes with ArgIdents, but failed. The patch also adjusts the code for a bunch of attributes. So if there are indeed cases where ArgIdents can show up, we will get some examples for unit tests.

The comment claims: "// Special case where the argument is a template id.". I would expect one of the following to hit that:
https://godbolt.org/z/znYW1s

[fhahn]

In D94092#2479753, @erichkeane wrote:

I tried a few different things to construct matrix_type attributes with ArgIdents, but failed. The patch also adjusts the code for a bunch of attributes. So if there are indeed cases where ArgIdents can show up, we will get some examples for unit tests.

The comment claims: "// Special case where the argument is a template id.". I would expect one of the following to hit that:
https://godbolt.org/z/znYW1s

I tried this snippet with the patch (and also an added assert(!Attr.isArgIdent(0)); to HandleVectorSizeAttr just to be sure). Compiled as expected, no crash.

[erichkeane]

In D94092#2479765, @fhahn wrote:

In D94092#2479753, @erichkeane wrote:

I tried a few different things to construct matrix_type attributes with ArgIdents, but failed. The patch also adjusts the code for a bunch of attributes. So if there are indeed cases where ArgIdents can show up, we will get some examples for unit tests.

The comment claims: "// Special case where the argument is a template id.". I would expect one of the following to hit that:
https://godbolt.org/z/znYW1s

I tried this snippet with the patch (and also an added assert(!Attr.isArgIdent(0)); to HandleVectorSizeAttr just to be sure). Compiled as expected, no crash.

Hmm... then I believe that you are correct that this is dead code. I can't come up with any other template-ids that would cause this to work differently.

I'll accept, but please give @aaron.ballman a day to take a look before submitting.

[fhahn]

Address Simon's comments, thanks!

[aaron.ballman]

In D94092#2479684, @erichkeane wrote:

I'm not sure how well Attr.td's constraints are enforced on type attributes, as these often happen before parsing is completely done.

Type (and statement) attributes do not have the same automatic error checking that declaration attributes enjoy (yet), so perhaps this was misguided error handling code.

I'd imagine this code was put into place at least the 1st time for good reason, but I'm curious as to why we wouldn't have tests that cover that (or, as you assert, it could simply be that this is simply dead code).

I think it's dead code. In ParseAttributeArgsCommon() in ParseDecl.cpp, we check to see whether the attribute definition from Attr.td specifies an identifier argument or not. If it doesn't (which these don't), then it parses the identifier as an expression instead. So I don't think there's a way to hit these code paths, even in template cases.

I'm generally OK with this (the asserts are unnecessary), but would like @aaron.ballman to double check my expectations here.

My expectation is that it's dead code, but I've been surprised before. :-D

[rjmccall]

Without bothering to look it up, I would guess that the attribute-parsing code used to generically handle the ambiguity between identifier expressions and identifier attribute arguments by just always parsing simple identifiers as identifier arguments, making it Sema's responsibility to turn that back into an expression. At some point, the parser was made sensitive to the actual attribute being parsed, but we never bothered to simplify Sema. At any rate, the parser does now know exactly which argument of which attribute it's parsing, so there's zero reason for it to force this complexity on Sema anymore; if we find a case that parses identifier arguments, we should fix it in the parser to parse an expression instead.

[aaron.ballman]

In D94092#2481276, @rjmccall wrote:

Without bothering to look it up, I would guess that the attribute-parsing code used to generically handle the ambiguity between identifier expressions and identifier attribute arguments by just always parsing simple identifiers as identifier arguments, making it Sema's responsibility to turn that back into an expression. At some point, the parser was made sensitive to the actual attribute being parsed, but we never bothered to simplify Sema. At any rate, the parser does now know exactly which argument of which attribute it's parsing, so there's zero reason for it to force this complexity on Sema anymore; if we find a case that parses identifier arguments, we should fix it in the parser to parse an expression instead.

I don't think it will be quite that trivial (switching all identifiers to be parsed as expressions instead). For instance, attributes that take enumeration arguments can parse those arguments as identifiers, but those identifiers will never be found by usual expression lookup because they don't really exist. That said, any attribute that currently accepts an identifier because it really wants to use that identifier in an expression in Sema should update the attribute argument clauses in Attr.td and make the corresponding changes in SemaDeclAttr.cpp/SemaStmt.cpp/SemaType.cpp as appropriate.

[erichkeane]

In D94092#2481857, @aaron.ballman wrote:

In D94092#2481276, @rjmccall wrote:

Without bothering to look it up, I would guess that the attribute-parsing code used to generically handle the ambiguity between identifier expressions and identifier attribute arguments by just always parsing simple identifiers as identifier arguments, making it Sema's responsibility to turn that back into an expression. At some point, the parser was made sensitive to the actual attribute being parsed, but we never bothered to simplify Sema. At any rate, the parser does now know exactly which argument of which attribute it's parsing, so there's zero reason for it to force this complexity on Sema anymore; if we find a case that parses identifier arguments, we should fix it in the parser to parse an expression instead.

I don't think it will be quite that trivial (switching all identifiers to be parsed as expressions instead). For instance, attributes that take enumeration arguments can parse those arguments as identifiers, but those identifiers will never be found by usual expression lookup because they don't really exist. That said, any attribute that currently accepts an identifier because it really wants to use that identifier in an expression in Sema should update the attribute argument clauses in Attr.td and make the corresponding changes in SemaDeclAttr.cpp/SemaStmt.cpp/SemaType.cpp as appropriate.

Right, there is an Identifier type (and collection type) in Attr.td that at least a handful of attributes are using intentionally. It is typically a case where GCC accepted an identifier, and it isn't always something that is look-up-able via normal C++ mechanisms (such as in the case of cpu_dispatch, where the identifiers are CPU names).

EDIT:
That said, I believe @rjmccall is correct, I think based on my run through history (and knowing a bit about it from past digging) that the vector_size attribute was added before attribute parsing did nearly as much as it did (I think it even predates Attr.td).

I suspect the rest of the attributes (Particularly Matrix and Address Space!) simply copy/pasted that section and edited it to work, despite not having a reproducer that hit it.

[fhahn]

In D94092#2482035, @erichkeane wrote:

In D94092#2481857, @aaron.ballman wrote:

In D94092#2481276, @rjmccall wrote:

Without bothering to look it up, I would guess that the attribute-parsing code used to generically handle the ambiguity between identifier expressions and identifier attribute arguments by just always parsing simple identifiers as identifier arguments, making it Sema's responsibility to turn that back into an expression. At some point, the parser was made sensitive to the actual attribute being parsed, but we never bothered to simplify Sema. At any rate, the parser does now know exactly which argument of which attribute it's parsing, so there's zero reason for it to force this complexity on Sema anymore; if we find a case that parses identifier arguments, we should fix it in the parser to parse an expression instead.

I don't think it will be quite that trivial (switching all identifiers to be parsed as expressions instead). For instance, attributes that take enumeration arguments can parse those arguments as identifiers, but those identifiers will never be found by usual expression lookup because they don't really exist. That said, any attribute that currently accepts an identifier because it really wants to use that identifier in an expression in Sema should update the attribute argument clauses in Attr.td and make the corresponding changes in SemaDeclAttr.cpp/SemaStmt.cpp/SemaType.cpp as appropriate.

Right, there is an Identifier type (and collection type) in Attr.td that at least a handful of attributes are using intentionally. It is typically a case where GCC accepted an identifier, and it isn't always something that is look-up-able via normal C++ mechanisms (such as in the case of cpu_dispatch, where the identifiers are CPU names).

EDIT:
That said, I believe @rjmccall is correct, I think based on my run through history (and knowing a bit about it from past digging) that the vector_size attribute was added before attribute parsing did nearly as much as it did (I think it even predates Attr.td).

I suspect the rest of the attributes (Particularly Matrix and Address Space!) simply copy/pasted that section and edited it to work, despite not having a reproducer that hit it.

For the matrix_type attribute, I originally just copied & adjusted the ext_vector_type version.

Thanks for all the comments, I'll go ahead and land this now. We should know soon enough if there's anything out in the wild that can pass an ArgIdent.

[rjmccall]

In D94092#2481857, @aaron.ballman wrote:

In D94092#2481276, @rjmccall wrote:

Without bothering to look it up, I would guess that the attribute-parsing code used to generically handle the ambiguity between identifier expressions and identifier attribute arguments by just always parsing simple identifiers as identifier arguments, making it Sema's responsibility to turn that back into an expression. At some point, the parser was made sensitive to the actual attribute being parsed, but we never bothered to simplify Sema. At any rate, the parser does now know exactly which argument of which attribute it's parsing, so there's zero reason for it to force this complexity on Sema anymore; if we find a case that parses identifier arguments, we should fix it in the parser to parse an expression instead.

I don't think it will be quite that trivial (switching all identifiers to be parsed as expressions instead). For instance, attributes that take enumeration arguments can parse those arguments as identifiers, but those identifiers will never be found by usual expression lookup because they don't really exist. That said, any attribute that currently accepts an identifier because it really wants to use that identifier in an expression in Sema should update the attribute argument clauses in Attr.td and make the corresponding changes in SemaDeclAttr.cpp/SemaStmt.cpp/SemaType.cpp as appropriate.

You're exactly right about how it's necessary to parse certain attributes. Just to be clear, though, I'm not proposing any change here; I'm describing the change that already happened, years ago, by way of trying to identify why this particular bit of code was once necessary and thus why it's likely unnecessary now. So I think you will find that those changes to Attr.td are in fact already in place.

clang/lib/Sema/SemaType.cpp

static void HandleVectorSizeAttr(QualType &CurType, const ParsedAttr &Attr,
   // Check the attribute arguments.
   if (Attr.getNumArgs() != 1) {
     S.Diag(Attr.getLoc(), diag::err_attribute_wrong_number_arguments) << Attr
                                                                       << 1;
     Attr.setInvalid();
     return;
   }
 
-  Expr *SizeExpr;
-  // Special case where the argument is a template id.
-  if (Attr.isArgIdent(0)) {
-    CXXScopeSpec SS;
-    SourceLocation TemplateKWLoc;
-    UnqualifiedId Id;
-    Id.setIdentifier(Attr.getArgAsIdent(0)->Ident, Attr.getLoc());
-
-    ExprResult Size = S.ActOnIdExpression(S.getCurScope(), SS, TemplateKWLoc,
-                                          Id, /*HasTrailingLParen=*/false,
-                                          /*IsAddressOfOperand=*/false);
-
-    if (Size.isInvalid())
-      return;
-    SizeExpr = Size.get();
-  } else {
-    SizeExpr = Attr.getArgAsExpr(0);
-  }
+  assert(Attr.isArgExpr(0) && "Argument to should be an expression");
+  Expr *SizeExpr = Attr.getArgAsExpr(0);
 
   QualType T = S.BuildVectorType(CurType, SizeExpr, Attr.getLoc());
   if (!T.isNull())
     CurType = T;
   else
     Attr.setInvalid();
 }
 
 /// Process the OpenCL-like ext_vector_type attribute when it occurs on
 /// a type.
 static void HandleExtVectorTypeAttr(QualType &CurType, const ParsedAttr &Attr,
                                     Sema &S) {
   // check the attribute arguments.
   if (Attr.getNumArgs() != 1) {
     S.Diag(Attr.getLoc(), diag::err_attribute_wrong_number_arguments) << Attr
                                                                       << 1;
     return;
   }
 
-  Expr *sizeExpr;
-
-  // Special case where the argument is a template id.
-  if (Attr.isArgIdent(0)) {
-    CXXScopeSpec SS;
-    SourceLocation TemplateKWLoc;
-    UnqualifiedId id;
-    id.setIdentifier(Attr.getArgAsIdent(0)->Ident, Attr.getLoc());
-
-    ExprResult Size = S.ActOnIdExpression(S.getCurScope(), SS, TemplateKWLoc,
-                                          id, /*HasTrailingLParen=*/false,
-                                          /*IsAddressOfOperand=*/false);
-    if (Size.isInvalid())
-      return;
-
-    sizeExpr = Size.get();
-  } else {
-    sizeExpr = Attr.getArgAsExpr(0);
-  }
+  assert(Attr.isArgExpr(0) && "Argument to should be an expression");
+  Expr *sizeExpr = Attr.getArgAsExpr(0);
 
   // Create the vector type.
   QualType T = S.BuildExtVectorType(CurType, sizeExpr, Attr.getLoc());
   if (!T.isNull())
     CurType = T;
 }
 
 static bool isPermittedNeonBaseType(QualType &Ty,
                                     VectorType::VectorKind VecKind, Sema &S) {
if (Attr.getNumArgs() != 2) {
     S.Diag(Attr.getLoc(), diag::err_attribute_wrong_number_arguments)
         << Attr << 2;
     return;
   }
 
   Expr *RowsExpr = nullptr;
   Expr *ColsExpr = nullptr;
 
-  // TODO: Refactor parameter extraction into separate function
   // Get the number of rows
-  if (Attr.isArgIdent(0)) {
-    CXXScopeSpec SS;
-    SourceLocation TemplateKeywordLoc;
-    UnqualifiedId id;
-    id.setIdentifier(Attr.getArgAsIdent(0)->Ident, Attr.getLoc());
-    ExprResult Rows = S.ActOnIdExpression(S.getCurScope(), SS,
-                                          TemplateKeywordLoc, id, false, false);
-
-    if (Rows.isInvalid())
-      // TODO: maybe a good error message would be nice here
-      return;
-    RowsExpr = Rows.get();
-  } else {
-    assert(Attr.isArgExpr(0) &&
-           "Argument to should either be an identity or expression");
-    RowsExpr = Attr.getArgAsExpr(0);
-  }
+  assert(Attr.isArgExpr(0) && "Argument should be an expression");
 
   // Get the number of columns
-  if (Attr.isArgIdent(1)) {
-    CXXScopeSpec SS;
-    SourceLocation TemplateKeywordLoc;
-    UnqualifiedId id;
-    id.setIdentifier(Attr.getArgAsIdent(1)->Ident, Attr.getLoc());
-    ExprResult Columns = S.ActOnIdExpression(
-        S.getCurScope(), SS, TemplateKeywordLoc, id, false, false);
-
-    if (Columns.isInvalid())
-      // TODO: a good error message would be nice here
-      return;
-    RowsExpr = Columns.get();
-  } else {
-    assert(Attr.isArgExpr(1) &&
-           "Argument to should either be an identity or expression");
-    ColsExpr = Attr.getArgAsExpr(1);
-  }
+  assert(Attr.isArgExpr(1) && "Argument to should be an expression");
 
   // Create the matrix type.
   QualType T = S.BuildMatrixType(CurType, RowsExpr, ColsExpr, Attr.getLoc());
   if (!T.isNull())
     CurType = T;
 }
 
 static void HandleLifetimeBoundAttr(TypeProcessingState &State,