Summary: [clang] Provide a way for WhileStmt to report the location of its LParen and RParen.

[oontvoo]

Summary

This helps avoiding hacks downstream.

[gribozavr2]

Could you take a look at test failures and check if they are relevant? linux > Clang.AST::ast-dump-attr.cpp looks extremely close to the area you're working on.

I'm not quite comfortable adding this code without unittests. We don't have a great way to unit test the AST, but I think we should try. I think clang/unittests/AST/MatchVerifier.h could help here. The test could match a while loop with AST matchers, and then assert that the source locations are what we expect. The while loop could come from the code directly, or be instantiated from a template; also test what happens for parse errors.

clang/include/clang/AST/Stmt.h

2292–2293

I'd prefer to unabbreviate here. (use LParenLoc as well)

2301–2302

I'd prefer to unabbreviate here. (use LParenLoc as well)

[riccibruno]

clang/lib/Serialization/ASTReaderStmt.cpp

275

The serialization part is missing (this is the reason for the test failures).

[oontvoo]

Added unit test and the missing serialisation code

[riccibruno]

No objection from me, but I am not a reviewer. I am just accepting this to cancel my comment on the missing serialization.

[oontvoo]

In D83529#2145087, @riccibruno wrote:

No objection from me, but I am not a reviewer. I am just accepting this to cancel my comment on the missing serialization.

No worries. Thanks for your input!

I think Dmitri has already accepted this (pending additional test and variables renaming, which I've done).
I'm re-running the sanitizer tests to make sure it's not related then will commit, unless someone else would like to chime in ...

[oontvoo]

rebase

[MaskRay]

Hi, your git commit contains extra Phabricator tags. You can drop Reviewers: Subscribers: Tags: and the text Summary: from the git commit with the following script:

arcfilter () {
        arc amend
        git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1} /Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:$/ {sub(/^Summary: /,"");print}' | git commit --amend --date=now -F -
}

Reviewed By: is considered important by some people. Please keep the tag. (--date=now is my personal preference (author dates are usually not useful. Using committer dates can make log almost monotonic in time))

llvm/utils/git/pre-push.py can validate the message does not include unneeded tags.

clang/include/clang/AST/Stmt.h

class WhileStmt final : public Stmt,
   //    Always present. This is in fact an "Expr *".
   //
   // * A "Stmt *" for the body.
   //    Always present.
   //
   enum { VarOffset = 0, BodyOffsetFromCond = 1 };
   enum { NumMandatoryStmtPtr = 2 };
 
+  SourceLocation LParenLoc, RParenLoc;
+
   unsigned varOffset() const { return VarOffset; }
   unsigned condOffset() const { return VarOffset + hasVarStorage(); }
   unsigned bodyOffset() const { return condOffset() + BodyOffsetFromCond; }
 
   unsigned numTrailingObjects(OverloadToken<Stmt *>) const {
     return NumMandatoryStmtPtr + hasVarStorage();
   }
 
   /// Build a while statement.
   WhileStmt(const ASTContext &Ctx, VarDecl *Var, Expr *Cond, Stmt *Body,
-            SourceLocation WL);
+            SourceLocation WL, SourceLocation LParenLoc,
+            SourceLocation RParenLoc);
 
   /// Build an empty while statement.
   explicit WhileStmt(EmptyShell Empty, bool HasVar);
 
 public:
   /// Create a while statement.
   static WhileStmt *Create(const ASTContext &Ctx, VarDecl *Var, Expr *Cond,
-                           Stmt *Body, SourceLocation WL);
+                           Stmt *Body, SourceLocation WL,
+                           SourceLocation LParenLoc, SourceLocation RParenLoc);
 
   /// Create an empty while statement optionally with storage for
   /// a condition variable.
   static WhileStmt *CreateEmpty(const ASTContext &Ctx, bool HasVar);
 
   /// True if this WhileStmt has storage for a condition variable.
   bool hasVarStorage() const { return WhileStmtBits.HasVar; }
 
