Page MenuHomePhabricator

[Concepts] Function trailing requires clauses
Needs ReviewPublic

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

saar.raz created this revision.Feb 15 2018, 3:03 PM
saar.raz retitled this revision from 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. to Function trailing requires clauses.Feb 15 2018, 3:30 PM
saar.raz edited the summary of this revision. (Show Details)
saar.raz retitled this revision from Function trailing requires clauses to [Concepts] Function trailing requires clauses.Mar 10 2018, 4:25 AM
saar.raz updated this revision to Diff 147188.May 16 2018, 2:57 PM
  • Fixed handling of empty/non-expression after trailing requires keyword.
  • Unified satisfaction check for templated/non-templated constraint exprs
saar.raz updated this revision to Diff 159349.Aug 6 2018, 12:07 PM
  • Fix bad diagnostic detection and suppression
rsmith added a comment.Aug 6 2018, 3:42 PM

Looking promising, but this patch will need some rework: you need to track the trailing requires clause on the Declarator itself, not on the DeclChunk, because it's not part of the DeclChunk (and may appear in contexts where there is no function chunk).

include/clang/AST/Decl.h
1793–1796

Please find a way to store this that doesn't make all FunctionDecls 8 bytes larger. At the very least, you can move this to CXXMethodDecl, but perhaps it'd be better to include this as an optional component in the DeclInfo.

1795

Not necessarily for this patch, but you'll need to think about how to represent a not-yet-instantiated trailing requires clause. (A requires clause isn't instantiated as part of instantiating the function it's attached to; it's instantiated later, when needed, like an exception specification or default argument.)

include/clang/Basic/DiagnosticSemaKinds.td
2433

"virtual function cannot have a requires clause" would be more in line with how we usually word diagnostics.

2435

We generally separate a general problem and a specific problem with a colon, not a hyphen, in diagnostics.

lib/Parse/ParseDecl.cpp
6098–6165

This is the wrong place to parse a requires-clause: a requires-clause is a trailing part of the overall init-declarator or member-declarator, not part of the function declarator chunk. For example:

using T = void ();
T x requires true; // ok, but you reject
void (f() requires true); // ill-formed but you accept
void (f()) requires true; // ok but you reject
6102

The ConceptsTS check here is redundant. If we see a kw_requires token, the feature is enabled.

6106–6110

This is not a reasonable way to deal with parse errors. Parsing an expression can have non-local effects, and suppressing diagnostics like this is not a safe way to provide error recovery. You're also suppressing all warnings within the expression, which is also inappropriate.

Instead, you should just parse the requires-clause. If you parse it correctly (not as a constraint-expression) the parse will stop before a possible -> token anyway, because a top-level -> expression is not permitted in a constraint-logical-or-expression.

6109

This isn't right: a constraint-logical-or-expression is required here, not a constraint-expression.

6112–6113

Clang style puts the && on the previous line.

lib/Parse/ParseTentative.cpp
955–956

This is wrong.ParenCount isn't just *your* parens, it's all enclosing ones. And I don't think you need this check at all: the requires is either part of a suitable declarator or it's ill-formed, so we can just disambiguate as a declarator (that way we'll correctly handle all valid programs and correctly reject all ill-formed ones, with a diagnostic saying that the nested declarator can't have a requires-clause).

lib/Sema/SemaDecl.cpp
7848–7851

Please parenthesize the && subexpression and run this through clang-format.

8377–8381

This is the wrong place for this check. We don't yet know whether the function is virtual here in general. A function can become virtual due to template instantiation:

template<typename T> struct A : T { void f() requires true; };
struct B { virtual void f(); };
template struct A<B>; // error, A<B>::f is constrained and virtual

This is perhaps a wording defect: it's not clear that A::f() really should override B::f(), but that is the consequence of the current rules. I've posted a question to the core reflector.

lib/Sema/SemaExpr.cpp
2716–2732

Why do we ever fail this check? Overload resolution should never select a function with unsatisfied constraints, and likewise for taking the address of / binding a reference to such a function.

lib/Sema/SemaOverload.cpp
1208–1221

This check is too late. We've already passed some return false;s at this point. (Consider changing the CUDA checks to only early-return on failure rather than moving this check earlier.)

6106

