Page MenuHomePhabricator

[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

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
faisalv added inline comments.Mar 14 2018, 10:31 AM
include/clang/Sema/Sema.h
5765

Hubert: clang-format this

5768

Hubert: Typo: emitted

lib/AST/ExprCXX.cpp
1632

Hubert: clang-format

1669

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.

1676

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
1669

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
1669

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
1669

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
1669

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
4534

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.

4537–4538

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

4573–4577

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.

4588–4595

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

include/clang/Basic/DiagnosticSemaKinds.td
2448

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

2450

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

2452

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

lib/AST/ExprCXX.cpp
1651

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

1743–1744

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
9948–9950

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
2191

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

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
2976–2977

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
4534

Isn't the FoundDecl just the NamedConcept?

4537–4538

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

4588–4595

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

lib/AST/StmtPrinter.cpp
2191

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 ↗(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

😮😳 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
4534

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

lib/AST/ExprCXX.cpp
1663

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
2191

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
4155–4159

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
694–696

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
694–696

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
694–696

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.Fri, Jun 14, 7:13 AM

Add support for CSE mangling

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

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