const DeclStmt *getConditionVariableDeclStmt() const {
     return hasVarStorage() ? static_cast<DeclStmt *>(
                                  getTrailingObjects<Stmt *>()[varOffset()])
                            : nullptr;
   }
 
   SourceLocation getWhileLoc() const { return WhileStmtBits.WhileLoc; }
   void setWhileLoc(SourceLocation L) { WhileStmtBits.WhileLoc = L; }
 
+  SourceLocation getLParenLoc() const { return LParenLoc; }
+  void setLParenLoc(SourceLocation L) { LParenLoc = L; }
+  SourceLocation getRParenLoc() const { return RParenLoc; }
+  void setRParenLoc(SourceLocation L) { RParenLoc = L; }
+
   SourceLocation getBeginLoc() const { return getWhileLoc(); }
   SourceLocation getEndLoc() const LLVM_READONLY {
     return getBody()->getEndLoc();
   }
 
   static bool classof(const Stmt *T) {
     return T->getStmtClass() == WhileStmtClass;
   }

clang/include/clang/Parse/Parser.h

private:
   StmtResult ParseCompoundStatement(bool isStmtExpr = false);
   StmtResult ParseCompoundStatement(bool isStmtExpr,
                                     unsigned ScopeFlags);
   void ParseCompoundStatementLeadingPragmas();
   bool ConsumeNullStmt(StmtVector &Stmts);
   StmtResult ParseCompoundStatementBody(bool isStmtExpr = false);
   bool ParseParenExprOrCondition(StmtResult *InitStmt,
                                  Sema::ConditionResult &CondResult,
-                                 SourceLocation Loc,
-                                 Sema::ConditionKind CK);
+                                 SourceLocation Loc, Sema::ConditionKind CK,
+                                 SourceLocation *LParenLoc = nullptr,
+                                 SourceLocation *RParenLoc = nullptr);
   StmtResult ParseIfStatement(SourceLocation *TrailingElseLoc);
   StmtResult ParseSwitchStatement(SourceLocation *TrailingElseLoc);
   StmtResult ParseWhileStatement(SourceLocation *TrailingElseLoc);
   StmtResult ParseDoStatement();
   StmtResult ParseForStatement(SourceLocation *TrailingElseLoc);
   StmtResult ParseGotoStatement();
   StmtResult ParseContinueStatement();
   StmtResult ParseBreakStatement();

clang/include/clang/Sema/Sema.h

StmtResult BuildIfStmt(SourceLocation IfLoc, bool IsConstexpr,
                          Stmt *InitStmt,
                          ConditionResult Cond, Stmt *ThenVal,
                          SourceLocation ElseLoc, Stmt *ElseVal);
   StmtResult ActOnStartOfSwitchStmt(SourceLocation SwitchLoc,
                                     Stmt *InitStmt,
                                     ConditionResult Cond);
   StmtResult ActOnFinishSwitchStmt(SourceLocation SwitchLoc,
                                            Stmt *Switch, Stmt *Body);
