[AST] Add more source information for DecltypeTypeLoc.

[hokein]

Summary

Adds the paren source location, and removes the hack in clangd.

[hokein]

clang/lib/Parse/ParseDeclCXX.cpp

1012

This is unfortunate for a common case decltype(expression) a;, the ParseDecltypeSpecifier is called twice to parse the decltype specifier:

• first time, we parse the raw decltype token, and we annotate that (the DS was created locally, and then was thrown away)
• second time, we parsed the annotated decltype, and set another DS, at this point, we lost the LParen information :(

One way to fix that is to add a new field SourceLocation in Token class (there is 4-bytes padding size left, so it won't increase its size), but I'm a little worried, it seems like an overkill to just fix this issue.

[sammccall]

Thank you for digging into this!

(BTW the parsing part of this is probably adjacent to a fix for https://github.com/clangd/clangd/issues/121, AutoType also doesn't have enough locations for decltype(auto). But definitely not something to touch in this patch)

clang/lib/Parse/ParseDeclCXX.cpp

1012

What do you think about only storing the kw loc + endloc then?
This is enough to compute the correct sourcerange, which was the original bug we hit.

Our options are:

• do the work to always compute it: nice to have but it does seem hard/intrusive
• don't store/expose it
• expose it but have it be unreliable

I don't know if 3 is better than 2 (without a use case it's hard to judge), maybe it just encourages writing buggy code. And if nothing else, 2 is smaller :-)

[hokein]

don't expose LParen loc.

[hokein]

(BTW the parsing part of this is probably adjacent to a fix for https://github.com/clangd/clangd/issues/121, AutoType also doesn't have enough locations for decltype(auto). But definitely not something to touch in this patch)

Yeah, that is my next step, this is an extending change of AutoTypeLoc.

clang/lib/Parse/ParseDeclCXX.cpp

1012

Yeah, having the decltype loc + rparen loc should be enough to fix our bug, and it might be a better idea than having a partial working getLParenLoc.

I don't know if 3 is better than 2 (without a use case it's hard to judge), maybe it just encourages writing buggy code. And if nothing else, 2 is smaller :-)

We don't have a concrete use case of the paren range of decltype, so 2 is probably the best (though having a getRParenLoc API without a paired getLParenLoc feels incomplete).

[sammccall]

In D116793#3227175, @hokein wrote:

(BTW the parsing part of this is probably adjacent to a fix for https://github.com/clangd/clangd/issues/121, AutoType also doesn't have enough locations for decltype(auto). But definitely not something to touch in this patch)

Yeah, that is my next step, this is an extending change of AutoTypeLoc.

Yay :-)

clang/lib/Parse/ParseDeclCXX.cpp

1012

Agree. Would it make sense to keep the internal names concrete but expose publicly only as begin/endLoc?

[sammccall]

LG thank you!

Keeping/removing the getRParenLoc() accessor is up to you, I see arguments both ways.

clang/include/clang/AST/TypeLoc.h

2001

nit: decltype (typo)

2015

So concretely I think I'm talking about just removing this getter, since getEndLoc() will by default to the end of getLocalSourceRange().

Probably the setter would stay as is?

Anyway, up to you, it's harmless.

clang-tools-extra/clangd/Selection.cpp

for (const auto *N = Common; N; N = N->Parent) {
     }
   }
   if (Common)
     SelectionUsedRecovery.record(0, LanguageLabel); // unused.
 }
 
 // Return the range covering a node and all its children.
 SourceRange getSourceRange(const DynTypedNode &N) {
-  // DeclTypeTypeLoc::getSourceRange() is incomplete, which would lead to
-  // failing to descend into the child expression.
-  // decltype(2+2);
-  // ~~~~~~~~~~~~~ <-- correct range
-  // ~~~~~~~~      <-- range reported by getSourceRange()
-  // ~~~~~~~~~~~~  <-- range with this hack(i.e, missing closing paren)
-  // FIXME: Alter DecltypeTypeLoc to contain parentheses locations and get
-  // rid of this patch.
-  if (const auto *TL = N.get<TypeLoc>()) {
-    if (auto DT = TL->getAs<DecltypeTypeLoc>()) {
-      SourceRange S = DT.getSourceRange();
-      S.setEnd(DT.getUnderlyingExpr()->getEndLoc());
-      return S;
-    }
-  }
   // MemberExprs to implicitly access anonymous fields should not claim any
   // tokens for themselves. Given:
   //   struct A { struct { int b; }; };
   // The clang AST reports the following nodes for an access to b:
   //   A().b;
   //   [----] MemberExpr, base = A().<anonymous>, member = b
   //   [----] MemberExpr: base = A(), member = <anonymous>
   //   [-]    CXXConstructExpr

clang-tools-extra/clangd/unittests/SelectionTests.cpp

Case Cases[] = {
           "UserDefinedLiteral"},
 
       {
           R"cpp(
         int a;
         decltype([[^a]] + a) b;
         )cpp",
           "DeclRefExpr"},