The ConceptsTS check here doesn't make any sense to me: if you've already parsed a requires clause, you should always check it.

6619–6629

According to [over.match.viable]p3, constraint satisfaction should be checked before we attempt to form implicit conversion sequences. This can affect the validity of code, if the associated constraints disable the function in a case where implicit conversion sequence formation would result in the program being ill-formed due to triggering a bad template instantiation (with an error outside an immediate context).

9123–9135

According to [over.match.best], this check belongs immediately after the "more specialized template" check, not down here.

lib/Sema/SemaTemplateInstantiateDecl.cpp
1637–1646

This is wrong. Per [temp.decls]p2, requires-clauses are instantiated separately from their associated functions. That doesn't need to be handled in this change, but please at least include a FIXME.

1641

Checking !isUsable() here is wrong. (1: SubstExpr should never produce a valid-but-null Expr*, 2: even if it did, that means it succeeded, which means it's wrong for you to return nullptr without producing a diagnostic.)

1965

(Likewise.)

lib/Sema/SemaTemplateVariadic.cpp
884–886

Please reformat.

saar.raz added inline comments.Aug 7 2018, 1:40 PM
lib/Sema/SemaDecl.cpp
8377–8381

I don't really see why A::f() should override B::f() indeed - since it is not marked virtual nor override, shouldn't it just hide B::f()? or am I missing something here?

rsmith added inline comments.Aug 7 2018, 2:08 PM
lib/Sema/SemaDecl.cpp
8377–8381

Functions override base class virtual functions if they have matching name, parameter types, cv-qualifiers and ref-qualifiers, *regardless* of whether they're declared virtual etc. Eg:

struct A { virtual void f(); };
struct B : A { void f(); };

B::f overrides A::f, and as a consequence, B::f is implicitly a virtual function. See [class.virtual]p2 and its footnote.

Note that the determination of whether the derived class function overrides the base class function doesn't care about the associated constraints on the derived class function. (After checking with the core reflector, general consensus seems to be that this is the right rule, and my previous example should indeed be ill-formed because it declares a constrained virtual function.)

Herald added a reviewer: shafik. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
saar.raz updated this revision to Diff 195033.Apr 13 2019, 1:00 PM
  • Add diagnostic explaining the unintuitive subsumption rules when user might be relying on expression equivalence for odering
  • Fixed incorrect checking of trailing requires clause subsumption
  • Fixed missing conversion constraint checking
  • Fixed constraint checking in function templates to recognize function parameters
  • Address CR comments by rsmith
    • Move TrailingRequiresClause to ExtInfo
    • Fixed diagnostic styling
    • Correct checking of constrained virtual functions
    • Small formatting fixes
    • Parse constraint-logical-or-expressions in requires clauses
    • Parse the requires clause as part of the init-declarator, member-declarator or function-definition
    • No dirty diagnostic suppression when trying to parse switched-order trailing requires and return type
martong requested changes to this revision.Apr 16 2019, 1:37 AM
martong added inline comments.
lib/AST/ASTImporter.cpp
2135

First you have to import the trailing requires clause and then pass the imported Expr here.
I think you can reuse the importSeq call above.

This revision now requires changes to proceed.Apr 16 2019, 1:37 AM
saar.raz updated this revision to Diff 195978.Apr 20 2019, 12:50 PM
  • Remove #if from test
saar.raz updated this revision to Diff 196157.Apr 22 2019, 4:19 PM
  • Address CR comment
  • Adjust to new getAssociatedConstraints interface
  • Add destructor and conversion operator tests
martong accepted this revision.Apr 23 2019, 7:32 AM

From the ASTImporter point of view it looks good, thanks for the changes!

This revision is now accepted and ready to land.Apr 23 2019, 7:32 AM
martong resigned from this revision.Apr 23 2019, 7:33 AM
This revision now requires review to proceed.Apr 23 2019, 7:33 AM
saar.raz updated this revision to Diff 203897.Jun 10 2019, 2:06 PM

Introduce function parameters into scope when parsing function requires clause

saar.raz updated this revision to Diff 204172.Jun 11 2019, 2:51 PM

Allow accessing 'this' in requires clause when appropriate.

saar.raz updated this revision to Diff 204953.Jun 16 2019, 9:32 AM

Add support for lambda expression trailing requires clauses