-  StmtResult ActOnWhileStmt(SourceLocation WhileLoc, ConditionResult Cond,
+  StmtResult ActOnWhileStmt(SourceLocation WhileLoc, SourceLocation LParenLoc,
+                            ConditionResult Cond, SourceLocation RParenLoc,
                             Stmt *Body);
   StmtResult ActOnDoStmt(SourceLocation DoLoc, Stmt *Body,
                          SourceLocation WhileLoc, SourceLocation CondLParen,
                          Expr *Cond, SourceLocation CondRParen);
 
   StmtResult ActOnForStmt(SourceLocation ForLoc,
                           SourceLocation LParenLoc,
                           Stmt *First,

clang/lib/AST/ASTImporter.cpp

 
 ExpectedStmt ASTNodeImporter::VisitWhileStmt(WhileStmt *S) {
 
   Error Err = Error::success();
   auto ToConditionVariable = importChecked(Err, S->getConditionVariable());
   auto ToCond = importChecked(Err, S->getCond());
   auto ToBody = importChecked(Err, S->getBody());
   auto ToWhileLoc = importChecked(Err, S->getWhileLoc());
+  auto ToLParenLoc = importChecked(Err, S->getLParenLoc());
+  auto ToRParenLoc = importChecked(Err, S->getRParenLoc());
   if (Err)
     return std::move(Err);
 
   return WhileStmt::Create(Importer.getToContext(), ToConditionVariable, ToCond,
-                           ToBody, ToWhileLoc);
+                           ToBody, ToWhileLoc, ToLParenLoc, ToRParenLoc);
 }
 
 ExpectedStmt ASTNodeImporter::VisitDoStmt(DoStmt *S) {
 
   Error Err = Error::success();
   auto ToBody = importChecked(Err, S->getBody());
   auto ToCond = importChecked(Err, S->getCond());
   auto ToDoLoc = importChecked(Err, S->getDoLoc());

clang/lib/AST/Stmt.cpp

void SwitchStmt::setConditionVariable(const ASTContext &Ctx, VarDecl *V) {
   }
 
   SourceRange VarRange = V->getSourceRange();
   getTrailingObjects<Stmt *>()[varOffset()] = new (Ctx)
       DeclStmt(DeclGroupRef(V), VarRange.getBegin(), VarRange.getEnd());
 }
 
 WhileStmt::WhileStmt(const ASTContext &Ctx, VarDecl *Var, Expr *Cond,
-                     Stmt *Body, SourceLocation WL)
+                     Stmt *Body, SourceLocation WL, SourceLocation LParenLoc,
+                     SourceLocation RParenLoc)
     : Stmt(WhileStmtClass) {
   bool HasVar = Var != nullptr;
   WhileStmtBits.HasVar = HasVar;
 
   setCond(Cond);
   setBody(Body);
   if (HasVar)
     setConditionVariable(Ctx, Var);
 
   setWhileLoc(WL);
+  setLParenLoc(LParenLoc);
+  setRParenLoc(RParenLoc);
 }
 
 WhileStmt::WhileStmt(EmptyShell Empty, bool HasVar)
     : Stmt(WhileStmtClass, Empty) {
   WhileStmtBits.HasVar = HasVar;
 }
 
 WhileStmt *WhileStmt::Create(const ASTContext &Ctx, VarDecl *Var, Expr *Cond,
-                             Stmt *Body, SourceLocation WL) {
+                             Stmt *Body, SourceLocation WL,
+                             SourceLocation LParenLoc,
+                             SourceLocation RParenLoc) {
   bool HasVar = Var != nullptr;
   void *Mem =
       Ctx.Allocate(totalSizeToAlloc<Stmt *>(NumMandatoryStmtPtr + HasVar),
                    alignof(WhileStmt));
-  return new (Mem) WhileStmt(Ctx, Var, Cond, Body, WL);
+  return new (Mem) WhileStmt(Ctx, Var, Cond, Body, WL, LParenLoc, RParenLoc);
 }
 
 WhileStmt *WhileStmt::CreateEmpty(const ASTContext &Ctx, bool HasVar) {
   void *Mem =
       Ctx.Allocate(totalSizeToAlloc<Stmt *>(NumMandatoryStmtPtr + HasVar),
                    alignof(WhileStmt));
   return new (Mem) WhileStmt(EmptyShell(), HasVar);
 }

clang/lib/Parse/ParseStmt.cpp

 ///
 /// This function parses and performs error recovery on the specified condition
 /// or expression (depending on whether we're in C++ or C mode).  This function
 /// goes out of its way to recover well.  It returns true if there was a parser
 /// error (the right paren couldn't be found), which indicates that the caller
 /// should try to recover harder.  It returns false if the condition is
 /// successfully parsed.  Note that a successful parse can still have semantic
 /// errors in the condition.
