Page MenuHomePhabricator

[Concepts] Concept Specialization Expressions
Needs ReviewPublic

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



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

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.

saar.raz added inline comments.Mar 14 2018, 11:21 AM

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

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.


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

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


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

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.


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


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.


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


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


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

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


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


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


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


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.


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

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.


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


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


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


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


Please fix :)

7690 ↗(On Diff #159234)

Please fix.


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.


Isn't the FoundDecl just the NamedConcept?


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


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


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


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

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

Missing a trailing comma here.


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

48 ↗(On Diff #160237)

Should you static-assert the expected results?

saar.raz added inline comments.Aug 11 2018, 12:29 PM

😮😳 Well you learn something every day

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

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


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


p0945r0 would allow them to differ.


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


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


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

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

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.