Page MenuHomePhabricator

[Concepts] Function trailing requires clauses

Authored by saar.raz on Feb 15 2018, 3:03 PM.



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.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
saar.raz updated this revision to Diff 234992.Dec 20 2019, 4:59 PM
saar.raz marked 18 inline comments as done.

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).
saar.raz added inline comments.Dec 20 2019, 5:01 PM

They expect the requires clause first.


Is there anything that needs to be in ActOnFinishTrailingRequiresClause?


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


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.

saar.raz updated this revision to Diff 235200.Dec 23 2019, 10:11 PM

Fix memory handling of NormalizedConstraint and AtomicConstraint
Fix some more failed tests and rebase onto trunk

nridge added a subscriber: nridge.Dec 26 2019, 9:17 PM
rsmith marked an inline comment as done.Jan 7 2020, 3:48 PM
rsmith added inline comments.

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.


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


Perhaps "functional cast expression" (or "explicit type conversion") would be a more familiar term than "initialization expression"?


"appear" is a better word than "come" in this context.


emplate -> template


Copy-paste error here; this is the comment for the previous map.


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.


Don't use \brief. (We enable the doxygen "autobrief" setting so it's unnecessary.)


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


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


Please add a FIXME to properly import the InheritedConstructor information.


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.


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


Combine simplify this by grouping these two conditions together as } else if ( && D.hasGroupingParens()) {.


Please add the (redundant) braces here to make the local style a little more consistent.


Please don't repeat the function name in the documentation comment.


I would put the call to CorrectDelayedTyposInExpr there, but not a big deal either way.


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.


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.


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.


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.


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.


he -> they


parent -> paren

Yes, I know this is pre-existing, but this comment was confusing me when reading your patch :)


These all have function call syntax, so should be treated as postfix-expressions not as primary-expressions.


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.


This is also not a primary-expression.


This should be treated as a postfix-expression, not a primary-expression, like typeid


These are all not primary-expressions.


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.


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.


These are in the wrong order: the requires-clause goes before GNU attributes.


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


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


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


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


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.


(Huh, this looks wrong: we should be checking this before we form parameter conversion sequences, shouldn't we?)


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.


If they're equally-constrained, we should continue and check the later tie-breakers.


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?


he -> they


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.


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


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.


This is reporting no match between a declared template specialization and the known template declarations. Associated constraint mismatches between those templates are irrelevant here.


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


Please add some kind of punctuation after "constrained".


Huh. Doing this down here does indeed seem to be correct, but that seems like a rather bad rule...


You don't need this flag; AddStmt and readStmt can cope fine with null pointers.


Are the #if 0s (here and below) intentional?

rsmith added inline comments.Jan 7 2020, 7:48 PM

This is going to get moved back up to where we had it before by CWG issue 2369 =)

saar.raz updated this revision to Diff 236914.Jan 8 2020, 2:53 PM
saar.raz marked 52 inline comments as done.

Address CR comments by rsmith.

saar.raz marked an inline comment as done.Jan 8 2020, 2:54 PM
saar.raz added inline comments.

Should I fix this in this patch?

saar.raz updated this revision to Diff 236918.Jan 8 2020, 3:11 PM

Use InclusiveOr precedence when recovering from non-primary constraint expr

rsmith accepted this revision.Jan 8 2020, 4:03 PM

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.


This diagnostic is now unused; please remove it.


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?


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.


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


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


It'd be nice to move the call to ParseCastExpression into CheckExpr rather than duplicating it.


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?


These are not primary-expressions.


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


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.


Nit: combine these into one line (if (Expr *RequiresClause = ...) to minimize the scope of RequiresClause.


Nit: as above, combine these to a single line.


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


Are the #if 0s here intentional?


It's useful to include a positive test case (requires C2<T> && C2<U> maybe?) in addition to the negative test case.

This revision is now accepted and ready to land.Jan 8 2020, 4:03 PM
saar.raz updated this revision to Diff 237004.Jan 9 2020, 3:19 AM

Address final CR comments by rsmith

This revision was automatically updated to reflect the committed changes.

This breaks tests on Windows:

Please take a look and revert if it takes a while to investigate.