-      {"[[decltype]]^(1) b;", "DecltypeTypeLoc"}, // Not the VarDecl.
+      {"[[decltype^(1)]] b;", "DecltypeTypeLoc"}, // Not the VarDecl.
 
       // Objective-C nullability attributes.
       {
           R"cpp(
             @interface I{}
             @property(nullable) [[^I]] *x;
             @end
           )cpp",

clang/include/clang/AST/TypeLoc.h

public:
 
   void setUnderlyingTInfo(TypeSourceInfo* TI) const {
     this->getLocalData()->UnderlyingTInfo = TI;
   }
 
   void initializeLocal(ASTContext &Context, SourceLocation Loc);
 };
 
-// FIXME: location of the 'decltype' and parens.
-class DecltypeTypeLoc : public InheritingConcreteTypeLoc<TypeSpecTypeLoc,
-                                                         DecltypeTypeLoc,
-                                                         DecltypeType> {
+// decltype(expression) abc;
+// ~~~~~~~~                  DecltypeLoc
+//                    ~      RParenLoc
+// FIXME: add LParenLoc, it is tricky to support due to the limitation of
+// annotated-decltype token.
+struct DecltypeTypeLocInfo {
+  SourceLocation DecltypeLoc;
+  SourceLocation RParenLoc;
+};
+class DecltypeTypeLoc
+    : public ConcreteTypeLoc<UnqualTypeLoc, DecltypeTypeLoc, DecltypeType,
+                             DecltypeTypeLocInfo> {
 public:
   Expr *getUnderlyingExpr() const { return getTypePtr()->getUnderlyingExpr(); }
+
+  SourceLocation getDecltypeLoc() const { return getLocalData()->DecltypeLoc; }
+  void setDecltypeLoc(SourceLocation Loc) { getLocalData()->DecltypeLoc = Loc; }
+
+  SourceLocation getRParenLoc() const { return getLocalData()->RParenLoc; }
+  void setRParenLoc(SourceLocation Loc) { getLocalData()->RParenLoc = Loc; }
+
+  SourceRange getLocalSourceRange() const {
+    return SourceRange(getDecltypeLoc(), getRParenLoc());
+  }
+
+  void initializeLocal(ASTContext &Context, SourceLocation Loc) {
+    setDecltypeLoc(Loc);
+    setRParenLoc(Loc);
+  }
 };
 
 struct UnaryTransformTypeLocInfo {
   // FIXME: While there's only one unary transform right now, future ones may
   // need different representations
   SourceLocation KWLoc, LParenLoc, RParenLoc;
   TypeSourceInfo *UnderlyingTInfo;
 };

clang/lib/Parse/ParseDeclCXX.cpp

SourceLocation Parser::ParseDecltypeSpecifier(DeclSpec &DS) {
 
   ExprResult Result;
   SourceLocation StartLoc = Tok.getLocation();
   SourceLocation EndLoc;
 
   if (Tok.is(tok::annot_decltype)) {
     Result = getExprAnnotation(Tok);
     EndLoc = Tok.getAnnotationEndLoc();
+    // Unfortunately, we don't know the LParen source location as the annotated
+    // token doesn't have it.
+    DS.setTypeofParensRange(SourceRange(SourceLocation(), EndLoc));
     ConsumeAnnotationToken();
     if (Result.isInvalid()) {
       DS.SetTypeSpecError();
       return EndLoc;
     }
   } else {
     if (Tok.getIdentifierInfo()->isStr("decltype"))
       Diag(Tok, diag::warn_cxx98_compat_decltype);
if (Tok.is(tok::kw_auto)) {
         return EndLoc;
       }
 
       Result = Actions.ActOnDecltypeExpression(Result.get());
     }
 
     // Match the ')'
     T.consumeClose();
+    DS.setTypeofParensRange(T.getRange());
     if (T.getCloseLocation().isInvalid()) {
       DS.SetTypeSpecError();
       // FIXME: this should return the location of the last token
       //        that was consumed (by "consumeClose()")
       return T.getCloseLocation();
     }
 
     if (Result.isInvalid()) {

clang/lib/Sema/SemaCXXScopeSpec.cpp

bool Sema::ActOnCXXNestedNameSpecifierDecltype(CXXScopeSpec &SS,
   if (!T->isDependentType() && !T->getAs<TagType>()) {
     Diag(DS.getTypeSpecTypeLoc(), diag::err_expected_class_or_namespace)
       << T << getLangOpts().CPlusPlus;
     return true;
   }
 
   TypeLocBuilder TLB;
   DecltypeTypeLoc DecltypeTL = TLB.push<DecltypeTypeLoc>(T);
-  DecltypeTL.setNameLoc(DS.getTypeSpecTypeLoc());
+  DecltypeTL.setDecltypeLoc(DS.getTypeSpecTypeLoc());
+  DecltypeTL.setRParenLoc(DS.getTypeofParensRange().getEnd());
   SS.Extend(Context, SourceLocation(), TLB.getTypeLocInContext(Context, T),
             ColonColonLoc);
   return false;
 }
 
 /// IsInvalidUnlessNestedName - This method is used for error recovery
 /// purposes to determine whether the specified identifier is only valid as
 /// a nested name specifier, for example a namespace name.  It is

clang/lib/Sema/SemaExprCXX.cpp

if (DS.getTypeSpecType() == DeclSpec::TST_decltype_auto) {
     Diag(DS.getTypeSpecTypeLoc(), diag::err_decltype_auto_invalid);
     return true;
   }
 
   QualType T = BuildDecltypeType(DS.getRepAsExpr(), /*AsUnevaluated=*/false);
 
   TypeLocBuilder TLB;
   DecltypeTypeLoc DecltypeTL = TLB.push<DecltypeTypeLoc>(T);
-  DecltypeTL.setNameLoc(DS.getTypeSpecTypeLoc());
+  DecltypeTL.setDecltypeLoc(DS.getTypeSpecTypeLoc());
+  DecltypeTL.setRParenLoc(DS.getTypeofParensRange().getEnd());
   TypeSourceInfo *DestructedTypeInfo = TLB.getTypeSourceInfo(Context, T);
   PseudoDestructorTypeStorage Destructed(DestructedTypeInfo);
 
   return BuildPseudoDestructorExpr(Base, OpLoc, OpKind, CXXScopeSpec(),
                                    nullptr, SourceLocation(), TildeLoc,
                                    Destructed);
 }
 

clang/lib/Sema/SemaType.cpp

void VisitTypeOfTypeLoc(TypeOfTypeLoc TL) {
       assert(DS.getTypeSpecType() == DeclSpec::TST_typeofType);
       TL.setTypeofLoc(DS.getTypeSpecTypeLoc());
       TL.setParensRange(DS.getTypeofParensRange());
       assert(DS.getRepAsType());
       TypeSourceInfo *TInfo = nullptr;
       Sema::GetTypeFromParser(DS.getRepAsType(), &TInfo);
       TL.setUnderlyingTInfo(TInfo);
     }
+    void VisitDecltypeTypeLoc(DecltypeTypeLoc TL) {
+      assert(DS.getTypeSpecType() == DeclSpec::TST_decltype);
+      TL.setDecltypeLoc(DS.getTypeSpecTypeLoc());
+      TL.setRParenLoc(DS.getTypeofParensRange().getEnd());
+    }
     void VisitUnaryTransformTypeLoc(UnaryTransformTypeLoc TL) {
       // FIXME: This holds only because we only have one unary transform.
       assert(DS.getTypeSpecType() == DeclSpec::TST_underlyingType);
       TL.setKWLoc(DS.getTypeSpecTypeLoc());
       TL.setParensRange(DS.getTypeofParensRange());
       assert(DS.getRepAsType());
       TypeSourceInfo *TInfo = nullptr;
       Sema::GetTypeFromParser(DS.getRepAsType(), &TInfo);

clang/lib/Sema/TreeTransform.h

QualType TreeTransform<Derived>::TransformDecltypeType(TypeLocBuilder &TLB,
 
   E = getSema().ActOnDecltypeExpression(E.get());
   if (E.isInvalid())
     return QualType();
 
   QualType Result = TL.getType();
   if (getDerived().AlwaysRebuild() ||
       E.get() != T->getUnderlyingExpr()) {
-    Result = getDerived().RebuildDecltypeType(E.get(), TL.getNameLoc());
+    Result = getDerived().RebuildDecltypeType(E.get(), TL.getDecltypeLoc());
     if (Result.isNull())
       return QualType();
   }
   else E.get();
 
   DecltypeTypeLoc NewTL = TLB.push<DecltypeTypeLoc>(Result);
-  NewTL.setNameLoc(TL.getNameLoc());
-
+  NewTL.setDecltypeLoc(TL.getDecltypeLoc());
+  NewTL.setRParenLoc(TL.getRParenLoc());
   return Result;
 }
 
 template<typename Derived>
 QualType TreeTransform<Derived>::TransformUnaryTransformType(
                                                             TypeLocBuilder &TLB,
                                                      UnaryTransformTypeLoc TL) {
   QualType Result = TL.getType();

clang/lib/Serialization/ASTReader.cpp

 void TypeLocReader::VisitTypeOfTypeLoc(TypeOfTypeLoc TL) {
   TL.setTypeofLoc(readSourceLocation());
   TL.setLParenLoc(readSourceLocation());
   TL.setRParenLoc(readSourceLocation());
   TL.setUnderlyingTInfo(GetTypeSourceInfo());
 }
 
 void TypeLocReader::VisitDecltypeTypeLoc(DecltypeTypeLoc TL) {
-  TL.setNameLoc(readSourceLocation());
+  TL.setDecltypeLoc(readSourceLocation());
+  TL.setRParenLoc(readSourceLocation());
 }
 
 void TypeLocReader::VisitUnaryTransformTypeLoc(UnaryTransformTypeLoc TL) {
   TL.setKWLoc(readSourceLocation());
   TL.setLParenLoc(readSourceLocation());
   TL.setRParenLoc(readSourceLocation());
   TL.setUnderlyingTInfo(GetTypeSourceInfo());
 }

clang/lib/Serialization/ASTWriter.cpp

 void TypeLocWriter::VisitTypeOfTypeLoc(TypeOfTypeLoc TL) {
   Record.AddSourceLocation(TL.getTypeofLoc());
   Record.AddSourceLocation(TL.getLParenLoc());
   Record.AddSourceLocation(TL.getRParenLoc());
   Record.AddTypeSourceInfo(TL.getUnderlyingTInfo());
 }
 
 void TypeLocWriter::VisitDecltypeTypeLoc(DecltypeTypeLoc TL) {
-  Record.AddSourceLocation(TL.getNameLoc());
+  Record.AddSourceLocation(TL.getDecltypeLoc());
+  Record.AddSourceLocation(TL.getRParenLoc());
 }
 
 void TypeLocWriter::VisitUnaryTransformTypeLoc(UnaryTransformTypeLoc TL) {
   Record.AddSourceLocation(TL.getKWLoc());
   Record.AddSourceLocation(TL.getLParenLoc());
   Record.AddSourceLocation(TL.getRParenLoc());
   Record.AddTypeSourceInfo(TL.getUnderlyingTInfo());
 }

clang/unittests/AST/SourceLocationTest.cpp

 }
 
 TEST(TypeLoc, LongRange) {
   RangeVerifier<TypeLoc> Verifier;
   Verifier.expectRange(1, 1, 1, 1);
   EXPECT_TRUE(Verifier.match("long a;", typeLoc()));
 }
 
+TEST(TypeLoc, DecltypeTypeLocRange) {
+  llvm::Annotations Code(R"(
+    $full1[[decltype(1)]] a;
+    struct A {struct B{};} var;
+    $full2[[decltype(var)]]::B c;
+  )");
+  auto AST = tooling::buildASTFromCodeWithArgs(Code.code(), /*Args=*/{});
+  ASTContext &Ctx = AST->getASTContext();
+  const auto &SM = Ctx.getSourceManager();
+
+  auto MatchedLocs = clang::ast_matchers::match(
+      typeLoc(loc(decltypeType())).bind("target"), Ctx);
+  ASSERT_EQ(MatchedLocs.size(), 2u);
+  auto verify = [&](SourceRange ActualRange,
+                    const llvm::Annotations::Range &Expected) {
+    auto ActualCharRange =
+        Lexer::getAsCharRange(ActualRange, SM, Ctx.getLangOpts());
+    EXPECT_EQ(SM.getFileOffset(ActualCharRange.getBegin()), Expected.Begin);
+    EXPECT_EQ(SM.getFileOffset(ActualCharRange.getEnd()), Expected.End);
+  };
+  const auto *Target1 = MatchedLocs[0].getNodeAs<DecltypeTypeLoc>("target");
+  verify(Target1->getSourceRange(), Code.range("full1"));
+
+  const auto *Target2 = MatchedLocs[1].getNodeAs<DecltypeTypeLoc>("target");
+  verify(Target2->getSourceRange(), Code.range("full2"));
+}
+
 TEST(TypeLoc, LongDoubleRange) {
   RangeVerifier<TypeLoc> Verifier;
   Verifier.expectRange(1, 1, 1, 6);
   EXPECT_TRUE(Verifier.match("long double a;", typeLoc()));
 }
 
 TEST(TypeLoc, DoubleLongRange) {
   RangeVerifier<TypeLoc> Verifier;
EXPECT_TRUE(Verifier.match("struct A;\n"
                              "struct A {\n"
                              "friend decltype(foo());\n"
                              "};\n",
                              friendDecl(), Lang_CXX11));
 }
 
 TEST(FriendDecl, FriendDecltypeRange) {
   RangeVerifier<FriendDecl> Verifier;
-  Verifier.expectRange(4, 1, 4, 8);
+  Verifier.expectRange(4, 1, 4, 22);
   EXPECT_TRUE(Verifier.match("struct A;\n"
                              "A foo();\n"
                              "struct A {\n"
                              "friend decltype(foo());\n"
                              "};\n",
                              friendDecl(), Lang_CXX11));
 }