[PATCH] D43357: [Concepts] Function trailing requires clauses
Richard Smith - zygoloid via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Aug 6 15:43:39 PDT 2018
rsmith added a comment.
Looking promising, but this patch will need some rework: you need to track the trailing requires clause on the `Declarator` itself, not on the `DeclChunk`, because it's not part of the `DeclChunk` (and may appear in contexts where there is no function chunk).
================
Comment at: include/clang/AST/Decl.h:1793-1796
+ /// \brief The constraint-expression introduced by the trailing
+ /// requires-clause provided in the function declaration, if any.
+ Expr *TrailingRequiresClause;
+
----------------
Please find a way to store this that doesn't make all `FunctionDecl`s 8 bytes larger. At the very least, you can move this to `CXXMethodDecl`, but perhaps it'd be better to include this as an optional component in the `DeclInfo`.
================
Comment at: include/clang/AST/Decl.h:1795
+ /// requires-clause provided in the function declaration, if any.
+ Expr *TrailingRequiresClause;
+
----------------
Not necessarily for this patch, but you'll need to think about how to represent a not-yet-instantiated trailing requires clause. (A requires clause isn't instantiated as part of instantiating the function it's attached to; it's instantiated later, when needed, like an exception specification or default argument.)
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2433
+def err_constrained_virtual_method : Error<
+ "a virtual function must not have a requires clause">;
+def err_reference_to_function_with_unsatisfied_constraints : Error<
----------------
"virtual function cannot have a requires clause" would be more in line with how we usually word diagnostics.
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2435
+def err_reference_to_function_with_unsatisfied_constraints : Error<
+ "invalid reference to function %0 - constraints not satisfied">;
----------------
We generally separate a general problem and a specific problem with a colon, not a hyphen, in diagnostics.
================
Comment at: lib/Parse/ParseDecl.cpp:6098-6165
+ if (getLangOpts().CPlusPlus11 && Tok.is(tok::arrow)) {
+ ParseTrailingReturn();
+ }
+ // Parse trailing requires-clause[opt].
+ if (getLangOpts().ConceptsTS && Tok.is(tok::kw_requires)) {
+ LocalEndLoc = Tok.getLocation();
+ ConsumeToken();
----------------
This is the wrong place to parse a //requires-clause//: a //requires-clause// is a trailing part of the overall //init-declarator// or //member-declarator//, not part of the function declarator chunk. For example:
```
using T = void ();
T x requires true; // ok, but you reject
void (f() requires true); // ill-formed but you accept
void (f()) requires true; // ok but you reject
```
================
Comment at: lib/Parse/ParseDecl.cpp:6102
+ // Parse trailing requires-clause[opt].
+ if (getLangOpts().ConceptsTS && Tok.is(tok::kw_requires)) {
+ LocalEndLoc = Tok.getLocation();
----------------
The `ConceptsTS` check here is redundant. If we see a `kw_requires` token, the feature is enabled.
================
Comment at: lib/Parse/ParseDecl.cpp:6106-6110
+ TentativeParsingAction TPA(*this);
+ DiagnosticErrorTrap Trap(Diags);
+ Diags.setSuppressAllDiagnostics(true);
+ TrailingRequiresClause = ParseConstraintExpression();
+ Diags.setSuppressAllDiagnostics(false);
----------------
This is not a reasonable way to deal with parse errors. Parsing an expression can have non-local effects, and suppressing diagnostics like this is not a safe way to provide error recovery. You're also suppressing all warnings within the expression, which is also inappropriate.
Instead, you should just parse the //requires-clause//. If you parse it correctly (not as a //constraint-expression//) the parse will stop before a possible `->` token anyway, because a top-level `->` expression is not permitted in a //constraint-logical-or-expression//.
================
Comment at: lib/Parse/ParseDecl.cpp:6109
+ Diags.setSuppressAllDiagnostics(true);
+ TrailingRequiresClause = ParseConstraintExpression();
+ Diags.setSuppressAllDiagnostics(false);
----------------
This isn't right: a //constraint-logical-or-expression// is required here, not a //constraint-expression//.
================
Comment at: lib/Parse/ParseDecl.cpp:6112-6113
+
+ if (!Trap.hasErrorOccurred() && TrailingRequiresClause.isUsable()
+ && !TrailingRequiresClause.isInvalid()) {
+ TPA.Commit();
----------------
Clang style puts the `&&` on the previous line.
================
Comment at: lib/Parse/ParseTentative.cpp:955-956
+ // A requires clause indicates a function declaration.
+ if (ParenCount) {
+ SkipUntil(tok::l_paren);
+ } else {
----------------
This is wrong.`ParenCount` isn't just *your* parens, it's all enclosing ones. And I don't think you need this check at all: the `requires` is either part of a suitable declarator or it's ill-formed, so we can just disambiguate as a declarator (that way we'll correctly handle all valid programs and correctly reject all ill-formed ones, with a diagnostic saying that the nested declarator can't have a requires-clause).
================
Comment at: lib/Sema/SemaDecl.cpp:7848-7851
+ Expr *TrailingRequiresClause = D.isFunctionDeclarator()
+ && D.hasTrailingRequiresClause() ?
+ D.getFunctionTypeInfo().getTrailingRequiresClause()
+ : nullptr;
----------------
Please parenthesize the `&&` subexpression and run this through clang-format.
================
Comment at: lib/Sema/SemaDecl.cpp:8377-8381
+ } else if (D.hasTrailingRequiresClause()) {
+ // C++2a [class.virtual]p6
+ // A virtual method shall not have a requires-clause.
+ Diag(NewFD->getTrailingRequiresClause()->getLocStart(),
+ diag::err_constrained_virtual_method);
----------------
This is the wrong place for this check. We don't yet know whether the function is virtual here in general. A function can become virtual due to template instantiation:
```
template<typename T> struct A : T { void f() requires true; };
struct B { virtual void f(); };
template struct A<B>; // error, A<B>::f is constrained and virtual
```
This is perhaps a wording defect: it's not clear that `A::f()` really should override `B::f()`, but that is the consequence of the current rules. I've posted a question to the core reflector.
================
Comment at: lib/Sema/SemaExpr.cpp:2716-2732
+ if (FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) {
+ if (Expr *RC = FD->getTrailingRequiresClause()) {
+ ConstraintSatisfaction Satisfaction;
+ bool Failed = S.CheckConstraintSatisfaction(RC, Satisfaction);
+ if (Failed)
+ // A diagnostic will have already been generated (non-constant
+ // constraint expression, for example)
----------------
Why do we ever fail this check? Overload resolution should never select a function with unsatisfied constraints, and likewise for taking the address of / binding a reference to such a function.
================
Comment at: lib/Sema/SemaOverload.cpp:1208-1221
+ Expr *NewRC = New->getTrailingRequiresClause(),
+ *OldRC = Old->getTrailingRequiresClause();
+ if ((NewRC != nullptr) != (OldRC != nullptr))
+ // RC are most certainly different - these are overloads.
+ return true;
+
+ if (NewRC) {
----------------
This check is too late. We've already passed some `return false;`s at this point. (Consider changing the CUDA checks to only early-return on failure rather than moving this check earlier.)
================
Comment at: lib/Sema/SemaOverload.cpp:6106
+ Expr *RequiresClause = Function->getTrailingRequiresClause();
+ if (LangOpts.ConceptsTS && RequiresClause) {
+ ConstraintSatisfaction Satisfaction;
----------------
The `ConceptsTS` check here doesn't make any sense to me: if you've already parsed a requires clause, you should always check it.
================
Comment at: lib/Sema/SemaOverload.cpp:6619-6629
+
+ Expr *RequiresClause = Method->getTrailingRequiresClause();
+ if (LangOpts.ConceptsTS && RequiresClause) {
+ ConstraintSatisfaction Satisfaction;
+ if (CheckConstraintSatisfaction(RequiresClause, Satisfaction) ||
+ !Satisfaction.IsSatisfied) {
+ Candidate.Viable = false;
----------------
According to [over.match.viable]p3, constraint satisfaction should be checked //before// we attempt to form implicit conversion sequences. This can affect the validity of code, if the associated constraints disable the function in a case where implicit conversion sequence formation would result in the program being ill-formed due to triggering a bad template instantiation (with an error outside an immediate context).
================
Comment at: lib/Sema/SemaOverload.cpp:9123-9135
+ if (Cand1.Function && Cand2.Function) {
+ Expr *RC1 = Cand1.Function->getTrailingRequiresClause();
+ Expr *RC2 = Cand2.Function->getTrailingRequiresClause();
+ if (RC1 && RC2) {
+ bool MoreConstrained1 = S.IsMoreConstrained(Cand1.Function, RC1,
+ Cand2.Function, RC2);
+ bool MoreConstrained2 = S.IsMoreConstrained(Cand2.Function, RC2,
----------------
According to [over.match.best], this check belongs immediately after the "more specialized template" check, not down here.
================
Comment at: lib/Sema/SemaTemplateInstantiateDecl.cpp:1637-1646
+ Expr *TrailingRequiresClause = D->getTrailingRequiresClause();
+ if (TrailingRequiresClause) {
+ ExprResult SubstRC = SemaRef.SubstExpr(TrailingRequiresClause,
+ TemplateArgs);
+ if (!SubstRC.isUsable() || SubstRC.isInvalid())
+ return nullptr;
+ TrailingRequiresClause = SubstRC.get();
----------------
This is wrong. Per [temp.decls]p2, //requires-clause//s are instantiated separately from their associated functions. That doesn't need to be handled in this change, but please at least include a FIXME.
================
Comment at: lib/Sema/SemaTemplateInstantiateDecl.cpp:1641
+ TemplateArgs);
+ if (!SubstRC.isUsable() || SubstRC.isInvalid())
+ return nullptr;
----------------
Checking `!isUsable()` here is wrong. (1: `SubstExpr` should never produce a valid-but-null `Expr*`, 2: even if it did, that means it succeeded, which means it's wrong for you to return `nullptr` without producing a diagnostic.)
================
Comment at: lib/Sema/SemaTemplateInstantiateDecl.cpp:1965
+ TemplateArgs);
+ if (!SubstRC.isUsable() || SubstRC.isInvalid())
+ return nullptr;
----------------
(Likewise.)
================
Comment at: lib/Sema/SemaTemplateVariadic.cpp:884-886
+ if (Chunk.Fun.hasTrailingRequiresClause()
+ &&Chunk.Fun.TrailingRequiresClause->containsUnexpandedParameterPack())
+ return true;
----------------
Please reformat.
Repository:
rC Clang
https://reviews.llvm.org/D43357
More information about the cfe-commits
mailing list