This is an archive of the discontinued LLVM Phabricator instance.

[Concepts] Requires Expressions
ClosedPublic

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

Event Timeline

saar.raz created this revision.Aug 6 2018, 2:16 PM
saar.raz updated this revision to Diff 159615.Aug 7 2018, 4:02 PM
  • Deal with cases where substitution fails but no diagnostic is available
saar.raz updated this revision to Diff 159758.Aug 8 2018, 11:06 AM
  • Predefine __cpp_concepts when supported
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2019, 4:34 PM
saar.raz updated this revision to Diff 195972.Apr 20 2019, 9:50 AM
  • 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
saar.raz updated this revision to Diff 195980.Apr 20 2019, 1:56 PM
  • Remove #ifs from test
  • Updated test diagnostics
riccibruno added inline comments.Apr 21 2019, 10:24 AM
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.

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
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:

  • 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.Jan 7 2020, 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)Jan 7 2020, 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.Jan 14 2020, 6:34 PM
saar.raz updated this revision to Diff 238349.Jan 15 2020, 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.Jan 15 2020, 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.Jan 15 2020, 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.Jan 15 2020, 2:53 PM
clang/include/clang/AST/ExprConcepts.h
144–148 ↗(On Diff #238349)

Please add a FIXME to store the diagnostic semantically rather than as a pre-rendered string.

clang/lib/Parse/ParseExprCXX.cpp
3288

Please call these ActOnStart... and ActOnFinish... for consistency with other such functions in Sema.

3291

This if body is long (even though it's just one statement); please add braces here.

3335

him -> them, and please add a period.

3390

Please add a FIXME to avoid walking these tokens twice (once in TryParseParameterDeclarationClause and again here).

3447–3456

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.

3500

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

3517

he -> they

3525

This should be called ActOn... not Create....

clang/lib/Sema/SemaConcept.cpp
390

Please don't use auto here; the type is not clear from context.

1009–1013

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
357–358

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
8119–8122

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
11109–11111

Add braces here please.

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

Address CR comments.

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

Addressed comments in latest diff.

rsmith accepted this revision.Jan 16 2020, 7:33 PM
rsmith added inline comments.
clang/include/clang/Parse/Parser.h
784–788

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
8120–8121

This variable is now unused; please remove it.

8138

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
10259–10261 ↗(On Diff #238411)

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.Jan 16 2020, 7:33 PM
This revision was automatically updated to reflect the committed changes.
saar.raz marked 6 inline comments as done.
thakis added a subscriber: thakis.Jan 18 2020, 3:57 AM

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.

I figured out the incremental build test problem, see D73202.