[PATCH] D155858: Add a concept AST node.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

(There are still outstanding comments e.g. in ASTImport)

I think it would be useful to add to the patch description:

- the current deficiencies of ConceptReference that make it not a well-behaved AST node now
- which of these are addressed in this patch (RecursiveASTVisitor, use in all relevant places, avoiding multiple inheritance)
- which of these will be addressed in future patches (DynTypedNode, maybe text-dumper?)



================
Comment at: clang/include/clang/AST/ASTConcept.h:112
 
 /// \brief Common data class for constructs that reference concepts with
 /// template arguments.
----------------
Doc comment is out of date. Suggestion:

```
/// A reference to a concept and its template args, as it appears in the code.
///
/// Examples:
///   template <int X> requires is_even<X> int half = X/2;
///                             ~~~~~~~~~~ (in ConceptSpecializationExpr)
///
///   std::input_iterator auto I = Container.begin();
///   ~~~~~~~~~~~~~~~~~~~ (in AutoTypeLoc)
///
///   template <std::derives_from<Expr> T> void dump();
///             ~~~~~~~~~~~~~~~~~~~~~~~ (in TemplateTypeParmDecl)
```


================
Comment at: clang/include/clang/AST/ASTConcept.h:140
 public:
   ConceptReference(NestedNameSpecifierLoc NNS, SourceLocation TemplateKWLoc,
                    DeclarationNameInfo ConceptNameInfo, NamedDecl *FoundDecl,
----------------
this constructor should now be private, like other AST nodes


================
Comment at: clang/include/clang/AST/ASTConcept.h:148
 
   ConceptReference()
       : FoundDecl(nullptr), NamedConcept(nullptr), ArgsAsWritten(nullptr) {}
----------------
following the pattern for other AST nodes, this constructor should be private or gone. If deserialization needs to create an empty ConceptReference, provide a CreateEmpty() factory


================
Comment at: clang/include/clang/AST/ASTConcept.h:193
 
-class TypeConstraint : public ConceptReference {
+class TypeConstraint {
   /// \brief The immediately-declared constraint expression introduced by this
----------------
We're not really changing this class, but I think it could use a doc comment as its relationship to ConceptReference, TemplateTypeParmType, and AutoTypeLoc wasn't all that clear. Suggestion:

```
/// Models the abbreviated syntax to constrain a template type parameter:
///   template <convertible_to<string> T> void print(T object);
///             ~~~~~~~~~~~~~~~~~~~~~~
/// Semantically, this adds an "immediately-declared constraint" with extra arg:
///    requires convertible_to<T, string>
///
/// In the C++ grammar, a type-constraint is also used for auto types:
///    convertible_to<string> auto X = ...;
/// We do *not* model these as TypeConstraints, but AutoType(Loc) directly.
```


================
Comment at: clang/include/clang/AST/ExprConcepts.h:90
+  // NOTE(massberg): For the first minimal prototype we keep the
+  // following functions to prevent. Later these functions should
+  // be accessed getConceptReference().
----------------
incomplete comment: "to prevent" what?

Also FWIW I don't think *all* uses should be migrated, e.g. `getNamedConcept()` is fundamental enough to be exposed here even if it's implemented via ConceptReference.


================
Comment at: clang/include/clang/AST/ExprConcepts.h:140-143
   SourceLocation getBeginLoc() const LLVM_READONLY {
-    if (auto QualifierLoc = getNestedNameSpecifierLoc())
+    if (auto QualifierLoc = CR->getNestedNameSpecifierLoc())
       return QualifierLoc.getBeginLoc();
+    return CR->getConceptNameInfo().getBeginLoc();
----------------
cor3ntin wrote:
> Should there be a getLocation() method on `ConceptReference` ? It seems useful. maybe even a `getSourceRange()` method too
agree, though we actually shouldn't call ConceptReference::getLocation() method here because primary location != start location.

I think
 - ConceptReference::getLocation() should return getConceptNameLoc() (just an alias)
 - ConceptSpecializationExpr::getExprLoc() should return ConceptReference::getLocation()
 - ConceptReference::getBeginLoc() should contain this logic, ConceptSpecializationExpr::getBeginLoc() should return it
 - ditto end loc
 - ConceptReference::getSourceRange() is a useful helper
 - ConceptSpecializationExpr already has getSourceRange() inherited from Stmt


================
Comment at: clang/include/clang/AST/RecursiveASTVisitor.h:2517
+    const ConceptReference *CR) {
+  TRY_TO(VisitConceptReference(CR));
+  TRY_TO(TraverseNestedNameSpecifierLoc(CR->getNestedNameSpecifierLoc()));
----------------
I think this should be guarded by `if (!getDerived().shouldTraversePostOrder())`, and then the opposite at the end


================
Comment at: clang/include/clang/AST/RecursiveASTVisitor.h:318
+  /// \returns false if the visitation was terminated early, true otherwise.
+  bool TraverseConceptLoc(const ConceptLoc *CL);
+
----------------
massberg wrote:
> This is basically the old `TraverseConceptReferenceHelper` function. Is it by purpose that this isn't const in most other functions? Could the passes objects be changed in such functions?
It's somewhere between intended and awkward historical baggage: most RAVs don't mutate the nodes, but some do.

I suggest making this take a mutable pointer (rather than the more natural const reference) purely for consistency - RAV is confusing enough as it is.


================
Comment at: clang/include/clang/AST/TypeLoc.h:2111
 
+  ConceptLoc *CL = nullptr;
   // Followed by a TemplateArgumentLocInfo[]
----------------
hokein wrote:
> just an idea, instead storing a `ConceptLoc*` in `AutoTypeLoc`, there is another approach (not sure it was considered before), which is to have a `TypeConstraint*` in the `AutoType` (and remove the `ConceptDecl*`).
> 
> This gives us a model following the grammar closely. type-constrains is used in the auto grammar rule, so it seems reasonable to have `TypeConstraint` in `AutoType`. The downside is that there is a lot of indirections: `AutoTypeLoc ->  AutoType -> TypeConstraint -> ConceptLoc` vs `AutoTypeLoc -> ConceptLoc`.
> 
I did look at this. Maybe it's a good idea, but it's not simple and it's mostly independent of the change in this patch.

The thing that TypeConstraint concretely adds to ConceptReference is the immediately-declared-constraint.
Today this constrains the TypeParmType and semantic analysis can treat it as a separate constraint: `void foo(Iterator auto x)` is equivalent to `template<class T> void foo(T x) requires Iterator<T>` or so.
However this formulation isn't convenient for placeholder types: `Iterator auto x = ...` isn't a template, there's no code processing requirements where we can tack on `requires Iterator<auto>`. It also doesn't clearly represent the idea that the two `auto` placeholders should be the same.

I agree it would be nice to follow the grammar (or if not, use different names) but I think that it's a separate change, and going from `AutoTypeLoc{ConceptNameLoc etc}` to `AutoTypeLoc{ConceptReference{ConceptNameLocEtc}}` is a useful stepping-stone to `AutoTypeLoc{TypeConstraint{ConceptReference{ConceptNameLoc etc}}}`


================
Comment at: clang/include/clang/AST/TypeLoc.h:2140
+  const NestedNameSpecifierLoc getNestedNameSpecifierLoc() const {
+    if (getConceptLoc())
+      return getConceptLoc()->getNestedNameSpecifierLoc();
----------------
hokein wrote:
> nit: `if (const auto* Concept = getConcetLoc())`, the same for the other places.
> 
> any reason why we need this null sanity check? 
Agree with the style change.

We need the check because there may be no constraining concept here: `auto x = 2` should return an empty NNSLoc, not crash


================
Comment at: clang/include/clang/AST/TypeLoc.h:2212-2213
+    // ConceptLoc.
     return TemplateArgumentLoc(getTypePtr()->getTypeConstraintArguments()[i],
-                               getArgLocInfo(i));
+                               getArgInfos()[i]);
   }
----------------
massberg wrote:
> I don't know why we need this here.
> `getArgLoc` should only be called if there are constraints and in that case `getConceptLoc()` should be set.
> However, we still run into this return. Note that `ArgsInfo` is only initialized in `initializeLocal` but never changed afterwards.
I think this FIXME should be addressed before landing this patch.

Currently the code (apparently) stores two copies of the args: one as trailing objects here and one in the conceptreference.

Ideally we'd remove the trailing objects immediately. A FIXME to do so later is fine if needed, but if they're inconsistent then we know one of them has a bug, or we don't really understand the design.

These can lead to downstream bugs now, and also make it less likely such a FIXME will ever be cleaned up.


================
Comment at: clang/lib/Sema/TreeTransform.h:6856
+        TL.getTypePtr()->getTypeConstraintConcept()->getDeclName());
+    /*DNI.setLoc(TL.getConceptNameLoc());
+    DNI.setName(TL.getTypePtr()->getTypeConstraintConcept()->getDeclName());
----------------
fix/remove commented-out code?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155858