[Concepts] Concept Specialization Expressions
Needs ReviewPublic

Authored by saar.raz on Dec 13 2017, 6:18 PM.

Details

Summary

Part of the P0734r0 Concepts implementation effort. Added Concept Specialization Expressions and tests thereof. Also added constraint expression type verification. This diff depends on D40381.

Diff Detail

There are a very large number of changes, so older changes are hidden. Show Older Changes
  • Moved general concepts-related function into SemaConcept.cpp
Quuxplusone added inline comments.
include/clang/Basic/DiagnosticSemaKinds.td
2404

Extra period . here

lib/Serialization/ASTReaderStmt.cpp
4055

Peanut gallery says: All the other cases look like

S = new (Context) FooExpr(Context, Empty);

or

S = new (Context) FooExpr::CreateEmpty(Context);

not

S = new (Context) FooExpr(Context);

Is the visual difference in this case significant?

test/CXX/concepts-ts/expr/expr.prim/expr.prim.id/p3.cpp
12

"test/Parser/cxx-concept-declaration.cpp" has some syntax tests for "value-concepts" of the form template<bool x> concept.... Would it make sense to add some semantic tests for template<bool x> concept... in or near this test file?
(This could plausibly be "out of scope." I merely mention it.)

test/Parser/cxx-concept-declaration.cpp
36

Peanut gallery says: IIUC, C12<T> is satisfied whenever T has a constexpr constructor and a constexpr implicit conversion to bool that returns true? Or (because this is an atomic constraint and atomic constraints don't do conversions, as we can see on line 31 above) is C12<T> *actually* never satisfiable (but there's no diagnostic expected here because T{} is dependent)?

saar.raz added inline comments.Feb 3 2018, 12:40 PM
test/CXX/concepts-ts/expr/expr.prim/expr.prim.id/p3.cpp
12

Good idea, I'll add some of those.

test/Parser/cxx-concept-declaration.cpp
36

C12 will only accept bool, and there's no diagnostic because it's dependent is correct.

saar.raz updated this revision to Diff 133799.Feb 11 2018, 12:35 PM
  • Addressed comments: Added semantic tests for value and template concepts, removed extra period and modified CSE ctor to fit in nicer.
saar.raz marked 6 inline comments as done.Feb 13 2018, 12:48 AM
saar.raz updated this revision to Diff 137906.Mar 10 2018, 3:57 AM
  • Fixed incorrect checking of atomic constraint types.
faisalv added inline comments.Mar 14 2018, 10:31 AM
include/clang/AST/ExprCXX.h
4416

Typo (Hubert): concepts -> concept

4419

Hubert: The concept named.

4420

NamedConcept

include/clang/Sema/Sema.h
5577

Hubert: clang-format this

5580

Hubert: Typo: emitted

lib/AST/ExprCXX.cpp
1441

Hubert: clang-format

1478

Hubert: This needs a TODO: the idea is not to drop SFINAE errors, but to avoid instantiation that may trigger errors not in the immediate context of instantiation. The substitution needs to happen piecewise.

1485

Hubert: The name of the function gives no indication that it modifies the expression node. The interface of the function extends past what is expected.

saar.raz added inline comments.Mar 14 2018, 11:21 AM
lib/AST/ExprCXX.cpp
1478

Could you elaborate/give an example where this handling is inappropriate?

saar.raz updated this revision to Diff 138497.Mar 14 2018, 11:41 PM

Addressed most comments.

saar.raz marked 6 inline comments as done.Mar 14 2018, 11:43 PM
saar.raz updated this revision to Diff 138499.Mar 14 2018, 11:52 PM

Applied missing clang-format.

saar.raz marked an inline comment as done.Mar 14 2018, 11:55 PM
lib/AST/ExprCXX.cpp
1478

Determining satisfaction requires normalization (lazy normalization should be considered).
The determination of satisfaction then proceeds by handling the left-hand side of conjunction and disjunction constraints before possibly substituting into the right-hand side; i.e., there is short-circuiting behaviour.

Example:

template <typename T>
concept C = true;

template <typename T>
struct Q { static constexpr T value = nullptr; };

template <typename T>
requires C<T> || T::value
struct A { };

template <typename T>
requires C<T> || Q<T>::value
struct B { };

A<int> a; // okay
B<int> b; // okay
saar.raz added inline comments.Mar 16 2018, 11:36 PM
lib/AST/ExprCXX.cpp
1478

OK I see your point. You said this should be a TODO - do you think this should be delayed to a further patch (namely D41569, where we actually deal with diagnostics and stuff)?
About lazy normalization, I tend to think it is not a good idea - you'll probably check satisfaction for every defined constraint expression at some point, and do that many more times than you'd define a new one

lib/AST/ExprCXX.cpp
1478

The status quo of this patch with regards to this topic is not quite something that I would like to commit onto trunk.

As for normalization: The process is needed for distinct purposes with somewhat differing output requirements.
The order-dependent form (to check for satisfaction) can be integrated with the substitution and evaluation: that is, the output of normalization is realized in control flow, and not "stored".

The order-independent form (to check for subsumption) could be delayed until overload resolution with a constrained candidate reaches the point where determining subsumption is necessary.

saar.raz updated this revision to Diff 140948.Apr 4 2018, 5:45 AM
  • Switched to piecewise substitution of atomic constraints when evaluating constraint expressions
  • Added tests for piecewise substitution behavior
  • Removed some redundant parentheses
hintonda removed a subscriber: hintonda.Apr 4 2018, 4:37 PM
saar.raz updated this revision to Diff 159234.Aug 5 2018, 2:49 PM
  • Switch to ASTTemplateArgumentListInfo, add ConstantEvaluated context when parsing constraints
rsmith added inline comments.Aug 6 2018, 5:34 PM
include/clang/AST/ExprCXX.h
4420

You should also track the FoundDecl and the optional NestedNameSpecifierLoc (just like a DeclRefExpr would). Clang-based tools (particularly refactoring tools) need those. There's also an optional template keyword, but it can never matter in practice because the nested name specifier can never be dependent, so it's probably not the most high-priority thing to track.

4423–4424

It'd be nice if at least one of these two arrays could be tail-allocated.

4459–4463

Passing Sema in here is a major layering violation. Here's how this should work:

  • The AST code represents the AST model; its functions should generally not be able to fail.
  • Sema checks the semantic constraints and enforces the validity of operations, and updates the AST node as appropriate

In this case, Sema should be checking the template argument list as written against the concept's parameter list, forming a resolved list of template arguments, and then passing both the as-written information and the resolved arguments to this AST node for storage.

4474–4481

Do you really need mutators for the 'satisfied' flag and the concept name location?

include/clang/Basic/DiagnosticSemaKinds.td
2405

Find a way to describe this problem without pretty-printing a substituted expression.

2407

Remove the '%0' here; you're already underlining it with a SourceRange (right?).

Please read http://clang.llvm.org/docs/InternalsManual.html#the-format-string and in particular "Take advantage of location information. The user will be able to see the line and location of the caret, so you don’t need to tell them that the problem is with the 4th argument to the function: just point to it."

2409

There's not really any such thing as a "concept specialization". Perhaps "while checking satisfaction of '%0'" or similar?

lib/AST/ExprCXX.cpp
1460

