[PATCH] D84136: [clang] Fix visitation of ConceptSpecializationExpr in constrained-parameter

[Nathan Ridge via Phabricator via cfe-commits]

nridge marked 2 inline comments as done.
nridge added inline comments.


================
Comment at: clang/include/clang/AST/RecursiveASTVisitor.h:1843
+  if (const auto *TC = D->getTypeConstraint()) {
+    TRY_TO(TraverseStmt(TC->getImmediatelyDeclaredConstraint()));
     TRY_TO(TraverseConceptReference(*TC));
----------------
hokein wrote:
> nridge wrote:
> > hokein wrote:
> > > nridge wrote:
> > > > hokein wrote:
> > > > > Looks like we may visit some nodes in `ConceptReference` twice:
> > > > > -  getImmediatelyDeclaredConstraint returns a `ConceptSpecializationExpr` (most cases?) which is a subclass of `ConceptReference`;
> > > > > - `TraverseStmt(ConceptSpecializationExpr*)` will dispatch to `TraverseConceptSpecializationExpr` which invokes `TraverseConceptReference` (see Line 2719);
> > > > > 
> > > > > 
> > > > > It is sad that we don't have enough test coverage, could you write some tests in `clang/unittests/Tooling/RecursiveASTVisitorTests/`?
> > > > It is true that there will be two calls to `TraverseConceptReference()`. However, they are called on two different `ConceptReference` objects:
> > > > 
> > > >   * the call in `TraverseConceptSpecializationExpr` will visit the base subobject of the `ConceptSpecializationExpr` (which inherits from `ConceptReference`)
> > > >   * the call in `TraverseTemplateTypeParmDecl` will visit the base subobject of the `TypeConstraint` (which also inherits from `ConceptReference`).
> > > > 
> > > > So, I think this is fine -- there are two distinct `ConceptReference` objects in the AST, and with this patch we visit both of them.
> > > I understand that they are two different `ConceptReference` objects, but they have members (`FoundDecl`, `ArgsAsWritten`) that may refer to the same AST nodes.
> > > 
> > > ```
> > > template <typename T, typename U>
> > > concept binary_concept = true;
> > > struct Foo {};
> > > 
> > > template<binary_concept<Foo> T> // the template argument Foo will be visited twice.
> > > void k2();
> > > ```
> > > 
> > > I'm not sure what's is the right approach here, I can see two options:
> > > 
> > > - traverse TC + immediately-declared-constraint expr, this seem to cause some ast nodes visited twice (maybe not a big deal?)
> > > - just traverse immediately-declared-constraint expr, this seems not breaking any tests, but the immediately-declared-constraint expr could be nullptr (e.g. broken code, missing required template arguments); or the immediately-declared-constraint expr could be a `CXXFoldExpr`, which will make some members in `ConceptReference` not be visited;
> > > 
> > > @rsmith, do you have any idea about this?
> > > 
> > From clangd's point of view, it would be sufficient to visit the immediately-declared-constraint-expr without visiting any of its descendants. However, I'm not sure how to accomplish this using `RecursiveASTVisitor`. (I think I'd want to call `WalkUpFromXXX(TC->getImmediatelyDeclaredConstraint())`, where `XXX` is the dynamic type of the immediately-delcared-constraint, but I don't know how to dispatch to that dynamic type; `RecursiveASTVisitor` seems to be designed to do the dispatch on `Traverse` calls, not `WalkUpFrom` calls).
> Thinking more about this.
> 
> I'm aligned on the idea of only traversing the immediately declared constraint since the TypeContraint is a wrapper of it. The regression is that we'd not perform traversal if the immediately declared constraint is nullptr (this just happens for broken code). 
> 
> Fix ideas:
> - build a recovery expression if we can't not build a immediately declared constraint due to the missing required template arguments etc;
> - still traverse the TC if immediately declared constraint is nullptr;
> 
> 
Thanks, this suggestion makes sense to me. I went with the second approach.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84136