[PATCH] D43357: [Concepts] Function trailing requires clauses
Richard Smith - zygoloid via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Jan 7 15:51:30 PST 2020
rsmith marked an inline comment as done.
rsmith added inline comments.
================
Comment at: clang/include/clang/AST/ASTNodeTraverser.h:391-392
+ if (const Expr *TRC = D->getTrailingRequiresClause())
+ Visit(TRC);
+
----------------
It would be better to do this before we visit the inits, so that we visit the parts of a constructor declaration in lexical order. Eg:
```
struct X {
X() requires true : x(0) {}
int x;
};
```
should dump `true` before `0`.
================
Comment at: clang/include/clang/AST/DeclCXX.h:2368
+ InheritedConstructor Inherited,
+ Expr *TrailingRequiresClause = nullptr);
----------------
It's better to not add the default argument here. We would want any new caller of this (private) constructor to think about what value to pass in here. (Likewise for the other private constructors below.)
================
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:187
+def err_expected_primary_got_typename : Error<
+ "parentheses are required around initialization expression in this context">;
+def note_potential_bin_op_in_constraint_logical_or : Note<
----------------
Perhaps "functional cast expression" (or "explicit type conversion") would be a more familiar term than "initialization expression"?
================
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:326
+def err_requires_clause_must_come_after_trailing_return : Error<
+ "trailing return type must come before trailing requires clause">;
+def err_requires_clause_on_declarator_not_declaring_a_function : Error<
----------------
"appear" is a better word than "come" in this context.
================
Comment at: clang/include/clang/Sema/Sema.h:6181
+ /// function or another callable function-like entity (e.g. a function
+ // emplate or overload set) for any substitution.
+ bool IsDependentFunctionNameExpr(Expr *E);
----------------
emplate -> template
================
Comment at: clang/include/clang/Sema/Sema.h:6189-6191
+ /// \brief Caches pairs of template-like decls whose associated constraints
+ /// were checked for subsumption and whether or not the first's constraints
+ /// did in fact subsume the second's.
----------------
Copy-paste error here; this is the comment for the previous map.
================
Comment at: clang/include/clang/Sema/Sema.h:6196-6198
+ const NormalizedConstraint *
+ getNormalizedAssociatedConstraints(
+ NamedDecl *ConstrainedDecl, ArrayRef<const Expr *> AssociatedConstraints);
----------------
I'm worried about this. `DenseMap` is not a node-based container, so if you return a pointer into the `NormalizationCache` here, it can be invalidated by any subsequent call to the same function. (So for example, you can't reliably get normalized constraints for two declarations at once.) Maybe `NormalizationCache` should store `NormalizedConstraint*`s instead, or maybe you should return `Optional<NormalizedConstraint>`s here.
================
Comment at: clang/include/clang/Sema/Sema.h:6212
+ /// \brief If D1 was not at least as constrained as D2, but would've been if
+ /// a pair of atomic constraints involved had been declared in a concept and
----------------
Don't use `\brief`. (We enable the doxygen "autobrief" setting so it's unnecessary.)
================
Comment at: clang/include/clang/Sema/SemaConcept.h:25
+ const Expr *ConstraintExpr;
+ Optional<SmallVector<TemplateArgumentLoc, 3>> ParameterMapping;
+
----------------
The `SmallVector` here is probably costing us space instead of saving space; we're probably better off directly allocating the correct number of parameter mappings rather than over-allocating in the (probably very common) case of one or two parameter mappings. (Plus `SmallVector` has size overhead to support resizing and capacity>size, which we should never need.)
================
Comment at: clang/include/clang/Sema/SemaConcept.h:86
+ : Constraint{CompoundConstraint{
+ new std::pair<NormalizedConstraint, NormalizedConstraint>{
+ std::move(LHS), std::move(RHS)}, Kind}} { };
----------------
All the allocation here should be allocated on the `ASTContext`. We're going to cache these for the lifetime of the AST anyway; there's no need to do manual heap management here. (And `ASTContext` allocation is cheaper than the regular heap anyway.)
================
Comment at: clang/lib/AST/ASTImporter.cpp:3317
+ D->isInlineSpecified(), D->isImplicit(), D->getConstexprKind(),
+ InheritedConstructor(), TrailingRequiresClause))
return ToFunction;
----------------
Please add a FIXME to properly import the `InheritedConstructor` information.
================
Comment at: clang/lib/AST/Decl.cpp:1839-1848
+ if (getExtInfo()->NumTemplParamLists == 0 &&
+ !getExtInfo()->TrailingRequiresClause) {
// Save type source info pointer.
TypeSourceInfo *savedTInfo = getExtInfo()->TInfo;
// Deallocate the extended decl info.
getASTContext().Deallocate(getExtInfo());
// Restore savedTInfo into (non-extended) decl info.
----------------
There's really no point doing any of this; `Deallocate` is a no-op. You can just delete this block and unconditionally store to `getExtInfo()->QualifierLoc`.
================
Comment at: clang/lib/Parse/ParseDecl.cpp:2045-2046
+ if (Tok.is(tok::kw_requires))
+ ParseTrailingRequiresClause(D);
+
----------------
This should be done earlier, before we parse GNU attributes. For example, GCC accepts:
```
template<typename T> struct X {
void f() requires true __attribute__((noreturn)),
g() requires true __attribute__((noreturn));
};
```
... and rejects if either //requires-clause// is the other side of the attributes. (We already get this right for the second and subsequent declarators.)
================
Comment at: clang/lib/Parse/ParseDecl.cpp:6044-6045
ParseBracketDeclarator(D);
+ } else if (Tok.is(tok::kw_requires)) {
+ if (D.hasGroupingParens())
+ // This declarator is declaring a function, but the requires clause is
----------------
Combine simplify this by grouping these two conditions together as `} else if (Tok.is(tok::kw_requires) && D.hasGroupingParens()) {`.
================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:3799-3810
+ if (D.isFunctionDeclarator()) {
+ auto &FTI = D.getFunctionTypeInfo();
+ if (FTI.Params)
+ for (auto &Param : ArrayRef<DeclaratorChunk::ParamInfo>(FTI.Params,
+ FTI.NumParams)){
+ auto *ParamDecl = cast<NamedDecl>(Param.Param);
+ if (ParamDecl->getIdentifier())
----------------
saar.raz wrote:
> rsmith wrote:
> > This scope-building code should be in `Sema`, not in the parser. (Consider adding an `ActOnStartTrailingRequiresClause` / `ActOnFinishTrailingRequiresClause` pair.)
> Is there anything that needs to be in ActOnFinishTrailingRequiresClause?
I would put the call to `CorrectDelayedTyposInExpr` there, but not a big deal either way.
================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:2319-2320
+ } else if (Tok.is(tok::kw_requires))
+ ParseTrailingRequiresClause(DeclaratorInfo);
+ else {
ParseOptionalCXX11VirtSpecifierSeq(
----------------
Please add the (redundant) braces here to make the local style a little more consistent.
================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:3800
+/// ParseTrailingRequiresClause - Parse a requires-clause as part of a function
+/// declaration.
----------------
Please don't repeat the function name in the documentation comment.
================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:3828
+ if (TrailingRequiresClause.isInvalid())
+ SkipUntil({tok::equal, tok::l_brace, tok::arrow, tok::kw_try, tok::comma,
+ tok::colon},
----------------
Hm, should we stop at an `=`? We can't have an initializer after a //requires-clause//, but we can't have a top-level `=` in a //constraint-expression// either. I'm inclined to think that we should skip past the `=`, but happy either way.
================
Comment at: clang/lib/Parse/ParseExpr.cpp:321
+ Expr *Culprit;
+ if (!Actions.CheckConstraintExpression(LHS.get(), &Culprit)) {
+ if ((Culprit->getType()->isFunctionType() ||
----------------
I'd expect you can recover from errors better by putting this check in `ParseConstraintLogicalAndExpression` after parsing each individual primary expression. (That is: check for this *before* forming an `&&` or `||` binary operator.) That'd make it a lot easier to recover by calling `ParsePostfixExpressionSuffix` or similar.
================
Comment at: clang/lib/Parse/ParseExpr.cpp:330-341
+ Diag(Tok.getLocation(),
+ diag::note_potential_function_call_in_constraint_logical_or);
+ else if (getBinOpPrecedence(Tok.getKind(), GreaterThanIsOperator,
+ getLangOpts().CPlusPlus11))
+ // We have the following case:
+ // template<typename T> requires size_<T> == 0 struct S { };
+ // The user probably isn't aware of the parentheses required around the
----------------
It's very unusual for the parser to attach a note to a diagnostic produced by `Sema`. Also, the parser ideally should not be considering the type of `Culprit` for itself (that's not a parsing concern), and should generally try to treat `Expr`s as being opaque. Please consider passing `Tok.getKind()` into `CheckConstraintExpression` instead and have it produce a suitable note for itself.
================
Comment at: clang/lib/Parse/ParseExpr.cpp:344
+ Actions.CorrectDelayedTyposInExpr(LHS);
+ SkipUntil(tok::r_paren, StopAtSemi);
+ return ExprError();
----------------
I think skipping to the `)` should only be done in the case where we found a `(`. Otherwise, there's no reason to expect there to be a `)` at all, and we'll skip the entire templated declaration (in the non-trailing requires clause case). As noted above, we could recover better by calling `ParsePostfixExpressionSuffix` in the case where we find an element of the //requires-clause// that is clearly not just a //primary-expression//.
================
Comment at: clang/lib/Parse/ParseExpr.cpp:357
+ // a member-declarator or init-declarator.
+ // The user probably tried to call func<T> but didn't realize he had to
+ // parenthesize the function call for it to be included in the constraint
----------------
he -> they
================
Comment at: clang/lib/Parse/ParseExpr.cpp:922
case tok::l_paren: {
// If this expression is limited to being a unary-expression, the parent can
// not start a cast expression.
----------------
parent -> paren
Yes, I know this is pre-existing, but this comment was confusing me when reading your patch :)
================
Comment at: clang/lib/Parse/ParseExpr.cpp:1262-1272
return ParseAvailabilityCheckExpr(Tok.getLocation());
case tok::kw___builtin_va_arg:
case tok::kw___builtin_offsetof:
case tok::kw___builtin_choose_expr:
case tok::kw___builtin_astype: // primary-expression: [OCL] as_type()
case tok::kw___builtin_convertvector:
case tok::kw___builtin_COLUMN:
----------------
These all have function call syntax, so should be treated as //postfix-expression//s not as //primary-expression//s.
================
Comment at: clang/lib/Parse/ParseExpr.cpp:1279
+ if (EnsureNotPrimary())
+ return ExprError();
// C++ [expr.unary] has:
----------------
Do we need to bail out here and below? Continuing and parsing the //postfix-expression// anyway (in the case where it can't possibly be anything else) would result in better error recovery.
================
Comment at: clang/lib/Parse/ParseExpr.cpp:1399
case tok::kw___builtin_bit_cast:
Res = ParseBuiltinBitCast();
break;
----------------
This is also not a //primary-expression//.
================
Comment at: clang/lib/Parse/ParseExpr.cpp:1407
case tok::kw___uuidof:
Res = ParseCXXUuidof();
break;
----------------
This should be treated as a //postfix-expression//, not a //primary-expression//, like `typeid`
================
Comment at: clang/lib/Parse/ParseExpr.cpp:1620-1626
case tok::kw___array_rank:
case tok::kw___array_extent:
return ParseArrayTypeTrait();
case tok::kw___is_lvalue_expr:
case tok::kw___is_rvalue_expr:
return ParseExpressionTrait();
----------------
These are all not //primary-expression//s.
================
Comment at: clang/lib/Parse/ParseExpr.cpp:1630
SourceLocation AtLoc = ConsumeToken();
return ParseObjCAtExpression(AtLoc);
}
----------------
This may or may not be a //primary-expression//. In principle, we need to pass the `ParseKind` into here so that it knows whether to call `ParsePostfixExpressionSuffix`. But in practice, we can disallow all of these: they will never be of type `bool`.
================
Comment at: clang/lib/Parse/ParseExpr.cpp:1651
if (!Res.isInvalid() && !Res.get())
Res = ParseObjCMessageExpression();
break;
----------------
Hmm. Should an Objective-C++20 message send be considered a //primary-expression// or a //postfix-expression//? I'm inclined to say that for now we shouldn't extend the set of things that C++20 allows at the top level in a //requires-clause//: we should allow //literal//s, parenthesized expressions, //id-expression//s, //requires-expression//s, and nothing else.
================
Comment at: clang/lib/Parse/ParseExpr.cpp:354
+ // template<typename> void foo() requires func<T>(
+ // In the first case, '(' cannot start a declaration, and in the second,
+ // '(' cannot follow the requires-clause in a function-definition nor in
----------------
saar.raz wrote:
> rsmith wrote:
> > I don't think this is true. Consider:
> >
> > ```
> > struct A {
> > template<typename T> requires X<T>
> > (A)() {}
> > };
> > ```
> >
> > For a trailing requires clause, we can be much more aggressive with our error detection here, though, and in that case perhaps we can unconditionally treat a trailing `(` as a function call.
> If the cases where this is possible are only very remote - could we maybe turn this into a warning instead and keep it here?
>
> Also, if X<T> is a function type or OverloadType, then this is ill-formed anyway, right? since the constraint expression can't be a bool.
Yes: I think this is correct if either (a) we've checked that the preceding expression is a function or overload set (or of non-dependent non-bool type), or (b) we're parsing a //trailing-requires-clause//.
================
Comment at: clang/lib/Parse/ParseTemplate.cpp:259-262
MaybeParseGNUAttributes(DeclaratorInfo, &LateParsedAttrs);
+ if (Tok.is(tok::kw_requires))
+ ParseTrailingRequiresClause(DeclaratorInfo);
----------------
These are in the wrong order: the //requires-clause// goes before GNU attributes.
================
Comment at: clang/lib/Sema/SemaConcept.cpp:711
+
+bool Sema::MaybeEmitAmbiguousAtomicConstraintsDiagnostic(NamedDecl *D1,
+ ArrayRef<const Expr *> AC1, NamedDecl *D2, ArrayRef<const Expr *> AC2) {
----------------
Should we skip this entirely when we're in a SFINAE context (or more precisely, if our notes will just be discarded and never shown to the user)?
================
Comment at: clang/lib/Sema/SemaConcept.cpp:747-759
+ if (subsumes(*this, D1, AC1, D2, AC2, Is1AtLeastAs2Normally,
+ NormalExprEvaluator))
+ return false;
+ if (subsumes(*this, D2, AC2, D1, AC1, Is2AtLeastAs1Normally,
+ NormalExprEvaluator))
+ return false;
+ bool Is1AtLeastAs2, Is2AtLeastAs1;
----------------
Can you avoid rebuilding the CNF and DNF versions of the normalized constraints here? (Probably not a big deal if this only happens on the path to a hard error.)
================
Comment at: clang/lib/Sema/SemaDecl.cpp:8335-8340
} else if (Name.getNameKind() == DeclarationName::CXXDeductionGuideName) {
SemaRef.CheckDeductionGuideDeclarator(D, R, SC);
return CXXDeductionGuideDecl::Create(SemaRef.Context, DC, D.getBeginLoc(),
ExplicitSpecifier, NameInfo, R, TInfo,
D.getEndLoc());
----------------
We should diagnose trailing requires clauses on deduction guides. (I've started a conversation in the C++ committee regarding whether we should allow them, and so far the suggestion is "yes, but only in C++23".)
================
Comment at: clang/lib/Sema/SemaDecl.cpp:10585-10589
+ std::any_of(Method->overridden_methods().begin(),
+ Method->overridden_methods().end(),
+ [](const CXXMethodDecl *M) {
+ return M->getTrailingRequiresClause() != nullptr;
+ })))
----------------
No need to check this: all overridden methods must be virtual, and virtual functions can't have trailing requires clauses (or if they do, we already diagnosed that when processing them).
================
Comment at: clang/lib/Sema/SemaOverload.cpp:6298
+ Expr *RequiresClause = Function->getTrailingRequiresClause();
+ if (LangOpts.ConceptsTS && RequiresClause) {
+ ConstraintSatisfaction Satisfaction;
----------------
No need to check the `LangOpts` here; if we have parsed a trailing requires clause, we must be in a suitable language mode, and we should check that requires clause.
================
Comment at: clang/lib/Sema/SemaOverload.cpp:6340-6347
if (!AllowExplicit) {
ExplicitSpecifier ES = ExplicitSpecifier::getFromDecl(Function);
if (ES.getKind() != ExplicitSpecKind::ResolvedFalse) {
Candidate.Viable = false;
Candidate.FailureKind = ovl_fail_explicit_resolved;
return;
}
----------------
(Huh, this looks wrong: we should be checking this before we form parameter conversion sequences, shouldn't we?)
================
Comment at: clang/lib/Sema/SemaOverload.cpp:6824-6833
+ Expr *RequiresClause = Method->getTrailingRequiresClause();
+ if (LangOpts.ConceptsTS && RequiresClause) {
+ ConstraintSatisfaction Satisfaction;
+ if (CheckConstraintSatisfaction(RequiresClause, Satisfaction) ||
+ !Satisfaction.IsSatisfied) {
+ Candidate.Viable = false;
+ Candidate.FailureKind = ovl_fail_constraints_not_satisfied;
----------------
Formally, I think we should be doing this before we perform object argument initialization, but ... I think it doesn't actually matter, because object argument initialization can only fail with a hard error by triggering completion of the object argument type, which must have already happened by this point. Subtle, but I think this is OK.
================
Comment at: clang/lib/Sema/SemaOverload.cpp:9555
+ return false;
+ return !AtLeastAsConstrained2;
+ } else if (RC1 || RC2)
----------------
If they're equally-constrained, we should continue and check the later tie-breakers.
================
Comment at: clang/lib/Sema/SemaOverload.cpp:10019-10024
+ for (auto I = CandBegin; I != CandEnd; ++I) {
+ SmallVector<const Expr *, 3> IAC;
+ NamedDecl *ICand = Extractor(*I, IAC);
+ if (!ICand || IAC.empty())
+ continue;
+ for (auto J = I + 1; J != CandEnd; ++J) {
----------------
Checking this for all pairs of functions could be very expensive if there are many overload candidates with constraints. Perhaps for now we should only do this if there are exactly two candidates that are tied for being the best viable candidate?
================
Comment at: clang/lib/Sema/SemaOverload.cpp:10033
+ JAC))
+ // Just show the user one diagnostic, he'll probably figure it out from
+ // here.
----------------
he -> they
================
Comment at: clang/lib/Sema/SemaOverload.cpp:10062
+
+ MaybeDiagnoseAmbiguousConstraints(*this, OvlExpr->decls_begin(),
+ OvlExpr->decls_end(),
----------------
I think generally we should not do this when displaying an arbitrary candidate set, but only when displaying a set of ambiguous candidates (that is, in `OCD_AmbiguousCandidates` mode) -- and ideally only when we know the ambiguity would be resolved if the constraints were ordered.
We get here (for example) when complaining that there were no viable functions, and in that case it makes no sense to talk about ambiguous constraints.
================
Comment at: clang/lib/Sema/SemaOverload.cpp:11325
NoteCandidates(S, Args, Cands, Opc, OpLoc);
}
----------------
This would be a good place to call `MaybeDiagnoseAmbiguousConstraints`, but only in the case where `OCD == OCD_AmbiguousCandidates` (that is, when we're explaining an ambiguity).
================
Comment at: clang/lib/Sema/SemaOverload.cpp:11376-11384
+ MaybeDiagnoseAmbiguousConstraints(S, Cands.begin(), E,
+ [] (const OverloadCandidate *Cand, SmallVectorImpl<const Expr *> &AC)
+ -> NamedDecl * {
+ if (FunctionDecl *FD = Cand->Function) {
+ FD->getAssociatedConstraints(AC);
+ return FD;
+ }
----------------
We should not do this here, because we might be reporting all candidates here, not only the best ones after overload resolution -- this is too low-level to determine whether these notes make sense.
================
Comment at: clang/lib/Sema/SemaOverload.cpp:11490-11500
+ MaybeDiagnoseAmbiguousConstraints(S, Cands.begin(), E,
+ [] (const TemplateSpecCandidate *Cand,
+ SmallVectorImpl<const Expr *> &AC) -> NamedDecl * {
+ if (Cand->Specialization)
+ if (FunctionDecl *FD = dyn_cast<FunctionDecl>(Cand->Specialization))
+ if (TemplateDecl *TD = FD->getPrimaryTemplate()) {
+ TD->getAssociatedConstraints(AC);
----------------
This is reporting no match between a declared template specialization and the known template declarations. Associated constraint mismatches between those templates are irrelevant here.
================
Comment at: clang/lib/Sema/SemaOverload.cpp:12019-12042
+ // We have more than one result - see if it is more constrained than the
+ // previous one.
+ if (Result) {
+ SmallVector<const Expr *, 1> ResultAC, FDAC;
+ Result->getAssociatedConstraints(ResultAC);
+ FD->getAssociatedConstraints(FDAC);
+ bool ResultMoreConstrained, FDMoreConstrained;
----------------
I don't believe this approach is correct. In particular, consider:
Function 1 and 2 are ambiguous. Function 3 is more constrained than function 1. You end up picking function 3 regardless of whether it's more constrained than function 2 (it might not be).
You need to at least check your end result against all the functions you skipped due to ambiguity. (That should be sufficient if we can assume that "more constrained than" is transitive, which it should be.)
================
Comment at: clang/lib/Sema/SemaOverload.cpp:12037
+ continue;
+ // FD is more constrained replace Result with it.
+ }
----------------
Please add some kind of punctuation after "constrained".
================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:3440-3447
+ // C++2a [temp.deduct]p5
+ // [...] When all template arguments have been deduced [...] all uses of
+ // template parameters [...] are replaced with the corresponding deduced
+ // or default argument values.
+ // [...] If the function template has associated constraints
+ // ([temp.constr.decl]), those constraints are checked for satisfaction
+ // ([temp.constr.constr]). If the constraints are not satisfied, type
----------------
Huh. Doing this down here does indeed seem to be correct, but that seems like a rather bad rule...
================
Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:825
Record.readQualifierInfo(*Info);
+ if (Record.readInt()) // has TrailingRequiresClause
+ Info->TrailingRequiresClause = cast<Expr>(Record.readStmt());
----------------
You don't need this flag; `AddStmt` and `readStmt` can cope fine with null pointers.
================
Comment at: clang/test/CXX/temp/temp.constr/temp.constr.constr/function-templates.cpp:2
// RUN: %clang_cc1 -std=c++2a -fconcepts-ts -x c++ -verify %s
-
+#if 0
template<typename T>
----------------
Are the `#if 0`s (here and below) intentional?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D43357/new/
https://reviews.llvm.org/D43357
More information about the cfe-commits
mailing list