You will need to skip over a top-level ExprWithCleanups (I can't think off-hand of any other nodes that could show up here, but there might be more).

1552–1553

Build a CodeSynthesisContext for this rather than adding an ad-hoc note to the last diagnostic that happens to come from substitution (which might not be the error you care about, and might even be suppressed by warning flags).

lib/AST/ExprConstant.cpp
9085–9087

You don't need to check for this. If the evaluator is called on a value-dependent expression, that's a bug in the caller.

lib/AST/StmtPrinter.cpp
2553

You should print out the nested name specifier here. (And ideally also the template keyword, if specified...). And you should print the name of the found declaration, not the name of the concept (although they can't differ currently).

lib/Parse/ParseTemplate.cpp
372–377

Remove comment and braces. We don't need to say that an invalid ExprResult means a diagnostic was produced, because that's always implied by the ExprResult being invalid.

lib/Sema/SemaConcept.cpp
23

* on the right, please. And replace BinaryOperator with auto since it's initialized by a dyn_cast.

25–26

&& on the previous line, and align the two operands. (Or get clang-format to do it for you.)

35

What justifies rejecting this prior to any use of the concept that would result in a satisfaction check?

(I think checking this is a good thing; what I'm wondering is whether we need to file a core issue to get the wording updated to allow us to reject such bogus concepts even if they're never used.)

38–42

Call IgnoreParenImpCasts before checking the type of an atomic constraint, rather than adding an extra recursive step here.

123

Please fix :)

lib/Sema/SemaExprCXX.cpp
7690 ↗(On Diff #159234)

Please fix.

lib/Sema/TreeTransform.h
2943–2963

We need to convert the template arguments when rebuilding. This should call CheckConceptTemplateId instead. (After fixing that, please inline CreateConceptSpecializationExpr into its only caller.)

Please also add a test that checks we do convert the template arguments here.

@rsmith - thanks for the responses!
Will also address the comments I haven't responded to soon.

include/clang/AST/ExprCXX.h
4420

Isn't the FoundDecl just the NamedConcept?

4423–4424

Now that you mention it, TemplateArgs is never even used or set, I'll remove it.
As for TemplateArgInfo - do you mean allocate the ASTTemplateArgumentListInfo itself as a trailing obj or the individual TemplateArgumentLocs? the former seems more reasonable to me as it's convenient to have an ASTTemplateArgumentListInfo object around

4474–4481

They're here only because of ASTReader/Writer. I guess I can use a friend decl instead.

lib/AST/StmtPrinter.cpp
2553

Are there possible upcoming changes that'll make them differ?

lib/Sema/SemaConcept.cpp
35

I guess this is already justified, if awkwardly (NDR) by [temp.res]p8.2

saar.raz added inline comments.Aug 7 2018, 12:22 PM
lib/Sema/SemaConcept.cpp
35

Correction - it says that IFNDR occurs when no substitution would result in a valid expression, so maybe this is well formed after all.
In this case it is a valid expression but not a valid constraint expression, maybe that's the missing word here?

saar.raz updated this revision to Diff 160216.Aug 10 2018, 5:33 PM

Address Richard's CR comments. Among other things:

  • CSEs overhauled and now store both source and converted template arguments (latter are tail-allocated), template KW location and NNS.
  • CSEs no longer violate layering - satisfaction check now moved back to CheckConceptTemplateId.
  • CodeSynthesisContexts created for both the act of checking the associated constraints of a template or the constraint expression of a concept (non-SFINAE), and for the act of substituting template arguments into a(n atomic) constraint expression (SFINAE).
  • Added more tests for cases where substitution leads to a non-SFINAE failure and emits diagnostics
  • Fixed and added test for incorrect conversion of instantiated CSE argument list.
saar.raz marked 22 inline comments as done.Aug 11 2018, 3:18 AM
saar.raz updated this revision to Diff 160237.Aug 11 2018, 3:22 AM

Removed unused "note_in_concept_specialization" diagnostic ID.

saar.raz marked an inline comment as done.Aug 11 2018, 3:22 AM
Quuxplusone added inline comments.Aug 11 2018, 10:41 AM
include/clang/Sema/Sema.h
7134

Missing a trailing comma here.

test/CXX/concepts-ts/expr/expr.prim/expr.prim.id/p3.cpp
121

Nit: You could use // expected-note@-1{{...}}, // expected-note@-2{{...}} to make lines like this more readable.

test/Parser/cxx-concept-declaration.cpp
48

Should you static-assert the expected results?

static_assert(!C12<bool>);
static_assert(C13<int>);
static_assert(C14<int>);
static_assert(C15<int>);
static_assert(C16<true>);
static_assert(C16<false>);
static_assert(C17<true>);
static_assert(!C17<false>);
saar.raz added inline comments.Aug 11 2018, 12:29 PM
test/CXX/concepts-ts/expr/expr.prim/expr.prim.id/p3.cpp
121

😮😳 Well you learn something every day

test/Parser/cxx-concept-declaration.cpp
48

That's not the point of these tests but it couldn't hurt, I guess

saar.raz updated this revision to Diff 160437.Aug 13 2018, 1:32 PM

Address Arthur's comments, add missing CorrectDelayedTyposInExpr

steveire added inline comments.
include/clang/AST/DeclTemplate.h
3063
steveire removed a subscriber: steveire.Aug 13 2018, 2:01 PM
saar.raz added inline comments.Aug 13 2018, 2:05 PM
include/clang/AST/DeclTemplate.h
3063

Good to know, but I'm not yet merged against trunk so getEndLoc isn't here yet (will soon post a merged version of this patch)

saar.raz updated this revision to Diff 161343.Aug 17 2018, 3:10 PM
  • Fix bad ArgsAsWritten assertion, add missing null initializer in ConceptSpecializationExpr.
rsmith added inline comments.Aug 17 2018, 7:40 PM
include/clang/AST/ExprCXX.h
4420

Not in general, no. The FoundDecl could be a UsingShadowDecl.

lib/AST/ExprCXX.cpp
1472

This is incorrect. Whether the expression is dependent is not the same thing as whether it's instantiation-depnedent. A concept specialization should be considered instantiation-dependent if it has any instantiation-dependent template arguments, and should be considered value-dependent if it has any dependent template arguments.

For example, Concept<sizeof(sizeof(T))> has an instantiation-dependent template argument, but no dependent template arguments, so should be instantiation-dependent (the validity of the construct can depend on the type T) but not value-dependent (the value of the construct is always Concept<sizeof(size_t)>, which doesn't depend on T).

lib/AST/StmtPrinter.cpp
2553

p0945r0 would allow them to differ.

lib/Sema/SemaConcept.cpp
106–113

Please collect the notes from EvalutaeAsBooleanCondition and emit them here, so the user knows /why/ the expression was non-constant.

lib/Sema/SemaTemplate.cpp
3908–3912

Add braces around this for, because its body contains multiple statements. (Generally, if an inner construct has braces we want outer constructs to have them too.)

lib/Sema/SemaTemplateInstantiate.cpp
679–681

Is this note ever useful? It will presumably always point into the same concept definition that the prior diagnostic also pointed at, and doesn't seem to add anything in the testcases.

Maybe we could keep the CodeSynthesisContext around as a marker that we've entered a SFINAE context, but not have any corresponding diagnostic. (The note produced for the enclosing ConstraintsCheck context covers that.) Or we could remove this and store a flag on the ConstraintsCheck to indicate whether we're in a SFINAEable portion of it.

saar.raz added inline comments.Aug 18 2018, 1:18 AM
lib/Sema/SemaTemplateInstantiate.cpp
679–681

If the concept definition is multiline/contains macros, this would point at the exact place where the problematic constraint occured, we should probably add tests for this case though.
Maybe we can omit the diagnostic when the concept and the constraint are on the same line or something?

saar.raz added inline comments.Aug 18 2018, 1:29 AM
lib/Sema/SemaTemplateInstantiate.cpp
679–681

Correction - I just now realized you were referring to the 'in instantiation of template class' note and not the 'while checking the satisfaction...' note.
In this case - the only case I can think of where the problematic instantiation and the constraint expression would be in very different places is indeed multiline constraint expressions or macros in a constraint expression (but you know better than I do - can you think of any other cases? or maybe there are other non-SFINAE errors that can result from substitution and would not produce a note?). Maybe these cases are remote enough to warrant removing this diagnostic or as I said earlier omit them in cases where they are unhelpful.

saar.raz updated this revision to Diff 161372.Aug 18 2018, 5:39 AM
  • Address CR comments - add FoundDecl tracking, instantiationdependent calculation, display non-constant expression diagnostic notes