Page MenuHomePhabricator

[Concepts] Requires Expressions
ClosedPublic

Authored by saar.raz on Aug 6 2018, 2:16 PM.

Details

Summary

Implement the requires expression. Depends on D44352.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
riccibruno added inline comments.Apr 21 2019, 10:24 AM
include/clang/AST/ExprCXX.h
4722 ↗(On Diff #195980)

You should also provide the const-qualified version.

saar.raz updated this revision to Diff 203713.Jun 8 2019, 4:11 PM

Remove outdated grammar support in return-type-requirements, removing some hacks and simplifying the code.

rsmith added inline comments.Jun 10 2019, 3:50 PM
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
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.

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.)

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:

  • If the token after requires is {, we have a requires-expression in a simple-requirement.
  • If the token after requires is (, skip to the matching ); if the next token is {, we have a requires-expression in a simple-requirement.
  • In all other cases, we have a nested-requirement.

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.)

saar.raz updated this revision to Diff 204176.Jun 11 2019, 2:54 PM

Ignore RequiresExprBodyDecl when looking for function level decl context

saar.raz updated this revision to Diff 204788.Jun 14 2019, 9:24 AM

Adjusted to changes in previous patches

saar.raz updated this revision to Diff 224935.Oct 14 2019, 4:38 PM
saar.raz marked an inline comment as done.

Address some CR comments

saar.raz added inline comments.Oct 14 2019, 5:24 PM
lib/AST/StmtProfile.cpp
1325 ↗(On Diff #203713)

Not yet :(

saar.raz marked 17 inline comments as done.Oct 14 2019, 5:27 PM
saar.raz updated this revision to Diff 227944.Nov 5 2019, 12:03 PM
saar.raz marked 6 inline comments as done.

Address final CR comments, add PCH tests

saar.raz added inline comments.Nov 5 2019, 12:04 PM
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

saar.raz updated this revision to Diff 236687.Tue, Jan 7, 1:55 PM

Refactor Requirement and subclasses to the AST, extract their creation logic to Sema.

saar.raz edited the summary of this revision. (Show Details)Tue, Jan 7, 1:55 PM

(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.

nridge added a subscriber: nridge.Tue, Jan 14, 6:34 PM
saar.raz updated this revision to Diff 238349.Wed, Jan 15, 1:19 PM
saar.raz marked 5 inline comments as done.

Address first round of CR comments

saar.raz marked 4 inline comments as done.Wed, Jan 15, 1:24 PM

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.

rsmith added inline comments.Wed, Jan 15, 2:08 PM
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.)

rsmith added inline comments.Wed, Jan 15, 2:53 PM
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

  • optionally, an annot_cxxscope, then
  • an identifier or an annot_template_id, then
  • a semi (or, not an l_brace nor an l_paren, depending on how you want to recover from errors)

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.

saar.raz updated this revision to Diff 238411.Wed, Jan 15, 8:02 PM
saar.raz marked 16 inline comments as done.

Address CR comments.

saar.raz marked an inline comment as done.Wed, Jan 15, 8:06 PM

Addressed comments in latest diff.

rsmith accepted this revision.Thu, Jan 16, 7:33 PM
rsmith added inline comments.
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 revision is now accepted and ready to land.Thu, Jan 16, 7:33 PM
This revision was automatically updated to reflect the committed changes.
saar.raz marked 6 inline comments as done.

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

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.