Page MenuHomePhabricator

[Clang] Make constraints of auto part of NTTP instead of AutoType
AbandonedPublic

Authored by ychen on Sep 27 2022, 3:46 PM.

Details

Reviewers
mizvekov
saar.raz
rsmith
shafik
Group Reviewers
Restricted Project
Summary
Why do this

While working on D128750, it is found that AutoType is not uniqued as expected in the partial ordering. This is because
AutoType contains the constraints for the NTTP and the NTTP itself has a reference to the constraint in AutoType.
Due to this restriction, the partial ordering between template<C auto> and template<D auto> could not be handled the same way
as the partial ordering between template<int> and template<int>. By P2113, they should work the same way and if it doesn't,
the implementation of P2113 would be much harder if at all possible. After this patch, NTTP constraints operate the same way as
TTP constraints, which is to say, constraints are represented separately from the type. This is also easier to understand and maintain.

Note that this patch only changes how constrained auto is represented in NTTP. Nothing is changed when
constrained auto is used as function parameters, as variable declarations, etc.

Implementation strategy
  • Add a TypeConstraint to NonTypeTemplateParmDecl in the same way as TemplateTypeParmDecl
  • During parsing, parse the constraint as a part of NTTP. This also means the constraint inside AutoType would be empty (and later in partial ordering, AutoType uniquing would work due to this).
  • Looking at how the constraint for TemplateTypeParmDecl is handled, add similar/same handling for the constraint for NonTypeTemplateParmDecl.
AST change

For (TTP W is added for comparison purposes.)

template <typename T> concept C = true;
template<C auto V, C W> struct A;

AST before this patch:

`-ClassTemplateDecl 0x9091c60 <line:2:1, col:32> col:32 A
  |-NonTypeTemplateParmDecl 0x9091818 <col:10, col:17> col:17 referenced 'C auto' depth 0 index 0 V    ========> C is part of AutoType
  | `-ConceptSpecializationExpr 0x9091948 <col:10> 'bool' Concept 0x90916e8 'C'
  |   `-TemplateArgument <col:17> type 'decltype(V)'
  |     `-DecltypeType 0x90918e0 'decltype(V)' dependent
  |       `-DeclRefExpr 0x9091878 <col:17> 'C auto' NonTypeTemplateParm 0x9091818 'V' 'C auto'
  |-TemplateTypeParmDecl 0x90919d0 <col:20, col:22> col:22 Concept 0x90916e8 'C' depth 0 index 1 W
  | `-ConceptSpecializationExpr 0x9091b08 <col:20> 'bool' Concept 0x90916e8 'C'
  |   `-TemplateArgument <col:22> type 'W'
  |     `-TemplateTypeParmType 0x9091aa0 'W' dependent depth 0 index 1
  |       `-TemplateTypeParm 0x90919d0 'W'
  `-CXXRecordDecl 0x9091bb0 <col:25, col:32> col:32 struct A

AST after this patch:

`-ClassTemplateDecl 0x887dd40 <line:2:1, col:32> col:32 A
  |-NonTypeTemplateParmDecl 0x887d8b8 <col:12, col:17> col:17 referenced Concept 0x887d788 'C' 'auto' depth 0 index 0 V   ========> C is part of NTTP
  | `-ConceptSpecializationExpr 0x887da28 <col:10> 'bool' Concept 0x887d788 'C'
  |   `-TemplateArgument <col:17> type 'decltype(V)'
  |     `-DecltypeType 0x887d9c0 'decltype(V)' dependent
  |       `-DeclRefExpr 0x887d960 <col:17> 'auto' NonTypeTemplateParm 0x887d8b8 'V' 'auto'
  |-TemplateTypeParmDecl 0x887dab0 <col:20, col:22> col:22 Concept 0x887d788 'C' depth 0 index 1 W
  | `-ConceptSpecializationExpr 0x887dbe8 <col:20> 'bool' Concept 0x887d788 'C'
  |   `-TemplateArgument <col:22> type 'W'
  |     `-TemplateTypeParmType 0x887db80 'W' dependent depth 0 index 1
  |       `-TemplateTypeParm 0x887dab0 'W'
  `-CXXRecordDecl 0x887dc90 <col:25, col:32> col:32 struct A

