This is an archive of the discontinued LLVM Phabricator instance.

[Concepts] Concept Specialization Expressions
ClosedPublic

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

Event Timeline

saar.raz created this revision.Dec 13 2017, 6:18 PM
changyu added inline comments.Dec 13 2017, 9:45 PM
lib/AST/ExprCXX.cpp
1481

You have four spaces of indenting here.

lib/Parse/ParseExpr.cpp
227

Indenting.

saar.raz updated this revision to Diff 126950.Dec 14 2017, 7:04 AM
  • Fixed indentation problems
saar.raz updated this revision to Diff 126953.Dec 14 2017, 7:07 AM

Previous updated diff mistakenly included D40381, fixed that.

saar.raz marked 2 inline comments as done.Dec 14 2017, 7:08 AM

Fixed indentations.

faisalv added inline comments.
lib/Parse/ParseExpr.cpp
225–226

Are you sure this only accepts logical-or-epxressions? Unlike Hubert's initial implementation here, this seems to parse any expression (i.e including unparenthesized commas and assignments)? What am I missing?

saar.raz added inline comments.Dec 17 2017, 4:31 AM
lib/Parse/ParseExpr.cpp
225–226

Mm.. Good point, I was under the false assumption that this was equivalent - I'll fix it back, thanks :)

saar.raz updated this revision to Diff 128121.Dec 24 2017, 1:27 PM

Reverted to original constraint parsing code.

saar.raz marked 2 inline comments as done.Dec 24 2017, 1:28 PM
saar.raz updated this revision to Diff 132743.Feb 3 2018, 10:07 AM
  • 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
4053

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
374–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.

45

Please fix :)

rsmith added inline comments.Aug 6 2018, 5:34 PM
lib/Sema/SemaExprCXX.cpp
7690

Please fix.