+/// Additionally, if LParenLoc and RParenLoc are non-null, it will assign
+/// the location of the outer-most '(' and ')', respectively, to them.
 bool Parser::ParseParenExprOrCondition(StmtResult *InitStmt,
                                        Sema::ConditionResult &Cond,
                                        SourceLocation Loc,
-                                       Sema::ConditionKind CK) {
+                                       Sema::ConditionKind CK,
+                                       SourceLocation *LParenLoc,
+                                       SourceLocation *RParenLoc) {
   BalancedDelimiterTracker T(*this, tok::l_paren);
   T.consumeOpen();
 
   if (getLangOpts().CPlusPlus)
     Cond = ParseCXXCondition(InitStmt, Loc, CK);
   else {
     ExprResult CondExpr = ParseExpression();
 
if (Cond.isInvalid() && Tok.isNot(tok::r_paren)) {
     // continue parsing the if statement.
     if (Tok.isNot(tok::r_paren))
       return true;
   }
 
   // Otherwise the condition is valid or the rparen is present.
   T.consumeClose();
 
+  if (LParenLoc != nullptr) {
+    *LParenLoc = T.getOpenLocation();
+  }
+  if (RParenLoc != nullptr) {
+    *RParenLoc = T.getCloseLocation();
+  }
+
   // Check for extraneous ')'s to catch things like "if (foo())) {".  We know
   // that all callers are looking for a statement after the condition, so ")"
   // isn't valid.
   while (Tok.is(tok::r_paren)) {
     Diag(Tok, diag::err_extraneous_rparen_in_condition)
       << FixItHint::CreateRemoval(Tok.getLocation());
     ConsumeParen();
   }
if (C99orCXX)
     ScopeFlags = Scope::BreakScope | Scope::ContinueScope |
                  Scope::DeclScope  | Scope::ControlScope;
   else
     ScopeFlags = Scope::BreakScope | Scope::ContinueScope;
   ParseScope WhileScope(this, ScopeFlags);
 
   // Parse the condition.
   Sema::ConditionResult Cond;
+  SourceLocation LParen;
+  SourceLocation RParen;
   if (ParseParenExprOrCondition(nullptr, Cond, WhileLoc,
-                                Sema::ConditionKind::Boolean))
+                                Sema::ConditionKind::Boolean, &LParen, &RParen))
     return StmtError();
 
   // C99 6.8.5p5 - In C99, the body of the while statement is a scope, even if
   // there is no compound stmt.  C90 does not have this clause.  We only do this
   // if the body isn't a compound statement to avoid push/pop in common cases.
   //
   // C++ 6.5p2:
   // The substatement in an iteration-statement implicitly defines a local scope
if (Body.isUsable())
     MIChecker.Check();
   // Pop the body scope if needed.
   InnerScope.Exit();
   WhileScope.Exit();
 
   if (Cond.isInvalid() || Body.isInvalid())
     return StmtError();
 
-  return Actions.ActOnWhileStmt(WhileLoc, Cond, Body.get());
+  return Actions.ActOnWhileStmt(WhileLoc, LParen, Cond, RParen, Body.get());
 }
 
 /// ParseDoStatement
 ///       do-statement: [C99 6.8.5.2]
 ///         'do' statement 'while' '(' expression ')' ';'
 /// Note: this lets the caller parse the end ';'.
 StmtResult Parser::ParseDoStatement() {
   assert(Tok.is(tok::kw_do) && "Not a do stmt!");

clang/lib/Sema/SemaStmt.cpp

if (!Context.hasSameUnqualifiedType(SrcType, DstType) &&
             Diag(SrcExpr->getExprLoc(), diag::warn_not_in_enum_assignment)
                 << DstType.getUnqualifiedType();
           }
         }
       }
     }
 }
 
