Implement the requires expression. Depends on D44352.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
- Rebase onto trunk
- Fixed ignoring of ScopeSpecifier in constrained parameter in compound requirements
- Fixed incorrect substitution and type deduction into constrained parameter return type requirement (depth was not being considered)
- Fix incorrect check for unevaluated context for requires expr local parameters.
- Added short-circuiting to the substitution-satisfaction check
- Disallow plain 'auto' in return type requirement
- Fixed bug where diagnosed but successful substitution would count as valid
- Add non-SFINAE CodeSynthesisContext to nested requirement constraints check
- Remove a shortcut 'if' in compound requirements which was non-conforming
- Split requires expr parameter and body scopes
- Add missing CorrectDelayedTyposInExpr
- Add RequiresBodyDecl to switch where it was missing
- Predefine __cpp_concepts when concepts are activated
- Deal with cases where substitution failed but no diagnostic is available
include/clang/AST/ExprCXX.h | ||
---|---|---|
4671 ↗ | (On Diff #195980) | You can stash these in the bit-field classes of Stmt to save some space. |
4674 ↗ | (On Diff #195980) | Can you tail-allocate them ? |
4703 ↗ | (On Diff #195980) | If in general you don't strictly need to modify some internal state of an AST node, I think it would be better to not provide the corresponding method. |
4715 ↗ | (On Diff #195980) | LLVM_READONLY is pointless here. |
4722 ↗ | (On Diff #195980) | You should also provide the const-qualified version. |
Remove outdated grammar support in return-type-requirements, removing some hacks and simplifying the code.
include/clang/AST/DeclCXX.h | ||
---|---|---|
2051 ↗ | (On Diff #203713) | The semicolon here is in the wrong place (should be before the final }, not after). |
include/clang/AST/DeclTemplate.h | ||
1185 ↗ | (On Diff #203713) | Please move this bugfix earlier in the patch series. |
include/clang/AST/ExprCXX.h | ||
4634 ↗ | (On Diff #203713) | Reorder this next to RequiresKWLoc. SourceLocations are 4 bytes, whereas for most platforms we care about, pointers are size-8 and align-8, so the reordering will save 8 bytes of padding. |
4645 ↗ | (On Diff #203713) | Please avoid adding setters to Expr subclasses if possible. We want the AST to be immutable. (Befriend ASTReaderStmt instead.) |
4674 ↗ | (On Diff #195980) | Using a SmallVector here is a bug; the destructor is not run on Expr nodes so this will leak memory. Please do change to using tail allocation here. |
include/clang/Basic/DiagnosticParseKinds.td | ||
752 ↗ | (On Diff #203713) | . Did -> ; did so this fits the general diagnostic pattern regardless of whether the diagnostic renderer capitalizes the diagnostic. |
753 ↗ | (On Diff #203713) | Move ? to the end. |
lib/AST/ExprCXX.cpp | ||
32–35 ↗ | (On Diff #203713) | These #includes are all unacceptable. AST is layered below Sema, and cannot depend on Sema headers and classes. |
lib/AST/ExprConstant.cpp | ||
9953–9955 ↗ | (On Diff #203713) | It is a logic error for the expression evaluator to encounter a value-dependent expression, and we assert on the way into the evaluator if we would encounter one. You don't need to check this here. |
lib/AST/StmtProfile.cpp | ||
1325 ↗ | (On Diff #203713) | We don't /need to/ profile isSimple, but we still could. (This "is equivalent to" doesn't override the general ODR requirement that you spell the expression with the same token sequence.) Do we mangle simple and compound requirements the same way? (Has a mangling for requires-expressions even been proposed on the Itanium ABI list yet?) |
lib/Parse/ParseExprCXX.cpp | ||
3097 ↗ | (On Diff #203713) | Recovering by producing an (always-satisfied) RequiresExpr with no requirements would seem reasonable here. |
3113 ↗ | (On Diff #203713) | This is incorrect. A simple-requirement can begin with the typename keyword. (Eg, requires { typename T::type() + 1; }) The right way to handle this is to call TryAnnotateTypeOrScopeToken() when you see tok::kw_typename, and then detect a type-requirement as a tok::annot_typename followed by a tok::semi. (By following that approach, you can also handle cases where the typename is missing.) You'll need to deal specially with the case where the nested-name-specifier is omitted, since in that case the typename keyword does not form part of a typename-specifier; in that case, after TryAnnotateTypeOrScopeToken() you'll have a tok::kw_typename, tok::identifier, tok::semi sequence or a tok::kw_typename, tok::annot_template_id, tok::semi sequence to detect and special-case. |
3125 ↗ | (On Diff #203713) | Do we need to skip the entirety of the requires-expression in this case? Skipping to the semicolon would seem sufficient, and would allow us to recover better. |
3299 ↗ | (On Diff #203713) | You need to distinguish between a simple-requirement and a nested-requirement here. requires { requires { 0; }; }; is valid and has a requires-expression as a simple-requirement. (We should issue a warning on such cases, since only the validity of the requires-expression is checked -- and it's always valid -- and its result is ignored. But we should at least parse it properly.) This might need unbounded lookahead. Eg, consider requires { requires (A<T1, T2, T3> && B): if the next token is {, it's a requires-expression in a simple-requirement. Otherwise, it's a nested-requirement. In general, I think this is correct:
You can optimize the second bullet by tentatively parsing a function parameter list (TryParseFunctionDeclarator) rather than just skipping to the ); that will often let us determine that we have a nested-requirement by inspecting fewer tokens. (And hopefully in the common case we can just see that the parenthesized tokens begin with a template-id naming a concept, so we can't possibly have a function parameter list.) |
lib/Sema/SemaExpr.cpp | ||
1809 ↗ | (On Diff #203713) | (Unused.) |
lib/Sema/SemaTemplateInstantiate.cpp | ||
1021–1052 ↗ | (On Diff #203713) | Do we really need to duplicate the TreeTransform code here, and add a representation for a requirement whose substitution failed, and so on? Possible alternative: create a SFINAETrap for the entire requires-expression, perform a TreeTransform of the whole thing, and if it fails, capture the diagnostic and build a FailedRequiresExpr node that stores the diagnostic and evaluates to false. (Under this model, a non-value-dependent RequiresExpr would always evaluate to true.) That should simplify this code, and the representations of RequiresExpr and Requirements (since you don't need to model "substitution failed" any more, nor conditionally store a diagnostic on a requirement). Are there cases where we want to retain more information than that? (I don't think there are, since you don't actually retain any useful information from the requirement that failed, other than the diagnostic itself, but maybe I've missed something.) The results of a successful substitution into some parts of a failed RequiresExpr don't really seem interesting. |
1028 ↗ | (On Diff #203713) | Typo Satisfcation -> Satisfaction |
lib/Serialization/ASTWriterStmt.cpp | ||
14 ↗ | (On Diff #203713) | Don't use angled includes to find Clang headers. (Also, the use of this header here is deeply suspicious: this header describes an interface between Sema and the Parser that Serialization should generally not need to know about.) |
lib/AST/StmtProfile.cpp | ||
---|---|---|
1325 ↗ | (On Diff #203713) | Not yet :( |
lib/Sema/SemaTemplateInstantiate.cpp | ||
---|---|---|
1021–1052 ↗ | (On Diff #203713) | Wouldn't it be interesting to store the successful requirements prior to the failed ones? I imagine some tools might want this information |
Refactor Requirement and subclasses to the AST, extract their creation logic to Sema.
(Partial comments; I'll try to complete the review today or tomorrow.)
clang/include/clang/AST/ExprCXX.h | ||
---|---|---|
4933 | You're adding a total of ~400 lines here; that seems sufficient to factor out into a separate header (ExprConcepts.h, containing this and ConceptSpecializationExpr maybe?) | |
4936 | This name (clang::Requirement) is a little too general for my tastes; consider moving these to a sub-namespace. | |
clang/include/clang/AST/RecursiveASTVisitor.h | ||
2714–2716 | We should traverse the entire type-constraint here, including the nested name specifier and concept name. | |
clang/include/clang/Basic/DiagnosticParseKinds.td | ||
766 | requirements -> requirement | |
774 | Missing ? somewhere in this diagnostic, I think. | |
clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
2638 | " and not" -> ", not" would be a bit more consistent with how we phrase similar things elsewhere. | |
clang/include/clang/Sema/Sema.h | ||
6285 | & on the right, please (here and below). | |
clang/lib/AST/ExprCXX.cpp | ||
1848–1855 | This representation for a type-constraint seems a bit surprising. Do we really need the template parameter + template parameter list here? It seems to me like we should only really need the TypeConstraint itself, plus its immediately-declared constraint. |
Addressed comments in latest diff.
clang/include/clang/AST/RecursiveASTVisitor.h | ||
---|---|---|
2714–2716 | We are traversing all that down the line when traversing the actual template type parameter. |
clang/lib/AST/StmtPrinter.cpp | ||
---|---|---|
2296 | I can see why this is the most straightforward way to implement this, but ... yuck. Please add a FIXME :) | |
clang/lib/AST/StmtProfile.cpp | ||
1363 | This means "equivalent to" in the plain English sense, not in the [temp.over.link] sense, and in any case is not normative. The normative rule is the one in [temp.over.link] based on the ODR / token sequence. It'd be fine to profile isSimple() if you want to. (But it's not necessary since a { expr }; requirement is functionally equivalent to a expr; requirement.) |
clang/include/clang/AST/ExprConcepts.h | ||
---|---|---|
145–149 | Please add a FIXME to store the diagnostic semantically rather than as a pre-rendered string. | |
clang/lib/Parse/ParseExprCXX.cpp | ||
3326 | Please call these ActOnStart... and ActOnFinish... for consistency with other such functions in Sema. | |
3329 | This if body is long (even though it's just one statement); please add braces here. | |
3373 | him -> them, and please add a period. | |
3428 | Please add a FIXME to avoid walking these tokens twice (once in TryParseParameterDeclarationClause and again here). | |
3485–3494 | Doing this by messing with the token stream and trying to perform annotation twice seems quite unclean, and you're doing too much of the semantic work of interpreting the typename requirement from inside the parser. Here's what I'd suggest: First, call TryAnnotateCXXScopeToken(), which will annotate a scope and a following template-id (if any) If we then have
then we have a typename-requirement. Call Sema::ActOnTypeRequirement and pass in the information you have (a scope and an identifier or template-id), and build a suitable type representation from Sema. (You might want to always build an ElaboratedType with ETK_Typename, even if the nested name specifier is non-dependent, to match the source syntax.) And do this all in a tentative parse action, so you can revert it to put the typename keyword back when you find out that this is actually a simple-requirement. | |
3538 | Is it useful to suggest a compound-requirement here? I expect this'll be hit a lot more for normal typos in the expression (eg, missing semicolon, forgotten operator, etc) than in cases where a compound-requirement was intended. ExpectAndConsumeSemi has various tricks to diagnose these cases well, and should be expected to gain more such tricks in the future; you should just call it here unless you're confident you know what the user did wrong (eg, in the noexcept case). | |
3555 | he -> they | |
3563 | This should be called ActOn... not Create.... | |
clang/lib/Sema/SemaConcept.cpp | ||
437 | Please don't use auto here; the type is not clear from context. | |
1018–1022 | Please move this long comment to the previous line. Long trailing comments are a pain to work with and harder to read. | |
clang/lib/Sema/SemaExpr.cpp | ||
353–354 | Please make sure you have a test for this in the typeid case (which we parse as unevaluated and then might transform to potentially-evaluated if the operand is an lvalue of polymorphic class type), eg: class X { virtual ~X(); }; requires (X &x) { static_cast<int(*)[(typeid(x), 0)]>(nullptr); } | |
clang/lib/Sema/SemaExprCXX.cpp | ||
8510–8513 | I'm surprised this is necessary; do we not already check for redefinition when building the ParmVarDecl and pushing it into scope? | |
clang/lib/Sema/TreeTransform.h | ||
11260–11262 | Add braces here please. |
clang/include/clang/Parse/Parser.h | ||
---|---|---|
806–807 | Looks like the flag you're adding here is never actually used any more. Please remove it again. | |
clang/lib/AST/StmtPrinter.cpp | ||
2296 | I think you've actually fixed this now: we always build a typename type, so never need to print an extra typename here. | |
clang/lib/Sema/SemaExprCXX.cpp | ||
8511–8512 | This variable is now unused; please remove it. | |
8529 | If we only do this now, do we properly handle requires (int x, decltype(x) y)? (Or is that handled in a separate function prototype scope which we've already left?) | |
clang/lib/Sema/SemaTemplate.cpp | ||
10253–10255 | Minor cleanup: I don't think we can ever reasonably get here for enable_if<...>::type with no Ctx. I mean, it's possible, but only from lexically within the definition of enable_if itself (or possibly a class derived from it?), and that's really not worth checking for. We could just skip calling isEnableIf and this whole if block if we don't have a Ctx. |
This breaks tests on non-Win, see e.g. http://45.33.8.238/linux/7846/step_7.txt http://45.33.8.238/mac/6120/step_7.txt
Please watch http://lab.llvm.org:8011/console for a bit after landing changes.
Please take a look, and if the fix isn't obvious please revert while you investigate.
Doesn't this still break check-clang everywhere? See e.g. http://45.33.8.238/linux/7860/step_7.txt
It looks like this is some incremental build issue; after a clean build everything's happy. I'll see if I can repro which sequence of builds gets me in the bad state, but it's possibly unrelated to this CL. If that changes, I'll comment here again.
This name (clang::Requirement) is a little too general for my tastes; consider moving these to a sub-namespace.