Page MenuHomePhabricator

[Concepts] Concept Specialization Expressions
ClosedPublic

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

Details

Reviewers
rsmith
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.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
rsmith added inline comments.Aug 6 2018, 5:34 PM
lib/Sema/SemaExprCXX.cpp
7690 ↗(On Diff #159234)

Please fix.

lib/Sema/TreeTransform.h
2943–2956 ↗(On Diff #159234)

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

Isn't the FoundDecl just the NamedConcept?

4423–4424 ↗(On Diff #159234)

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

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

lib/AST/StmtPrinter.cpp
2553 ↗(On Diff #159234)

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

lib/Sema/SemaConcept.cpp
34 ↗(On Diff #159234)

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

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

Missing a trailing comma here.

test/CXX/concepts-ts/expr/expr.prim/expr.prim.id/p3.cpp
121 ↗(On Diff #160237)

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

test/Parser/cxx-concept-declaration.cpp
48 ↗(On Diff #160237)

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

😮😳 Well you learn something every day

test/Parser/cxx-concept-declaration.cpp
48 ↗(On Diff #160237)

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

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

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

lib/AST/ExprCXX.cpp
1472 ↗(On Diff #161343)

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

p0945r0 would allow them to differ.

lib/Sema/SemaConcept.cpp
105–112 ↗(On Diff #161343)

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

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.Mon, Sep 16, 11:05 AM
rsmith added inline comments.
include/clang/Sema/Sema.h
5904 ↗(On Diff #209686)

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

7487–7488 ↗(On Diff #209686)

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
1703–1704 ↗(On Diff #209686)

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.

1708–1709 ↗(On Diff #209686)

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
1303–1305 ↗(On Diff #209686)

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

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

37–41 ↗(On Diff #159234)

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

29 ↗(On Diff #209686)

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

51–68 ↗(On Diff #209686)

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

69–71 ↗(On Diff #209686)

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

72–74 ↗(On Diff #209686)

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
4256–4264 ↗(On Diff #209686)

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

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.Sun, Oct 13, 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.Sun, Oct 13, 11:15 AM
saar.raz added inline comments.
lib/Sema/SemaConcept.cpp
51–68 ↗(On Diff #209686)

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

72–74 ↗(On Diff #209686)

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.Mon, Oct 14, 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

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

1719–1720

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

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

40–41

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

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

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

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