[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

[Alexander Shaposhnikov via Phabricator via cfe-commits]

alexander-shaposhnikov added a comment.

@shafik - it's a bit early to review this patch, this was the first attempt to fix the issue related to the current behavior of  clang::Sema::TemplateParameterListsAreEqual that causes Clang to mishandle out-of-line definitions involving constraints (and possibly is the root cause of a few other issues related to concepts).
It's still work-in-progress. What's happening is roughly the following: there is a mismatch of depths of types (TemplateTypeParmType) referenced by ConceptSpecializationExpr. 
E.g. for the code

  template <class T>
  concept Concept = true;
  
  template<bool>
  struct a
  {
    template<Concept T1>
    void c(T1&& t);
  };
  
  template<>
  template<Concept T2>
  void a<true>::c(T2&& t)
  {}



  lldb) p OldConstr->dump()
  ConceptSpecializationExpr 0x555564bc4c50 '_Bool' Concept 0x555564bc4608 'Concept'
  |-ImplicitConceptSpecializationDecl 0x555564bc4be0 
  | `-TemplateArgument type 'type-parameter-1-0'
  |   `-TemplateTypeParmType 0x555564bc4b70 'type-parameter-1-0' dependent depth 1 index 0
  `-TemplateArgument type 'T1'
    `-TemplateTypeParmType 0x555564bc4ba0 'T1' dependent depth 1 index 0
      `-TemplateTypeParm 0x555564bc4ac8 'T1'
  
  (lldb) p NewConstr->dump()
  ConceptSpecializationExpr 0x555564bc5140 '_Bool' Concept 0x555564bc4608 'Concept'
  |-ImplicitConceptSpecializationDecl 0x555564bc50d0 
  | `-TemplateArgument type 'type-parameter-0-0'
  |   `-TemplateTypeParmType 0x555564bc4580 'type-parameter-0-0' dependent depth 0 index 0
  `-TemplateArgument type 'T2'
    `-TemplateTypeParmType 0x555564bc5090 'T2' dependent depth 0 index 0
      `-TemplateTypeParm 0x555564bc4ff0 'T2'

Inside Sema::AreConstraintExpressionsEqual there is some logic do adjust the depths, but at the moment i can't claim that i fully understand it - still investigating.
I think @erichkeane has also looked into the issue / debugged what's going on here - if I'm missing something / or something is not right - any corrections would be appreciated.
My current plan is to try to adopt the suggestion from

In D146178#4199382 <https://reviews.llvm.org/D146178#4199382>, @erichkeane wrote:

> In D146178#4199263 <https://reviews.llvm.org/D146178#4199263>, @erichkeane wrote:
>
>> After a night of sleeping on it, the use of a AdjustConstraintDepth::Diff and AdjustConstraintDepth::Value feels like a hacky solution to SOMETHING here, though I'm not sure what yet. The depth of a template shouldn't change between the equality and constraint checking, it is usually a property of the specialization level.  I could definitely believe that `getTemplateInstantiationArgs` needs some sort of change here to better detect its own state, but this solution doesn't seem right to me.
>
> I debugged a bit: It isn't correct I think that `FunctionTemplateDecl` doesn't pick up its template arguments in `getTemplateInstantiationArgs`.  Additionally, instead of picking up its `DeclContext`, it probably needs its `LexicalDeclContext` as the next step, which I believe fixes the problem (plus your change in SemaOverload.cpp)
>
> EDIT: `FunctionTemplateDecl` doesn't pick up its template arguments on purpose: there ARE no template arguments, so that is me being silly.  However, the difference here is that it isn't picking its lexical decl context here.  There is likely a similar solution here for VarTemplateDecl.
>
> I'm leaning toward the solution here being in the CalculateTemplateDepth here instead: Recognize those two types, and set the correct DeclContext to pass to the getTemplateInstantiationArgs (that is, the lexical decl context).

and see if i can make it functional, this will take some time.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146178/new/

https://reviews.llvm.org/D146178