lib/Sema/TreeTransform.h
2943–2959

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
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
3907–3911

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 ↗(On Diff #161343)

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 ↗(On Diff #161343)

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 ↗(On Diff #161343)

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
saar.raz marked 12 inline comments as done.Apr 10 2019, 2:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 10 2019, 2:05 PM
saar.raz marked an inline comment as done.Apr 10 2019, 2:09 PM
saar.raz updated this revision to Diff 194811.Apr 12 2019, 1:19 AM
saar.raz marked 3 inline comments as done.

Moved from getLocStart, getLocEnd to getBeginLoc, getEndLoc. Rebase onto master/trunk.

saar.raz updated this revision to Diff 194978.Apr 12 2019, 4:04 PM

Add new CodeSynthesisContexts to switch where they were missing

saar.raz updated this revision to Diff 194979.Apr 12 2019, 4:06 PM

Fix wrong patch uploaded

saar.raz updated this revision to Diff 204764.Jun 14 2019, 7:13 AM

Add support for CSE mangling

saar.raz updated this revision to Diff 204784.Jun 14 2019, 9:15 AM

Delay support for mangling to later patch (where CSEs are formed instead of UnresolvedLookupExprs with dependent args)

saar.raz updated this revision to Diff 209686.Jul 13 2019, 6:23 AM

Rebase onto trunk.

rsmith marked an inline comment as done.Sep 16 2019, 11:05 AM
rsmith added inline comments.
include/clang/Sema/Sema.h
5582

(Nit: please align the second and subsequent parameters here with the first one.)

7127–7128

It'd be useful to note that this context covers the check that atomic constraints have type bool and can be constant-evaluated. (It's not obvious how we'd hit diagnostics in this case, since most substitution failures result in an unsatisfied constraint.)

lib/AST/ExprCXX.cpp
1471–1472

It'd be nice to assert that the nested name specifier is neither instantiation-dependent nor contains an unexpanded parameter pack. (That's true today because the NNS of a concept specialization expression can only refer to a namespace.) If the NNS could be instantiation-dependent or contain an unexpanded pack, we should inherit those properties here.

1476–1477

This code would be a little clearer if you moved an if (IsDependent && ContainsUnexpandedParameterPack && IsInstantiationDependent) check to the end of the loop. (I doubt the change will ever make any performance difference.)

lib/AST/StmtProfile.cpp
1239–1241

It seems a little inconsistent to use the named concept rather than the found decl (that is, resolved rather than as-written), but to use the as-written template arguments rather than the resolved and converted arguments. (I don't think it matters either way, since the profiler is in principle allowed to use either the as-written or resolved form, but generally it should prefer to use the as-written form.)

lib/Sema/SemaConcept.cpp
30

Nit: indent the second line so that the two CheckConstraintExpression calls line up.

35

OK, I'll take this to the core reflector to make sure the wording is clear this is ill-formed, no diagnostic required.

38–42

I think we're missing this, and we won't reject:

template<typename T> concept bool X = true && (0 && 0);

because we treat (0 && 0) as an atomic constraint rather than treating the two 0 subexpressions as atomic constraints here.

(Maybe add ConstraintExpression = ConstraintExpression->IgnoreParenImpCasts(); or something like that to the start of the function?)

52–69

If an operator&& or operator|| function is declared, this could be a CXXOperatorCallExpr instead. You will need to recurse into those constructs too.

70–72

Consider using IgnoreParenImpCasts here rather than manually handling these cases. (We should be consistent in the mechanism by which we traverse constraint expressions.)

This will do different things particularly for certain language extensions, such as _Generic, __builtin_choose_expr, and __extension (which are treated as being parens-like).

73–75

This case is not handled by CheckConstraintExpression; we should be consistent in whether we step over these or not.

Perhaps it would make sense to factor out a mechanism to take an expression and classify it as atomic constraint (with the corresponding expression), or conjunction (with a pair of subexpressions), or disjunction (with a pair of subexpressions), and use that everywhere we traverse a constraint expression.

lib/Sema/SemaTemplate.cpp
3904–3912

isSatisfied only looks at whether the expression is value-dependent; if it's instantiation-dependent but not value-dependent, then you won't have computed whether it's satisfied but will still permit access to that value. We should be checking whether the arguments are dependent here, not whether they're value-dependent. (And since you need to compute that here anyway, I think it'd make more sense to pass in an "is value-dependent" flag to ConstraintSpecializationExpr::Create.)

lib/Sema/SemaTemplateInstantiate.cpp
522 ↗(On Diff #209686)

Please delete this empty else block (I assume this is left behind from prior changes).

test/CXX/concepts-ts/expr/expr.prim/expr.prim.id/p3.cpp
8–11

These tests at least theoretically depend on the target; please add a -triple x86_64-linux-gnu or whatever to this file so that this test doesn't fail on targets where sizeof(int) is not 4. (I'm not sure we have any such targets right now, but it's not guaranteed.)

saar.raz updated this revision to Diff 224789.Oct 13 2019, 11:11 AM
saar.raz marked 2 inline comments as done.

Address CR comments by rsmith

saar.raz marked 14 inline comments as done and 2 inline comments as done.Oct 13 2019, 11:15 AM
saar.raz added inline comments.
lib/Sema/SemaConcept.cpp
52–69

Atomic constraints will have bool types anyway, can this really happen?

73–75

Since this part will be changed in later patches, I'd like to delay this fix to a later patch for now.

lib/Sema/SemaTemplateInstantiate.cpp
679–681 ↗(On Diff #161343)

In any case, I'm in favor of pushing this to a separate patch (after we have diagnostics for requires exprs, which would suffer from a similar problem)

rsmith accepted this revision.Oct 14 2019, 3:27 PM

Only minor comments, let me know if you'd like me to take another look after addressing these, or just go ahead and commit. Thanks!

clang/lib/AST/ExprCXX.cpp
1676–1677 ↗(On Diff #224789)

Consider passing IsSatisfied and IsValueDependent as a single Optional<bool> (with None meaning value-dependent).

1719–1720 ↗(On Diff #224789)

Please add

assert((!isValueDependent() || isInstantiationDependent()) && "should not be value-dependent");

or similar. If we marked the expression as being value-dependent but it's not instantiation-dependent, things would go badly wrong later (we wouldn't substitute into it during instantiation, for example).

clang/lib/Sema/SemaConcept.cpp
39 ↗(On Diff #224789)

No need for getNonReferenceType() here; expressions never have reference type.

40–41 ↗(On Diff #224789)

I think it'd be a bit better to remove the getUnqualifiedType() call and use hasSameUnqualifiedType instead. That way the diagnostic below will preserve the cv-qualifications (along with any type sugar that you would need to remove to remove the qualifiers).

clang/lib/Serialization/ASTReaderStmt.cpp
737 ↗(On Diff #224789)

Please can you add some basic test coverage for the serialization code?

clang/test/CXX/concepts-ts/expr/expr.prim/expr.prim.id/p3.cpp
1 ↗(On Diff #224789)

Can we remove -fconcepts-ts from the RUN line?

This revision is now accepted and ready to land.Oct 14 2019, 3:27 PM