Diff Detail

Event Timeline

ychen created this revision.Sep 27 2022, 3:46 PM
Herald added a project: Restricted Project. · View Herald Transcript
ychen requested review of this revision.Sep 27 2022, 3:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 27 2022, 3:47 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
ychen added a reviewer: Restricted Project.Sep 27 2022, 3:47 PM
ychen edited the summary of this revision. (Show Details)
ychen edited the summary of this revision. (Show Details)Sep 27 2022, 3:50 PM
ychen edited the summary of this revision. (Show Details)

I'm somewhat skeptical of this approach, because constrained auto can appear in places within the type of an NTTP other than the top level -- for example, in template<C auto*>. (Clang is non-conforming and doesn't support this yet, but it will need to do so eventually.) In principle, there could even be multiple different constrained auto types within the type of the same NTTP in the future -- I don't think that can happen with the current language rules, but it could happen in the Concepts TS, and it only seems to be happenstance that prevents it in C++20 rather than design intent. Fundamentally, the constraint is not associated with the NTTP and is instead associated with the relevant portion of the NTTP's type. If something in the design of constrained auto is meaning that uniquing isn't working properly, maybe there's some way we can address that directly?

I don't think the problem here is an uniquing problem per se, it's just that the constraints are part of the canonical type of undeduced auto, which is undesirable for the NTTP.

Maybe it would work to just add a new flag parameter to ASTContext::getAutoType which makes it not add the constraints to the canonical type?

It is odd that the handling for TTP and NTTP is different here, but perhaps we could move TTP to this solution as well?

ychen added a comment.Sep 27 2022, 5:41 PM

@rsmith, thanks for chiming in.

I'm somewhat skeptical of this approach, because constrained auto can appear in places within the type of an NTTP other than the top level -- for example, in template<C auto*>. (Clang is non-conforming and doesn't support this yet, but it will need to do so eventually.)

Yep, I'm aware of the cases like template<C auto*>/template<C auto&>/template<C decltype(auto)>. It would need to retrieve the deduced type explicitly and the difference this patch brings is to look for the constraints from NTTP instead. It is not geat but doable.

In principle, there could even be multiple different constrained auto types within the type of the same NTTP in the future -- I don't think that can happen with the current language rules, but it could happen in the Concepts TS, and it only seems to be happenstance that prevents it in C++20 rather than design intent. Fundamentally, the constraint is not associated with the NTTP and is instead associated with the relevant portion of the NTTP's type.

I fully agree that fundamentally the constraint is associated with auto (the syntax rule suggests that too). I didn't think of the possibility that multiple different constrained auto types within the type of the same NTTP. It is very interesting. Any chance this approach could be tuned for that future use case? Like maintaining an array of constrains for the NTTP?

If something in the design of constrained auto is meaning that uniquing isn't working properly, maybe there's some way we can address that directly?

I gave a lot of thought to it but couldn't find a satisfactory approach. One way is to recreate injected template argument for constrained auto NTTP, but without the constraint, then use that the instantiate all the types I need for the partial ordering. It is definitely doable but pretty expensive. Any suggestions are greatly appreciated. :-)

ychen added a comment.Sep 27 2022, 5:56 PM

I don't think the problem here is an uniquing problem per se, it's just that the constraints are part of the canonical type of undeduced auto, which is undesirable for the NTTP.

Maybe it would work to just add a new flag parameter to ASTContext::getAutoType which makes it not add the constraints to the canonical type?

Hmm. That sounds promising to me. Let me see if I could get this to work. Thanks.

It is odd that the handling for TTP and NTTP is different here, but perhaps we could move TTP to this solution as well?

Yeah, it is odd from the POV of template handling. From the type hierarchy POV, it is consistent because TTP is a type and auto is also a type. I guess ultimately we choose the one that is easy and extensible for future language features.

ychen abandoned this revision.Oct 5 2022, 10:23 AM

D135088 was landed instead.