-StmtResult Sema::ActOnWhileStmt(SourceLocation WhileLoc, ConditionResult Cond,
-                                Stmt *Body) {
+StmtResult Sema::ActOnWhileStmt(SourceLocation WhileLoc,
+                                SourceLocation LParenLoc, ConditionResult Cond,
+                                SourceLocation RParenLoc, Stmt *Body) {
   if (Cond.isInvalid())
     return StmtError();
 
   auto CondVal = Cond.get();
   CheckBreakContinueBinding(CondVal.second);
 
   if (CondVal.second &&
       !Diags.isIgnored(diag::warn_comma_operator, CondVal.second->getExprLoc()))
     CommaVisitor(*this).Visit(CondVal.second);
 
   if (isa<NullStmt>(Body))
     getCurCompoundScope().setHasEmptyLoopBodies();
 
   return WhileStmt::Create(Context, CondVal.first, CondVal.second, Body,
-                           WhileLoc);
+                           WhileLoc, LParenLoc, RParenLoc);
 }
 
 StmtResult
 Sema::ActOnDoStmt(SourceLocation DoLoc, Stmt *Body,
                   SourceLocation WhileLoc, SourceLocation CondLParen,
                   Expr *Cond, SourceLocation CondRParen) {
   assert(Cond && "ActOnDoStmt(): missing expression");
 

clang/lib/Sema/TreeTransform.h

StmtResult RebuildSwitchStmtBody(SourceLocation SwitchLoc,
                                    Stmt *Switch, Stmt *Body) {
     return getSema().ActOnFinishSwitchStmt(SwitchLoc, Switch, Body);
   }
 
   /// Build a new while statement.
   ///
   /// By default, performs semantic analysis to build the new statement.
   /// Subclasses may override this routine to provide different behavior.
-  StmtResult RebuildWhileStmt(SourceLocation WhileLoc,
-                              Sema::ConditionResult Cond, Stmt *Body) {
-    return getSema().ActOnWhileStmt(WhileLoc, Cond, Body);
+  StmtResult RebuildWhileStmt(SourceLocation WhileLoc, SourceLocation LParenLoc,
+                              Sema::ConditionResult Cond,
+                              SourceLocation RParenLoc, Stmt *Body) {
+    return getSema().ActOnWhileStmt(WhileLoc, LParenLoc, Cond, RParenLoc, Body);
   }
 
   /// Build a new do-while statement.
   ///
   /// By default, performs semantic analysis to build the new statement.
   /// Subclasses may override this routine to provide different behavior.
   StmtResult RebuildDoStmt(SourceLocation DoLoc, Stmt *Body,
                            SourceLocation WhileLoc, SourceLocation LParenLoc,
TreeTransform<Derived>::TransformWhileStmt(WhileStmt *S) {
   if (Body.isInvalid())
     return StmtError();
 
   if (!getDerived().AlwaysRebuild() &&
       Cond.get() == std::make_pair(S->getConditionVariable(), S->getCond()) &&
       Body.get() == S->getBody())
     return Owned(S);
 
-  return getDerived().RebuildWhileStmt(S->getWhileLoc(), Cond, Body.get());
+  return getDerived().RebuildWhileStmt(S->getWhileLoc(), S->getLParenLoc(),
+                                       Cond, S->getRParenLoc(), Body.get());
 }
 
 template<typename Derived>
 StmtResult
 TreeTransform<Derived>::TransformDoStmt(DoStmt *S) {
   // Transform the body
   StmtResult Body = getDerived().TransformStmt(S->getBody());
   if (Body.isInvalid())

clang/lib/Serialization/ASTReaderStmt.cpp

void ASTStmtReader::VisitWhileStmt(WhileStmt *S) {
   bool HasVar = Record.readInt();
 
   S->setCond(Record.readSubExpr());
   S->setBody(Record.readSubStmt());
   if (HasVar)
     S->setConditionVariable(Record.getContext(), readDeclAs<VarDecl>());
 
   S->setWhileLoc(readSourceLocation());
+  S->setLParenLoc(readSourceLocation());
+  S->setRParenLoc(readSourceLocation());
 }
 
 void ASTStmtReader::VisitDoStmt(DoStmt *S) {
   VisitStmt(S);
   S->setCond(Record.readSubExpr());
   S->setBody(Record.readSubStmt());
   S->setDoLoc(readSourceLocation());
   S->setWhileLoc(readSourceLocation());

clang/lib/Serialization/ASTWriterStmt.cpp

void ASTStmtWriter::VisitWhileStmt(WhileStmt *S) {
   Record.push_back(HasVar);
 
   Record.AddStmt(S->getCond());
   Record.AddStmt(S->getBody());
   if (HasVar)
     Record.AddDeclRef(S->getConditionVariable());
 
   Record.AddSourceLocation(S->getWhileLoc());
+  Record.AddSourceLocation(S->getLParenLoc());
+  Record.AddSourceLocation(S->getRParenLoc());
   Code = serialization::STMT_WHILE;
 }
 
 void ASTStmtWriter::VisitDoStmt(DoStmt *S) {
   VisitStmt(S);
   Record.AddStmt(S->getCond());
   Record.AddStmt(S->getBody());
   Record.AddSourceLocation(S->getDoLoc());

clang/unittests/AST/SourceLocationTest.cpp

 }
 
 TEST(RangeVerifier, WrongRange) {
   RangeVerifier<VarDecl> Verifier;
   Verifier.expectRange(1, 1, 1, 1);
   EXPECT_FALSE(Verifier.match("int i;", varDecl()));
 }
 
+class WhileParenLocationVerifier : public MatchVerifier<WhileStmt> {
+  unsigned ExpectLParenLine = 0, ExpectLParenColumn = 0;
+  unsigned ExpectRParenLine = 0, ExpectRParenColumn = 0;
+
+public:
+  void expectLocations(unsigned LParenLine, unsigned LParenColumn,
+                       unsigned RParenLine, unsigned RParenColumn) {
+    ExpectLParenLine = LParenLine;
+    ExpectLParenColumn = LParenColumn;
+    ExpectRParenLine = RParenLine;
+    ExpectRParenColumn = RParenColumn;
+  }
+
+protected:
+  void verify(const MatchFinder::MatchResult &Result,
+              const WhileStmt &Node) override {
+    SourceLocation LParenLoc = Node.getLParenLoc();
+    SourceLocation RParenLoc = Node.getRParenLoc();
+    unsigned LParenLine =
+        Result.SourceManager->getSpellingLineNumber(LParenLoc);
+    unsigned LParenColumn =
+        Result.SourceManager->getSpellingColumnNumber(LParenLoc);
+    unsigned RParenLine =
+        Result.SourceManager->getSpellingLineNumber(RParenLoc);
+    unsigned RParenColumn =
+        Result.SourceManager->getSpellingColumnNumber(RParenLoc);
+
+    if (LParenLine != ExpectLParenLine || LParenColumn != ExpectLParenColumn ||
+        RParenLine != ExpectRParenLine || RParenColumn != ExpectRParenColumn) {
+      std::string MsgStr;
+      llvm::raw_string_ostream Msg(MsgStr);
+      Msg << "Expected LParen Location <" << ExpectLParenLine << ":"
+          << ExpectLParenColumn << ">, found <";
+      LParenLoc.print(Msg, *Result.SourceManager);
+      Msg << ">\n";
+
+      Msg << "Expected RParen Location <" << ExpectRParenLine << ":"
+          << ExpectRParenColumn << ">, found <";
+      RParenLoc.print(Msg, *Result.SourceManager);
+      Msg << ">";
+
+      this->setFailure(Msg.str());
+    }
+  }
+};
+
+TEST(LocationVerifier, WhileParenLoc) {
+  WhileParenLocationVerifier Verifier;
+  Verifier.expectLocations(1, 17, 1, 38);
+  EXPECT_TRUE(Verifier.match("void f() { while(true/*some comment*/) {} }",
+                             whileStmt()));
+}
+
 class LabelDeclRangeVerifier : public RangeVerifier<LabelStmt> {
 protected:
   SourceRange getRange(const LabelStmt &Node) override {
     return Node.getDecl()->getSourceRange();
   }
 };
 
 TEST(LabelDecl, Range) {