Function trailing requires clauses now parsed, supported in overload resolution and when calling, referencing and taking the address of functions or function templates.Does not directly affect code generation yet.
Depends on D41910.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
- Build Status
Buildable 42864 Build 43454: arc lint + arc unit
Event Timeline
- Fixed handling of empty/non-expression after trailing requires keyword.
- Unified satisfaction check for templated/non-templated constraint exprs
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).
include/clang/AST/Decl.h | ||
---|---|---|
1793–1796 ↗ | (On Diff #159349) | Please find a way to store this that doesn't make all FunctionDecls 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. |
1795 ↗ | (On Diff #159349) | 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.) |
include/clang/Basic/DiagnosticSemaKinds.td | ||
2433 ↗ | (On Diff #159349) | "virtual function cannot have a requires clause" would be more in line with how we usually word diagnostics. |
2435 ↗ | (On Diff #159349) | We generally separate a general problem and a specific problem with a colon, not a hyphen, in diagnostics. |
lib/Parse/ParseDecl.cpp | ||
6098–6165 ↗ | (On Diff #159349) | 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 |
6102 ↗ | (On Diff #159349) | The ConceptsTS check here is redundant. If we see a kw_requires token, the feature is enabled. |
6106–6110 ↗ | (On Diff #159349) | 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. |
6109 ↗ | (On Diff #159349) | This isn't right: a constraint-logical-or-expression is required here, not a constraint-expression. |
6112–6113 ↗ | (On Diff #159349) | Clang style puts the && on the previous line. |
lib/Parse/ParseTentative.cpp | ||
955–956 ↗ | (On Diff #159349) | 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). |
lib/Sema/SemaDecl.cpp | ||
7848–7851 ↗ | (On Diff #159349) | Please parenthesize the && subexpression and run this through clang-format. |
8377–8381 ↗ | (On Diff #159349) | 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. |
lib/Sema/SemaExpr.cpp | ||
2716–2732 ↗ | (On Diff #159349) | 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. |
lib/Sema/SemaOverload.cpp | ||
1208–1221 ↗ | (On Diff #159349) | 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.) |
6106 ↗ | (On Diff #159349) | The ConceptsTS check here doesn't make any sense to me: if you've already parsed a requires clause, you should always check it. |
6619–6629 ↗ | (On Diff #159349) | 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). |
9123–9135 ↗ | (On Diff #159349) | According to [over.match.best], this check belongs immediately after the "more specialized template" check, not down here. |
lib/Sema/SemaTemplateInstantiateDecl.cpp | ||
1637–1646 ↗ | (On Diff #159349) | This is wrong. Per [temp.decls]p2, requires-clauses are instantiated separately from their associated functions. That doesn't need to be handled in this change, but please at least include a FIXME. |
1641 ↗ | (On Diff #159349) | 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.) |
1965 ↗ | (On Diff #159349) | (Likewise.) |
lib/Sema/SemaTemplateVariadic.cpp | ||
884–886 ↗ | (On Diff #159349) | Please reformat. |
lib/Sema/SemaDecl.cpp | ||
---|---|---|
8377–8381 ↗ | (On Diff #159349) | I don't really see why A::f() should override B::f() indeed - since it is not marked virtual nor override, shouldn't it just hide B::f()? or am I missing something here? |
lib/Sema/SemaDecl.cpp | ||
---|---|---|
8377–8381 ↗ | (On Diff #159349) | Functions override base class virtual functions if they have matching name, parameter types, cv-qualifiers and ref-qualifiers, *regardless* of whether they're declared virtual etc. Eg: struct A { virtual void f(); }; struct B : A { void f(); }; B::f overrides A::f, and as a consequence, B::f is implicitly a virtual function. See [class.virtual]p2 and its footnote. Note that the determination of whether the derived class function overrides the base class function doesn't care about the associated constraints on the derived class function. (After checking with the core reflector, general consensus seems to be that this is the right rule, and my previous example should indeed be ill-formed because it declares a constrained virtual function.) |
- Add diagnostic explaining the unintuitive subsumption rules when user might be relying on expression equivalence for odering
- Fixed incorrect checking of trailing requires clause subsumption
- Fixed missing conversion constraint checking
- Fixed constraint checking in function templates to recognize function parameters
- Address CR comments by rsmith
- Move TrailingRequiresClause to ExtInfo
- Fixed diagnostic styling
- Correct checking of constrained virtual functions
- Small formatting fixes
- Parse constraint-logical-or-expressions in requires clauses
- Parse the requires clause as part of the init-declarator, member-declarator or function-definition
- No dirty diagnostic suppression when trying to parse switched-order trailing requires and return type
lib/AST/ASTImporter.cpp | ||
---|---|---|
3053 ↗ | (On Diff #195033) | First you have to import the trailing requires clause and then pass the imported Expr here. |
- Address CR comment
- Adjust to new getAssociatedConstraints interface
- Add destructor and conversion operator tests
clang/include/clang/Basic/DiagnosticParseKinds.td | ||
---|---|---|
184–185 | I think the "did you forget parentheses?" part here may be irritating to users who didn't know they needed parentheses at all -- it may be read as patronizing by some. Can we rephrase as "parentheses are required around a top-level unary %0 expression in a requires clause" or something like that? | |
clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
2599 | Don't use a colon to indicate a location. Diagnostics are formatted in many different ways and this won't always make sense. Also, where possible, don't pretty-print expressions in diagnostics; underline the source range instead. | |
2600 | Similarly, this note presentation won't always make sense. Can we somehow rephrase these notes so they're more presenting the facts and less telling the user what to do? | |
4039–4048 | Don't repeat this %select; use %sub{select_ovl_candidate_kind} instead. (The enum and the appropriate %select have already changed at least twice since this was copy-pasted...) | |
clang/include/clang/Parse/Parser.h | ||
1676–1682 | Don't repeat the name of the entity in the doc comment. (This is an old style that we're phasing out; new code shouldn't do it.) | |
1677 | We would usually call this sort of enumeration SomethingKind not SomethingOption. | |
1682 | Please don't use Type as the name of something that's not a language-level type. | |
2725 | Line-wrapped parameters should start on the same column as the first parameter. | |
clang/lib/Parse/ParseDecl.cpp | ||
2093–2094 | We should check with GCC to see which side of the requires-clause they expect this. For the second and subsequent declarations, we expect the asm label before the requires clause; here we parse the asm label after the requires clause. | |
2290–2291 | We already parsed this for the first declarator in the group. Do we accidentally permit two requires clauses on the first declarator? | |
clang/lib/Parse/ParseDeclCXX.cpp | ||
3812–3823 | This scope-building code should be in Sema, not in the parser. (Consider adding an ActOnStartTrailingRequiresClause / ActOnFinishTrailingRequiresClause pair.) | |
3814 | Add braces for this multi-line if. | |
3838 | Also tok::colon for constructors. | |
clang/lib/Parse/ParseExpr.cpp | ||
349–350 | The parser shouldn't be encoding this kind of semantic knowledge. Please move this to Sema. (This is also not really a very general way to detect a function-like name: checking for an expression whose type is a function type or OverloadType would catch more cases.) | |
354 | 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. | |
901 | Do not hardcode English strings into diagnostic arguments. Use a %select or a different diagnostic instead. |
Address CR comments by rsmith.
Also, add Sema normalization API (useful on its own right) and a normalization cache, for two reasons:
- Performance (not having to re-normalize the constraints for every template or concept on every use).
- Preventing normalization diagnostics from popping up multiple times (when the causing constraint is normalized again).
clang/lib/Parse/ParseDecl.cpp | ||
---|---|---|
2093–2094 | They expect the requires clause first. | |
clang/lib/Parse/ParseDeclCXX.cpp | ||
3812–3823 | Is there anything that needs to be in ActOnFinishTrailingRequiresClause? | |
clang/lib/Parse/ParseExpr.cpp | ||
349–350 | Function types and overload types would not reach here (would be eliminated in the previous check for 'bool' types). Only dependent types and bools get here, and checking for a function type or OverloadType would not catch those cases. Anyway, moved this to Sema | |
354 | 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. |
Fix memory handling of NormalizedConstraint and AtomicConstraint
Fix some more failed tests and rebase onto trunk
clang/include/clang/AST/ASTNodeTraverser.h | ||
---|---|---|
385–386 | 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. | |
clang/include/clang/AST/DeclCXX.h | ||
2368 | 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.) | |
clang/include/clang/Basic/DiagnosticParseKinds.td | ||
187 | Perhaps "functional cast expression" (or "explicit type conversion") would be a more familiar term than "initialization expression"? | |
326 | "appear" is a better word than "come" in this context. | |
clang/include/clang/Sema/Sema.h | ||
6180 | emplate -> template | |
6188–6190 | Copy-paste error here; this is the comment for the previous map. | |
6195–6197 | 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. | |
6211 | Don't use \brief. (We enable the doxygen "autobrief" setting so it's unnecessary.) | |
clang/include/clang/Sema/SemaConcept.h | ||
26 | 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.) | |
87 | 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.) | |
clang/lib/AST/ASTImporter.cpp | ||
3290 | Please add a FIXME to properly import the InheritedConstructor information. | |
clang/lib/AST/Decl.cpp | ||
1839–1848 | 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. | |
clang/lib/Parse/ParseDecl.cpp | ||
2045–2046 | 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.) | |
6044–6045 | Combine simplify this by grouping these two conditions together as } else if (Tok.is(tok::kw_requires) && D.hasGroupingParens()) {. | |
clang/lib/Parse/ParseDeclCXX.cpp | ||
2319–2320 | Please add the (redundant) braces here to make the local style a little more consistent. | |
3800 | Please don't repeat the function name in the documentation comment. | |
3812–3823 | I would put the call to CorrectDelayedTyposInExpr there, but not a big deal either way. | |
3828 | 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. | |
clang/lib/Parse/ParseExpr.cpp | ||
321 | 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. | |
330–341 | 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 Exprs as being opaque. Please consider passing Tok.getKind() into CheckConstraintExpression instead and have it produce a suitable note for itself. | |
344 | 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. | |
354 | 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. | |
357 | he -> they | |
922 | parent -> paren Yes, I know this is pre-existing, but this comment was confusing me when reading your patch :) | |
1262–1272 | These all have function call syntax, so should be treated as postfix-expressions not as primary-expressions. | |
1279 | 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. | |
1399 | This is also not a primary-expression. | |
1407 | This should be treated as a postfix-expression, not a primary-expression, like typeid | |
1620–1626 | These are all not primary-expressions. | |
1630 | 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. | |
1651 | 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 literals, parenthesized expressions, id-expressions, requires-expressions, and nothing else. | |
clang/lib/Parse/ParseTemplate.cpp | ||
259–265 | These are in the wrong order: the requires-clause goes before GNU attributes. | |
clang/lib/Sema/SemaConcept.cpp | ||
708 | 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)? | |
744–756 | 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.) | |
clang/lib/Sema/SemaDecl.cpp | ||
8332–8337 | 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".) | |
10582–10586 | 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). | |
clang/lib/Sema/SemaOverload.cpp | ||
6302 | 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. | |
6344–6351 | (Huh, this looks wrong: we should be checking this before we form parameter conversion sequences, shouldn't we?) | |
6828–6837 | 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. | |
9559 | If they're equally-constrained, we should continue and check the later tie-breakers. | |
10023–10028 | 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? | |
10037 | he -> they | |
10066 | 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. | |
11327 | 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). | |
11378–11386 | 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. | |
11492–11502 | This is reporting no match between a declared template specialization and the known template declarations. Associated constraint mismatches between those templates are irrelevant here. | |
12021–12044 | 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.) | |
12039 | Please add some kind of punctuation after "constrained". | |
clang/lib/Sema/SemaTemplateDeduction.cpp | ||
3440–3447 | Huh. Doing this down here does indeed seem to be correct, but that seems like a rather bad rule... | |
clang/lib/Serialization/ASTReaderDecl.cpp | ||
825 | You don't need this flag; AddStmt and readStmt can cope fine with null pointers. | |
clang/test/CXX/temp/temp.constr/temp.constr.constr/function-templates.cpp | ||
2 | Are the #if 0s (here and below) intentional? |
clang/lib/Sema/SemaTemplateDeduction.cpp | ||
---|---|---|
3440–3447 | This is going to get moved back up to where we had it before by CWG issue 2369 =) |
clang/lib/Sema/SemaOverload.cpp | ||
---|---|---|
6344–6351 | Should I fix this in this patch? |
Thanks, I'm happy to iterate on this further after you commit (though feel free to ask for another round of review if you prefer). The changes to recovery from missing parens in a trailing requires clause can be done in a separate change after this one if you like.
clang/include/clang/Basic/DiagnosticParseKinds.td | ||
---|---|---|
186–188 | This diagnostic is now unused; please remove it. | |
clang/include/clang/Sema/SemaConcept.h | ||
26 | This will leak, since we're never going to actually run the destructor for AtomicConstraint. (That's not a problem for compilation, but can be a problem for use of Clang as a library.) Perhaps we could store this as an ArrayRef to an array allocated on the ASTContext allocator? Alternatively, can we compute the number of parameter mappings when we create the AtomicConstraint object, and tail-allocate the parameter mappings? | |
clang/lib/Parse/ParseDecl.cpp | ||
6067–6069 | I would just drop this comment; I don't think it's all that useful, since it's just really describing how the grammar works. | |
clang/lib/Parse/ParseExpr.cpp | ||
280 | Please return false here (unless E.isInvalid()): we should recovering as described by the fixit hints, so we shouldn't bail out of outer parsing steps. (I suppose you should also set NotPrimaryExpression back to false for subsequent parses, to handle things like template<typename T> requires a == b && c (X(T));, for which we want to diagnose the missing parens around a == b but not assume that the c (X(T)) is a function call.) | |
283 | I think we can catch and diagnose even more cases here. If the next token is a binary operator with higher precedence than && or a trailing postfix operator other than ( (that is, ., ->, ++, --, or a [ not followed by [), that's always an error and we can parse the remaining tokens as a non-primary expression to recover. (For a trailing requires clause, the only thing that can come next is a function body, and that never starts with any of those tokens -- it can start with =, but that has lower precedence than &&. For a non-trailing requires clause, none of those tokens can appear next -- ( can, and CheckConstraintExpression checks for that case.) | |
295–299 | It'd be nice to move the call to ParseCastExpression into CheckExpr rather than duplicating it. | |
363–381 | Do we still need this here? CheckConstraintExpression already sets PossibleNonPrimary to true in cases very much like this one -- could we make it handle these remaining cases too? | |
1622 | These are not primary-expressions. | |
clang/lib/Sema/SemaConcept.cpp | ||
47–52 | I think it'd be better to not produce a diagnostic here in the case where you set PossibleNonPrimary to true, and leave it to the caller to do that instead. That way we can properly recover in the parser and parse the rest of the expression (and produce a diagnostic showing where the parens should be inserted). | |
clang/lib/Sema/SemaDeclCXX.cpp | ||
3887 | We need to return the result of this back to the caller; we want to use the typo-corrected constraint expression in any further processing. | |
clang/lib/Sema/SemaOverload.cpp | ||
6301–6302 | Nit: combine these into one line (if (Expr *RequiresClause = ...) to minimize the scope of RequiresClause. | |
6828–6829 | Nit: as above, combine these to a single line. | |
10054 | You can break after this }. Consider flattening / simplifying the two nested loops here into a single loop (walk the candidates and collect those ones that have associated constraints, and make sure you find exactly two). | |
clang/test/CXX/temp/temp.constr/temp.constr.constr/non-function-templates.cpp | ||
2–3 | Are the #if 0s here intentional? | |
clang/test/CXX/temp/temp.constr/temp.constr.order/class-template-partial-specializations.cpp | ||
60–61 | It's useful to include a positive test case (requires C2<T> && C2<U> maybe?) in addition to the negative test case. |
This breaks tests on Windows: http://45.33.8.238/win/5368/step_7.txt
Please take a look and revert if it takes a while to investigate.
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:
should dump true before 0.