[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions
Alexander Shaposhnikov via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Apr 13 10:27:59 PDT 2023
alexander-shaposhnikov planned changes to this revision.
alexander-shaposhnikov added inline comments.
================
Comment at: clang/lib/Sema/SemaConcept.cpp:773
+ // ConstrExpr for the inner template will properly adjust the depths.
+ if (isa<CXXRecordDecl>(ND) && isa<CXXRecordDecl>(OtherND))
+ ForConstraintInstantiation = true;
----------------
erichkeane wrote:
> erichkeane wrote:
> > erichkeane wrote:
> > > alexander-shaposhnikov wrote:
> > > > erichkeane wrote:
> > > > > alexander-shaposhnikov wrote:
> > > > > > erichkeane wrote:
> > > > > > > alexander-shaposhnikov wrote:
> > > > > > > > erichkeane wrote:
> > > > > > > > > Hmm... this seems really strange to have to do. `ForConstraintInstantiation` shouldn't be used here, the point of that is to make sure we 'keep looking upward' once we hit a spot we normally stop with. What exactly is the issue that you end up running into here? Perhaps I can spend some time debugging what we should really be doign.
> > > > > > > > yeah, I agree. I haven't found a proper solution or at least a better workaround (but would be happy to).
> > > > > > > > This kicks in for the case
> > > > > > > >
> > > > > > > > ```
> > > > > > > > template <class T0>
> > > > > > > > concept Constraint = true;
> > > > > > > >
> > > > > > > >
> > > > > > > > template<Constraint T1>
> > > > > > > > struct Iterator {
> > > > > > > > template <Constraint T2>
> > > > > > > > friend class Iterator;
> > > > > > > > void operator*();
> > > > > > > > };
> > > > > > > >
> > > > > > > > Iterator<char*> I2;
> > > > > > > > ```
> > > > > > > > yeah, I agree. I haven't found a proper solution or at least a better workaround (but would be happy to).
> > > > > > > > This kicks in for the case
> > > > > > > >
> > > > > > > > ```
> > > > > > > > template <class T0>
> > > > > > > > concept Constraint = true;
> > > > > > > >
> > > > > > > >
> > > > > > > > template<Constraint T1>
> > > > > > > > struct Iterator {
> > > > > > > > template <Constraint T2>
> > > > > > > > friend class Iterator;
> > > > > > > > void operator*();
> > > > > > > > };
> > > > > > > >
> > > > > > > > Iterator<char*> I2;
> > > > > > > > ```
> > > > > > >
> > > > > > > Alright, well, I should have time later in the week to poke at this, perhaps I can come up with something better? I DO remember self-friend is a little wacky, and I spent a bunch of time on it last time.
> > > > > > Ok, sounds good + maybe Richard will give it another look.
> > > > > So IMO, `ForConstraintInstantiation` should be 'true' always, and that makes those examples pass. However, I'm now seeing that it causes a failure in the concepts-out-of-line-def.cpp file.
> > > > >
> > > > > I took the example of `foo3`:
> > > > >
> > > > > ```
> > > > > template<typename T0> concept C = true;
> > > > > template <typename T1>
> > > > > struct S {
> > > > > template <typename F3> requires C<F3>
> > > > > void foo3(F3 f); // #1
> > > > > };
> > > > > template <typename T4>
> > > > > template <typename F6> requires C<F6>
> > > > > void S<T4>::foo3(F6 f) {} // #3
> > > > > ```
> > > > >
> > > > > Which, seems to require `ForConstraintInstantiation` to be false to pass. However, I don't think this is correct. This is only working because when evaluating the in-line one (#1 above!) its skipping the application of `T1`, which is wrong.
> > > > >
> > > > > However, I think the problem here is that the `out of line` version (#3) is not applying the T4 like it should be. SO, I think the `HandleFunctionTemplateDecl` I provided you earlier needs modification.
> > > > >
> > > > > FIRST, though not related to this, I think we might need to add `FunctionTemplateDecl::getInjectedTemplateArgs` to the `Result`, but only because that 'sounds right' to me? IDK what other problem that would cause, but it is worth evaluating/saving for later. It might just not matter, since we're treating them as equal at the moment, I don't think injecting them would cause anything.
> > > > >
> > > > > Secondly: I see that the we can get to the `T4` via the `FTD->getTemplatedDecl()->getQualifier()->getAsType()->getAs<TemplateSpecializationType>()->template_arguments()`.
> > > > >
> > > > > HOWEVER, the problem that comes with picking THOSE up, is that it ALSO applies with a `FunctionTemplateDecl` inside of an out-of-line `ClassTemplateSpecializationDecl` (which doesn't NEED the specialization's template args).
> > > > >
> > > > > SO I think the fix here is something in `HandleFunctionTemplateDecl` to make sure we pick up the right list of template arguments. I haven't figured out the magic incantation to make it work unfortunately, perhaps @rsmith has some hints? Else I'll keep poking at it.
> > > > >
> > > > > BUT, my debugging has shown me that I'm quite convinced the setting `ForConstraintInstantiation ` to false is incorrect.
> > > > p.s. Richard is on vacation.
> > > >
> > > > to quote his comment above (where the story began):
> > > >
> > > > >The right way to fix that and the issue being addressed here is that, rather than adjusting the depths, we ?>should substitute the outer template arguments from the scope specifier (A<int>::) into the constraint before >performing the comparison. (In the special case where none of the outer template parameters are used by the >inner template, that does effectively just adjust the depths of any inner template parameters.)
> > > >
> > > > A. the formal meaning of ForConstraintInstantiation=true/false is unclear, @eirchkean - if you happen to understand it - mind expanding the comment a bit ? (and we can add this to the documentation)
> > > >
> > > > B. <related to A> T1 - maybe it should be collected as a retained layer ?
> > > >
> > > I also notice that the work in `HandleRecordDecl` inside of the `ForConstraintInstantiation` is a little odd, I don't recall why I put that in place, but it DOES result in 5 failed tests if I remove it. Perhaps we just need to be more careful when we substitute in these arguments? It seems to me that putting those arguments in place is perhaps not the right thing to do, since they don't have any meaningful values yet? Perhaps something else for us to poke at.
> > Ah, didn't know he was away :)
> >
> > My understanding is that is what you're doing (the quote). By collecting the instantiation args correctly, that is what should be happening here.
> >
> > A:
> > The `ForConstraintInstantiation`s intent is to make it so we collect arguments 'all the way up' rather than stopping at some points where normal instantiation would. The idea is that a Constraint is never stored in the AST as substituted, so the depth is always relative to the TU. Other situations where we collect arguments is relative to the 'last thing that has been instantiated'. So thats the difference (is where we are substituting 'relative' to).
> >
> > B: At this point, we COULD and probably SHOULD collect T1 as a retained outer layer, but that would still cause a problem with that example. I think the issue here is that we're not ALSO collecting `T4`.
> To answer this question:
>
> ```
> namespace DelayedInst {
> template<unsigned I>
> struct AAA {
> template<typename T>
> requires (sizeof(T) == I)
> struct B {
> static_constexpr int a = 0;
> };
>
> static constexpr auto foo() {
> return B<int>::a;
> }
> };
> ```
> Ends up instantiating `B<int>` to get `a` despite `I` not being filled in yet, but we still try instantiation into the requires clause for various syntactic checking. So we need something to take up the space of the `I`. We cant just do the retained layer, since `AAA` might be inside of ANOTHER class template (perhaps a specialization?), so we'd need that one as well.
>
> So that code goes to try to introduce the `I`. So I think this solution ends up coming down to 'how do we get `T4` into `foo3`'s MLTAL.
Regarding ForConstraintInstantiation=true - I agree,
i think i got confused by a particular example i was looking at.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D146178/new/
https://reviews.llvm.org/D146178
More information about the cfe-commits
mailing list