Page MenuHomePhabricator

[Concepts] Function trailing requires clauses
ClosedPublic

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

Details

Summary

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
clang/lib/Parse/ParseDecl.cpp
2093–2094

They expect the requires clause first.

clang/lib/Parse/ParseDeclCXX.cpp
3813–3824

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.

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

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

nridge added a subscriber: nridge.Thu, Dec 26, 9:17 PM
rsmith marked an inline comment as done.Tue, Jan 7, 3:48 PM
rsmith added inline comments.
clang/include/clang/AST/ASTNodeTraverser.h
394–395

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
2369

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

320

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

clang/include/clang/Sema/Sema.h
6183

emplate -> template

6193–6195

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

6201–6203

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.

6217

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
3317

Please add a FIXME to properly import the InheritedConstructor information.

clang/lib/AST/Decl.cpp
1843–1844

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
2048–2049

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
2321–2322

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

3801

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

3813–3824

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

3829

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

919

parent -> paren

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

1266–1278

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

1285

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.

1407

This is also not a primary-expression.

1417

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

1634–1644

These are all not primary-expressions.

1650

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.

1675–1676

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
263–266

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

clang/lib/Sema/SemaConcept.cpp
758

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

794–806

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
8339–8348

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

10593–10597

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
6278

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.

6319–6326

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

6803–6812

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.

9533

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

9998–10003

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?

10012

he -> they

10054

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.

11318–11321

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

11372–11380

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.

11474–11484

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

12008–12025

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

12026

Please add some kind of punctuation after "constrained".

clang/lib/Sema/SemaTemplateDeduction.cpp
3437–3444

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?

rsmith added inline comments.Tue, Jan 7, 7:48 PM
clang/lib/Sema/SemaTemplateDeduction.cpp
3437–3444

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.Wed, Jan 8, 2:53 PM
saar.raz marked 52 inline comments as done.

Address CR comments by rsmith.

saar.raz marked an inline comment as done.Wed, Jan 8, 2:54 PM
saar.raz added inline comments.
clang/lib/Sema/SemaOverload.cpp
6319–6326

Should I fix this in this patch?

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

Use InclusiveOr precedence when recovering from non-primary constraint expr

rsmith accepted this revision.Wed, Jan 8, 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.

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
6061–6063

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?

1638

These are not primary-expressions.

clang/lib/Sema/SemaConcept.cpp
80–84

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
6277–6278

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

6803–6804

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

10029

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
3

Are the #if 0s here intentional?

clang/test/CXX/temp/temp.constr/temp.constr.order/class-template-partial-specializations.cpp
61–62

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.Wed, Jan 8, 4:03 PM
saar.raz updated this revision to Diff 237004.Thu, Jan 9, 3:19 AM

Address final CR comments by rsmith

This revision was automatically updated to reflect the committed changes.

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.