This is an archive of the discontinued LLVM Phabricator instance.

[Concepts] Type Constraints
ClosedPublic

Authored by saar.raz on Mar 10 2018, 4:13 AM.

Details

Summary

Added support for type-constraints in template type parameters. Depends on D43357.

Diff Detail

Event Timeline

saar.raz created this revision.Mar 10 2018, 4:13 AM
saar.raz updated this revision to Diff 138537.Mar 15 2018, 5:32 AM

Fixed test.

saar.raz updated this revision to Diff 147190.May 16 2018, 3:00 PM

Adjusted to changes in dependent patches.

saar.raz updated this revision to Diff 159378.Aug 6 2018, 1:24 PM

Split TryParseConstrainedParameter and ParseConstrainedTemplateParameter in preparation for requries expressions.

Herald added a reviewer: shafik. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: arphaman. · View Herald Transcript
martong requested changes to this revision.Apr 16 2019, 1:23 AM
martong added inline comments.
lib/AST/ASTImporter.cpp
3708 ↗(On Diff #159378)

Please use the import function instead of VisitExpr. Calling VisitExpr would return immediately with an error, that's the default case of the StmtVisitor.
Also please check that there was no error during the import of the CE expression:

ExpectedExpr CEOrErr = import(D->getConstraintExpression());
if (!CEOrErr)
  return CEOrErr.takeError();
R->setConstraintExpression(*CEOrErr);

And could you please adjust the changes below too?

This revision now requires changes to proceed.Apr 16 2019, 1:23 AM
balazske added inline comments.Apr 16 2019, 3:40 AM
lib/AST/ASTImporter.cpp
3708 ↗(On Diff #159378)

The code above works without the if (Expr *CE = D->getConstraintExpression()) too. The import returns nullptr for a nullptr (*CEOrErr will be nullptr and no error).

saar.raz updated this revision to Diff 195801.Apr 18 2019, 12:32 PM
  • Fixed bad ordering of associated constraints
  • Fixed ignoring of ScopeSpecifier in constrained parameters
  • Adjustments for template template parameter handling
  • Fixed constrained template template parameter and argument handling
rsmith requested changes to this revision.Apr 18 2019, 4:55 PM

This needs revision to reflect changes between the Concepts TS and C++20. In particular, only the name of a type-concept can be used to declare a template-parameter in the new rules:

template<typename> concept A = true;
template<template<typename> typename> B = true;
template<int> concept C = true;

template<A> struct XA {}; // ok
template<B> struct XB {}; // error, not a type-concept
template<C> struct XC {}; // error, not a type-concept

We don't appear to have any explicit representation of the type-concept as written, only of its immediately-declared constraint. One design goal of Clang is for the AST to directly represent the program as written (to the extent possible) to facilitate writing tools on top of Clang's AST. Please consider adding a representation of the TypeConstraint itself. (This could be as simple as a wrapper around the Expr* that you currently have, that provides access to the concept name and the template arguments, and additionally stores a flag to determine whether an explicit template argument list was specified, to distinguish TypeConcept from TypeConcept<>.)

lib/Parse/ParseTemplate.cpp
590–619 ↗(On Diff #195801)

This is out of date compared to the current C++20 wording.

677 ↗(On Diff #195801)

This should not require tentative parsing. We should be able to annotate the nested-name-specifier (if any) then determine whether we have a type-name or a type-constraint with a single token of lookahead.

1039–1045 ↗(On Diff #195801)

This is incorrect; this rule was removed when concepts were merged into C++20.

1153–1176 ↗(On Diff #195801)

All the work to build expressions and establish the semantic effects of declarations should be done by Sema, not by the Parser. The Parser should just figure out how the program matches the grammar, and report to Sema what it found.

This revision now requires changes to proceed.Apr 18 2019, 4:55 PM
saar.raz updated this revision to Diff 196997.Apr 27 2019, 6:27 PM
saar.raz marked 4 inline comments as done.
  • removed old handling of constrained-parameters for type-constraints
  • use template-id annotations during parsing to avoid tentative parses and set the ground for placeholder type-constraints
  • add source information to the new TypeConstraint class, which shares a base class (ConceptReference) with ConceptSpecializationExpr
  • change TemplateTypeParmDecl to not return true for wasDeclaredWithTypename if a type-constraint was present (and update places that assumed only 'class' or 'typename' were possible)
saar.raz marked 2 inline comments as done.Apr 27 2019, 6:27 PM
saar.raz retitled this revision from [Concepts] Constrained template parameters to [Concepts] Type Constraints.Apr 27 2019, 6:28 PM
saar.raz edited the summary of this revision. (Show Details)
saar.raz updated this revision to Diff 203735.Jun 9 2019, 6:49 AM
  • Fixed bad wereArgumentsSpecified() implementation
saar.raz updated this revision to Diff 204786.Jun 14 2019, 9:18 AM

Adjust to changes in previous patches

saar.raz updated this revision to Diff 204789.Jun 14 2019, 9:26 AM

Fix incorrect patch upload

saar.raz updated this revision to Diff 204844.Jun 14 2019, 2:00 PM

Fix incorrect comparison of type-constraints in template template argument matching

saar.raz updated this revision to Diff 209698.Jul 13 2019, 8:42 AM

Rebase onto trunk.

rsmith added inline comments.Jan 8 2020, 5:14 PM
include/clang/AST/ASTNodeTraverser.h
518 ↗(On Diff #209698)

For consistency with DeclRefExpr, I think this function should be called hasExplicitTemplateArgs.

include/clang/AST/RecursiveASTVisitor.h
1781 ↗(On Diff #209698)

We should visit the nested name specifier and concept name too. Perhaps it'd make sense to add a TraverseConceptReference extension point and call it from here and from visitation of a ConceptSpecializationExpr?

include/clang/Basic/DiagnosticSemaKinds.td
2508 ↗(On Diff #209698)

"must not be" -> "is"

2512 ↗(On Diff #209698)

"must be" -> "is not"

lib/AST/ASTContext.cpp
703–707 ↗(On Diff #209698)

Canonicalization should presumably also remove / canonicalize the "as-written" parts here: no NestedNameSpecifierLoc, no template keyword location, the found decl should just be the named concept itself, and the explicit template arguments should be canonicalized. (We don't want the locations and "as-written" parts from the first instance of a particular template template parameter that we happen to encounter to be reachable through later declarations of identical template template parameters.)

709–716 ↗(On Diff #209698)

Similarly, we presumably shouldn't be preserving locations here through canonicalization.

lib/AST/DeclPrinter.cpp
1048–1049 ↗(On Diff #209698)

Nit: I think this would read more naturally if the type constraint case were first.

lib/AST/TextNodeDumper.cpp
1680–1682 ↗(On Diff #209698)

As above, I think this would be clearer if the "type constraint" and "typename" cases were reversed.

1689 ↗(On Diff #209698)

It'd be useful to dump either the explicit arguments or the immediately-declared constraint here.

lib/Parse/ParseExprCXX.cpp
162 ↗(On Diff #209698)

We don't need both of these :)

487–490 ↗(On Diff #209698)

Is this necessary for this change in particular? (I'd expect isTemplateName to be able to cope with this corner case.)

lib/Parse/ParseTemplate.cpp
552 ↗(On Diff #209698)

Have we necessarily annotated a template-id at this point, if necessary? Usually functions that want to look at template-id annotations perform that annotation themselves rather than imposing a side-condition on their caller to do it before calling them.

619–644 ↗(On Diff #209698)

As mentioned above, I'd expect this all to be done by isStartOfTemplateTypeParameter itself.

679–681 ↗(On Diff #209698)

I'd drop the second sentence of this comment. We don't need to talk about how the language works in the parser, just how to parse it.

683–686 ↗(On Diff #209698)

This is out-of-date compared to the C++20 wording.

801–811 ↗(On Diff #209698)

Does this need to be done as two separate steps? (I wonder if combining them might allow us to remove the complexity of having a "has an invalid type constraint" state from TemplateTypeParmDecl.)

1286 ↗(On Diff #209698)

Interesting. So we're using a tok::annot_template_id annotation for this even in the case where we have no template arguments, so it's not actually a template-id? That's a bit weird, but I'm not necessarily opposed if it keeps things simpler. Please at least mention that in the comment on ANNOTATION(template_id) in Basic/TokenKinds.def, and also in the doc comment for class TemplateIdAnnotation.

lib/Sema/SemaCXXScopeSpec.cpp
620 ↗(On Diff #209698)

Nit: && on the previous line, please.

lib/Sema/SemaConcept.cpp
847–849 ↗(On Diff #209698)

Please go ahead and commit this renaming (as a separate change).

lib/Sema/SemaTemplate.cpp
1059 ↗(On Diff #209698)

Fixit hints on an error or warning should be valid C++ code that fixes the problem, and you should recover as if the fix were applied. See http://clang.llvm.org/docs/InternalsManual.html#fix-it-hints for the rules and rationale.

We don't have a way of annotating text on a diagnostic that isn't a fixit hint (although we probably should); for now, let's just omit the hint. (Maybe add a FIXME though?)

1087 ↗(On Diff #209698)

This (and the later references to it) have the wrong section and paragraph listed: this is [temp.param]p4 not [temp]p6.

1089 ↗(On Diff #209698)

Please replace these characters (ellipsis and prime) with ASCII; bad things may happen to these characters when this file is viewed from an editor that defaults to something other than UTF-8 (which is quite likely on Windows).

1123 ↗(On Diff #209698)

Similarly here.

1967–1983 ↗(On Diff #209698)

This will substitute into each of the template arguments twice. It would be better to rebuild the immediately-declared constraint from scratch (and that might even be necessary for correctness in some cases: if a template argument contains a lambda-expression, we should introduce only one closure type, not two -- and will in any case avoid duplicate warnings).

7047 ↗(On Diff #209698)

We shouldn't gate this behind ConceptsTS. Let's just start turning RelaxedTemplateTemplateArgs on unconditionally. (This breaks at least one testcase, but it's not clear that it's a real world problem.)

As a separate change, though :)

lib/Sema/SemaTemplateInstantiateDecl.cpp
2387–2388 ↗(On Diff #209698)

As above, this should be recreated from scratch, not substituted.

saar.raz updated this revision to Diff 237226.Jan 9 2020, 5:26 PM
saar.raz marked 25 inline comments as done.

Rebase onto trunk.
Address CR comments by rsmith.

Addressed all but one comment in recent commit.

saar.raz updated this revision to Diff 237228.Jan 9 2020, 5:31 PM

Remove accidental change to CMakeLists.txt

rsmith added inline comments.Jan 9 2020, 7:19 PM
clang/include/clang/AST/DeclTemplate.h
1297–1300

It'd be more efficient to ask if the immediately-declared constraint is a CXXFoldExpr, but it's a bit ugly to depend on that here. What do you think?

clang/lib/Parse/ParseTemplate.cpp
553

I'm a bit confused by what's going on with this SuppressDiagnostic flag. This isn't a tentative parse; if we don't produce diagnostics for an invalid scope specifier here, I don't see anything that guarantees that any later code will produce any diagnostics either, which would be a problem. I would expect to be able to parse and produce diagnostics normally here, without this SuppressDiagnostic flag.

635

If isStartOfTemplateTypeParameter fails due to an invalid scope specifier, can we just return nullptr immediately, rather than making a likely-ill-fated attempt to parse a template or non-type parameter? It seems unlikely that we can reasonably recover in that case.

696–697

This is a bit surprising: the other bool TryAnnotate* functions follow the normal convention of returning true if they've encountered and diagnosed a parse error (and the caller is expected to check Tok to see whether it was annotated). Please consider making this consistent with the other similar functions.

735–736

I would expect it to fail if the template argument list is malformed, and in that case we should fail too.

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
2484

Nit: please add braces to this outer if.

saar.raz updated this revision to Diff 237288.Jan 10 2020, 5:11 AM
saar.raz marked 2 inline comments as done.

Address some CR comments.

This comment was removed by saar.raz.
saar.raz updated this revision to Diff 237478.Jan 10 2020, 7:38 PM

Remove SuppressDiagnostics, conform TryAnnotateTypeConstraint, readd concepts to relaxed template argument check, fix recovery from bad scope in ParseTemplateParameter

nridge added a subscriber: nridge.Jan 13 2020, 4:15 PM
rsmith accepted this revision.Jan 14 2020, 2:12 PM

Some cleanups (mainly parameters that are no longer necessary), then this is good to go. Thanks!

clang/include/clang/Basic/LangOptions.def
146 ↗(On Diff #237478)

This change has no effect because the default value is unconditionally overwritten in CompilerInvocation.cpp. Revert for now; we can flip this flag as a separate change.

clang/lib/AST/DeclTemplate.cpp
110–114

Do we need something like this for expanded template template parameter packs?

clang/lib/Parse/ParseExprCXX.cpp
2710

This SuppressDiag parameter is never set by any caller; please remove it.

clang/lib/Parse/ParseTemplate.cpp
684

ScopeError is always false here; you can remove the ErrorInTypeSpec parameter and the comment here.

707

The Tok.is check here is redundant; it's a subset of the following Tok.isNot check.

clang/lib/Sema/SemaTemplate.cpp
136

This new parameter appears to be unused now.

7086

This is not appropriate; -fconcepts-ts (or hopefully soon -std=c++2a) should not override the value of this LangOpt.

Please revert this change and manually turn on -frelaxed-template-template-args as necessary in the corresponding test cases.

clang/lib/Sema/SemaTemplateDeduction.cpp
645–659

Looks like we repeat this (at least) three times. (And one of the instances is missing the template template parameter case.) Please consider moving this to somewhere more central (DeclTemplate.h?) -- as a separate patch, though.

This revision was not accepted when it landed; it landed in state Needs Review.Jan 14 2020, 6:10 PM
This revision was automatically updated to reflect the committed changes.
saar.raz marked 12 inline comments as done.
ychen added a subscriber: ychen.Jan 15 2020, 4:00 PM

I saw a null pointer dereference for the p3-2a.cpp file added here. It is not always reproducible for me, I was able to reproduce it once offline. Any idea?

Crash stack is here:
https://results.llvm-merge-guard.org/amd64_debian_testing_clang8-1220/console-log.txt

From D67847, you could see that the first time merge bot is ran, p3-2a.cpp failed. The second time, without me doing anything except updating some unrelated tests, p3-2a.cpp passes.