Quantcast

Re: Next steps for D24361 (patch to fix hasDeclaration wrt ElaboratedType)

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Next steps for D24361 (patch to fix hasDeclaration wrt ElaboratedType)

Ivan Garramona via cfe-dev
Now with the right cfe-dev

On Fri, Oct 28, 2016 at 4:14 PM Manuel Klimek <[hidden email]> wrote:
Somehow we lost cfe-dev on this mail thread :( :(

adding Aaron, who has also contributed to that part of the code

On Fri, Oct 28, 2016 at 4:11 PM Manuel Klimek <[hidden email]> wrote:
On Fri, Oct 28, 2016 at 3:54 PM Łukasz Anforowicz <[hidden email]> wrote:
Some notes from me (that hopefully will helpful to consider during the meeting):
  • I agree that forcing hasDeclaration to consider all subclasses of Types (i.e. forcing via #include "clang/AST/TypeNodes.def" in the proposal below) seems to be a good thing.  The subset of types supported today seems to be rather arbitrary (I think;  please let me know if there is good reasoning + a way to describe the currently chosen subset of supported types).
  • As a user of hasDeclaration, I have to "manually" drill through *some* can paints today.  For example in https://codereview.chromium.org/2256913002/patch/180001/190001 I am manually jumping over pointers and references (what I want to do is to tell whether a member dereference will end up using one of "my" classdecls where I am renaming methods and fields) - see |blink_qual_type_matcher| in that Chromium code review.  Manually stripping pointers and references is doable but 1) it is easy to get wrong (i.e. strip only one level) and 2) it is questionable whether other required stripping is not missed (i.e. auto type?),
  • Today hasDeclaration doesn't match "the outermost can in our russian doll" as suggested (or proposed) by Manual.  Today desugaring is done in an arbitrary order of types - some (deep) types will be preferred over other (shallow) types as shown in an example in the separate doc.  In the example, even though the typeAliasDecl is the most shallow desugaring result, it will *never* be matched because hasDeclaration will first try (and latch onto) other types first.
  • "has" vs "hasDescendant" is not a perfectly valid analogy.  "hasDeclaration" is not like "has" today, because even today it can strip multiple layers of sugar (see the example from the previous bullet item).
So:
  • I would vote to make "hasDeclaration" match at every desugaring level (i.e. without splitting it into a "has" and a "hasDescendant"-like variant).  Having "hasDeclaration" and "eventuallyDesugarsToDeclaration" ("hasDeepDeclaration"?) sounds like an okay option, but it seems to me that even today 1) hasDeclaration code and 2) users of hasDeclaration, behave like or expect behavior of "eventuallyDesugarsToDeclaration"
I'm still looking into where we made the fundamental error that we're running into those discussions now. There should be something simpler so that the behavior more clearly matches the design of the clang AST.
  • I hope that the discussion of "hasDeclaration" vs "hasDeepDeclaration" can be independent from, and not block the fixes from https://reviews.llvm.org/D24361 (i.e. being able to land them in the next 1-2 would be great). 
As Richard noted on the doc, it really depends on which direction we want to go in, and whether the patch is a step in the right direction. Sorry :(
But please continue pinging / making sure we drive this forward - I think this is important, and I'm happy to spend some time to make sure we get this right, and thanks for all the work you've already put into this, I really appreciate it, and I think it's helping us getting to the root of the problem.
 

On Thu, Oct 27, 2016 at 1:12 AM, Manuel Klimek <[hidden email]> wrote:
+ben & sam

On Wed, Oct 26, 2016 at 6:50 PM Łukasz Anforowicz <[hidden email]> wrote:
On Wed, Oct 26, 2016 at 9:46 AM, Łukasz Anforowicz <[hidden email]> wrote:
So - is it fair to say that the CL under review can proceed as-is (*) given that:
  • The ordering problem has existed *before* the CL under review (admittedly the CL under review adds 2 more cases to the list of problematic ordering scenarios)
  • Below it seems that we have a promising direction for solving the ordering problem (the solution probably requires a slightly bigger CL - one that also tackles the question of what hasDeclaration should do about AutoType, PointerType, etc.)
-Lukasz

(*) I know that I need to add documentation changes to mention ElaboratedType

On Tue, Oct 25, 2016 at 5:02 PM, Richard Smith <[hidden email]> wrote:
On 25 October 2016 at 16:47, Łukasz Anforowicz <[hidden email]> wrote:
I think I understand the concern - the concern is that desugaring done in HasDeclarationMatcher::matchesSpecialized(const QualType&, ...) *latches* onto desugared types in *arbitrary* order.  I agree that this is a problem (most obvious for me for the typeAliasDecl and classTemplateSpecializationDecl case).  I agree that the CL adds a new case to the set of ordered desugaring attempts done by this method (and therefore can be seen as making the old problem slightly worse).  

I think this problem can be fixed by getting rid of the *order* inside HasDeclarationMatcher::matchesSpecialized(const QualType&, ...) - instead of:

if (auto *TD = Node->getAsTagDecl())
  return matchesDecl(TD, Finder, Builder);
else if (auto *TT = Node->getAs<TypedefType>())
  return matchesDecl(TT->getDecl(), Finder, Builder);
/* the CL under review would add ElaboratedType and
   TemplateSpecializationType somewhere here */
...
else if (auto *ICNT = Node->getAs<InjectedClassNameType>())
  return matchesDecl(ICNT->getDecl(), Finder, Builder);
return false;

have something like this:

    if (auto *TD = Node->getAsTagDecl()) {
      if (matchesDecl(TD, Finder, Builder))
        return true;
    }

    if (auto *TT = Node->getAs<TypedefType>()) {
      if (matchesDecl(TT->getDecl(), Finder, Builder))
        return true;
    }

    if (auto *ET = Node->getAs<ElaboratedType>()) {
      if (matchesSpecialized(ET->getNamedType(), Finder, Builder))
        return true;
    }

    if (auto *TST = Node->getAs<TemplateSpecializationType>()) {
      if (matchesSpecialized(*TST, Finder, Builder))
        return true;
    }
...
    if (auto *ICNT = Node->getAs<InjectedClassNameType>()) {
      if (matchesDecl(ICNT->getDecl(), Finder, Builder))
        return true;
    }

    return false;

I think this is an interesting idea. It would certainly address my concerns. (You'd actually want something slightly different from the above, I think: given

  typedef int A;
  typedef A B;
  B x;

... varDecl(hasType(qualType(hasDeclaration(typedefNameDecl().bind("TD"))))) should match x /twice/: once with TD pointing at A and once with it pointing at B. So you probably want to repeatedly single-step desugar the type and try extracting a declaration from each level of type sugar.)

Oh, good point - I haven't thought about this scenario.  I think this direction (based on getAsSuger from Type.cpp) look promising (passes a test with 2 chained typedefs):

template <typename T, typename DeclMatcherT>
class HasDeclarationMatcher : public WrapperMatcherInterface<T> {
...
  /// \brief Extracts the TagDecl of a QualType and returns whether the inner
  /// matcher matches on it.
  bool matchesSpecialized(const QualType &Node, ASTMatchFinder *Finder,
                          BoundNodesTreeBuilder *Builder) const {
    if (Node.isNull())
      return false;

    // Do not use getAs<TemplateTypeParmType> instead of the direct dyn_cast.
    // Calling getAs will return the canonical type, but that type does not
    // store a TemplateTypeParmDecl. We *need* the uncanonical type, if it is
    // available, and using dyn_cast ensures that.
    if (auto *TTP = dyn_cast<TemplateTypeParmType>(Node.getTypePtr())) {
      if (matchesDecl(TTP->getDecl(), Finder, Builder))
        return true;
    }

    const Type* Cur = Node.getTypePtr();
    while (Cur) {
      switch (Cur->getTypeClass()) {
#define ABSTRACT_TYPE(Class, Parent)
#define TYPE(Class, Parent) \
      case Type::Class: { \
        const Class##Type *Ty = cast<Class##Type>(Cur); \
        if (matchesSpecialized(*Ty, Finder, Builder)) \
          return true; \
        if (!Ty->isSugared()) return false; \
        Cur = Ty->desugar().getTypePtr(); \
        break; \
      }
#include "clang/AST/TypeNodes.def"
      }
    }

    return false;
  }

BTW: I have to note that the code above is potentially O(N^2) where N is the length of the desugaring chain (because each desugaring step can potentially call back into the QualType overload).  This is obviously suboptimal.  We probably need to discuss this when we review / talk about landing something like this.

It's basically the same difference between has() and hasDescendant.
I think
a) if we go that route, we should make it explicit by having a matcher that has it in its name
b) we need to be able to memoize at each step, so this needs to become part of the visitor interface of the matchers; given the sheer number of types this seems like it'll get rather complex

I'll make sure to discuss this with Richard etc al in person next week when we're at LLVM conf.
 
 
but it opens a whole bunch of questions - what to do with other types - for example should hasDeclaration "jump over / drill holes in paint cans" for:
  • AutoType, TypeOfExprType, DecltypeType, TypeOfType 
  • LValueReferenceType, ValueReferenceType
  • PointerType, ReferenceType
  • And all the other types where I had to add a matchSpecialized overload for so that things compile (some of these obviously don't have a decl [and therefore shouldn't match hasDeclaration(...)] - like BuiltinType, but I am listing all of them for completeness): AtomicType, PipeType, ObjCObjectPointerType, ObjCObjectType, PackExpansionType, DependentTemplateSpecializationType, DependentNameType, AttributedType, UnaryTransformType, SubstTemplateTypeParmPackType, SubstTemplateTypeParmType, AdjustedType, DecayedType, ParenType, FunctionNoProtoType, FunctionProtoType, ExtVectorType, VectorType, DependentSizedExtVectorType, DependentSizedArrayType, VariableArrayType, IncompleteArrayType, ConstantArrayType, MemberPointerType, BlockPointerType, ComplexType, BuiltinType, 
Also - I am not quite sure if I understand if special-casing of TemplateTypeParmType is needed.  It probably is (because here we use dyn_cast rather than cast).  At any rate drilling into details here can probably wait until a real review in Phabricator...

 
The code above passes all tests from the clang-test ninja target + results in the following behavior for the 2 cases pointed out in an email:

TEST(HasDeclaration, Case1FromEmail) {
  std::string input =
      "template<typename T>\n"
      "class Template {\n"
      " public:\n"
      "  void Method() {}\n"
      "};\n"
      "template <typename U>\n"
      "void Function(Template<U> param) {\n"
      "  param.Method();\n"
      "};\n";

  // param type has-declaration matches class template decl?
  EXPECT_TRUE(matches(input, parmVarDecl(
      hasName("param"),
      hasType(qualType(
          hasDeclaration(decl(classTemplateDecl(hasName("Template")))))))));

  // param type has-declaration matches class decl?
  EXPECT_FALSE(matches(input, parmVarDecl(
      hasName("param"),
      hasType(qualType(
          hasDeclaration(decl(cxxRecordDecl(hasName("Template")))))))));
}

TEST(HasDeclaration, Case2FromEmail) {
  std::string input =
      "template<typename T>\n"
      "class Template {\n"
      " public:\n"
      "  void Method() {}\n"
      "};\n"
      "void Function(Template<int> param) {\n"
      "  param.Method();\n"
      "};\n";

  // param type has-declaration matches class template decl?
  EXPECT_TRUE(matches(input, parmVarDecl(
      hasName("param"),
      hasType(qualType(
          hasDeclaration(decl(classTemplateDecl(hasName("Template")))))))));

  // param type has-declaration matches class decl?
  EXPECT_TRUE(matches(input, parmVarDecl(
      hasName("param"),
      hasType(qualType(
          hasDeclaration(decl(cxxRecordDecl(hasName("Template")))))))));
}

If the test expectations above look reasonable, then we can just discuss how to land the code changes above (e.g. as a single CL or 2 CLs - I would prefer 2 separate CLs).  Otherwise, let's talk about the test expectations - what should they look like / how should they be different from the test snippet above?


On Tue, Oct 25, 2016 at 1:42 PM, Richard Smith <[hidden email]> wrote:
On 25 October 2016 at 01:57, Manuel Klimek <[hidden email]> wrote:
On Tue, Oct 25, 2016 at 3:25 AM Richard Smith <[hidden email]> wrote:
On 24 October 2016 at 16:41, Łukasz Anforowicz <[hidden email]> wrote:
I am not sure if we are making progress, but so far, we have seen 1) test cases that are broken and fixed by the CL under review and 2) test cases that are not changed by the CL under review and 3) no test cases that are made worse by the CL under review.  I've tried to gather the scenarios and the associated test cases in https://docs.google.com/document/d/1y1F6uKdulluYBFch680g6bmHe_zDe8mHFYjfXRoakNI/edit.  Could you please take a look and shout if I misrepresented anything (i.e. whether we agree that a specific test scenario shows a bug).

I've also added some comments below / inline.

On Mon, Oct 24, 2016 at 3:54 PM, Richard Smith <[hidden email]> wrote:
On 24 October 2016 at 15:25, Łukasz Anforowicz <[hidden email]> wrote:
On Mon, Oct 24, 2016 at 2:21 PM, Richard Smith <[hidden email]> wrote:
On 24 October 2016 at 11:23, Łukasz Anforowicz <[hidden email]> wrote:
I've run a few more experiments and I see that before any changes:
  • (1) Presence of ElaboratedType means that hasDeclaration doesn’t match *anything*.  This is clearly wrong.  I still want to fix that.
  • (2) Both examples from Richard’s email consistently match the class template decl (and not the class decl).  If Richard’s expectations are not met *prior* to my CL, then I don’t think these expectations should be fixed by my CL.
    • OTOH, maybe it is worth to open a bug to investigate this more - personally I would argue that hasDeclaration should match *any* decl - for example any of type alias / template class decl / class decl.
  • (3) If desugaring can result in both TemplateSpecializationType and TypedefType, then only one is arbitrarily picked - see test case TypedefTypeAndTemplateSpecializationTypeAreBothHere.  Again - this problem exists *prior* to my CL.
So - my plan right now is to just simplify/revert my patches so they only fix (1).  I don't think discussion of (2) should impact the CL, because the behavior that disagrees with Richard's expectations is happening *before* my changes.  I will also not attempt to fix (3), because this starts to interfere with (2) - sorry :-/.

QUESTION: Does the plan from the previous paragraph sound good to you?

Yes; I agree that failing to look through ElaboratedType here is just a bug.

Ack.  I've marked "elaborated type" section in the doc as something that is improved by the CL under review.
 
Experiments before my changes were done with the following unit tests:

// This testcase shows that Richard’s expectations are not met *prior* to my CL.
// (i.e. the testcase below passes before my CL;  the expectations in the testcase
// are the opposite of Richard’s expectations).
TEST(HasDeclaration, Case1FromEmail) {
  std::string input =
      "template<typename T>\n"
      "class Template {\n"
      " public:\n"
      "  void Method() {}\n"
      "};\n"
      "template <typename U>\n"
      "void Function(Template<U> param) {\n"
      "  param.Method();\n"
      "};\n";


  // param type has-declaration matches class template decl?
  EXPECT_TRUE(matches(input, parmVarDecl(
      hasName("param"),
      hasType(templateSpecializationType(
          hasDeclaration(decl(classTemplateDecl(hasName("Template")))))))));

I don't think we're talking about the same thing. It seems fine, and appropriate, for hasType(templateSpecializationType(hasDeclaration(...))) to match the template. There's nothing else that hasDeclaration could reasonably mean as a TemplateSpecializationType matcher (ignoring "interesting" cases with alias templates).

Hmm...  To me not matching the cxxRecordDecl is not that obvious, but this is not important - the more important thing is to figure out what exactly (if anything) is made worse by the CL under review.

My concern is entirely about hasType(hasDeclaration(...)), /without/ the intervening match of a TemplateSpecializationType, which -- unless I've completely misunderstood the patch -- is what you're changing. I would expect that matcher to not match here...


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Next steps for D24361 (patch to fix hasDeclaration wrt ElaboratedType)

Ivan Garramona via cfe-dev
Playing around with the code, we have 2 problems we need to solve:
a) the confusion between type & qualType weirdness
That got in by accident, I think, though the getDecl() specialization that behaves differently from the partial desugaring we have due to b)
b) the partial desugaring we do
This basically was born from the idea that some of the sugaring classes we have are really really weird for devs (like TemplateSpecializationType) or basically useless to match (like ElaboratedType, where we can match on the NNS if we need to).

a) seems fixable by a more fundamental patch in HasDeclarationMatcher on which I'm working
b) seems like something we should discuss

On Fri, Oct 28, 2016 at 4:27 PM Manuel Klimek <[hidden email]> wrote:
Now with the right cfe-dev


On Fri, Oct 28, 2016 at 4:14 PM Manuel Klimek <[hidden email]> wrote:
Somehow we lost cfe-dev on this mail thread :( :(

adding Aaron, who has also contributed to that part of the code

On Fri, Oct 28, 2016 at 4:11 PM Manuel Klimek <[hidden email]> wrote:
On Fri, Oct 28, 2016 at 3:54 PM Łukasz Anforowicz <[hidden email]> wrote:
Some notes from me (that hopefully will helpful to consider during the meeting):
  • I agree that forcing hasDeclaration to consider all subclasses of Types (i.e. forcing via #include "clang/AST/TypeNodes.def" in the proposal below) seems to be a good thing.  The subset of types supported today seems to be rather arbitrary (I think;  please let me know if there is good reasoning + a way to describe the currently chosen subset of supported types).
  • As a user of hasDeclaration, I have to "manually" drill through *some* can paints today.  For example in https://codereview.chromium.org/2256913002/patch/180001/190001 I am manually jumping over pointers and references (what I want to do is to tell whether a member dereference will end up using one of "my" classdecls where I am renaming methods and fields) - see |blink_qual_type_matcher| in that Chromium code review.  Manually stripping pointers and references is doable but 1) it is easy to get wrong (i.e. strip only one level) and 2) it is questionable whether other required stripping is not missed (i.e. auto type?),
  • Today hasDeclaration doesn't match "the outermost can in our russian doll" as suggested (or proposed) by Manual.  Today desugaring is done in an arbitrary order of types - some (deep) types will be preferred over other (shallow) types as shown in an example in the separate doc.  In the example, even though the typeAliasDecl is the most shallow desugaring result, it will *never* be matched because hasDeclaration will first try (and latch onto) other types first.
  • "has" vs "hasDescendant" is not a perfectly valid analogy.  "hasDeclaration" is not like "has" today, because even today it can strip multiple layers of sugar (see the example from the previous bullet item).
So:
  • I would vote to make "hasDeclaration" match at every desugaring level (i.e. without splitting it into a "has" and a "hasDescendant"-like variant).  Having "hasDeclaration" and "eventuallyDesugarsToDeclaration" ("hasDeepDeclaration"?) sounds like an okay option, but it seems to me that even today 1) hasDeclaration code and 2) users of hasDeclaration, behave like or expect behavior of "eventuallyDesugarsToDeclaration"
I'm still looking into where we made the fundamental error that we're running into those discussions now. There should be something simpler so that the behavior more clearly matches the design of the clang AST.
  • I hope that the discussion of "hasDeclaration" vs "hasDeepDeclaration" can be independent from, and not block the fixes from https://reviews.llvm.org/D24361 (i.e. being able to land them in the next 1-2 would be great). 
As Richard noted on the doc, it really depends on which direction we want to go in, and whether the patch is a step in the right direction. Sorry :(
But please continue pinging / making sure we drive this forward - I think this is important, and I'm happy to spend some time to make sure we get this right, and thanks for all the work you've already put into this, I really appreciate it, and I think it's helping us getting to the root of the problem.
 

On Thu, Oct 27, 2016 at 1:12 AM, Manuel Klimek <[hidden email]> wrote:
+ben & sam

On Wed, Oct 26, 2016 at 6:50 PM Łukasz Anforowicz <[hidden email]> wrote:
On Wed, Oct 26, 2016 at 9:46 AM, Łukasz Anforowicz <[hidden email]> wrote:
So - is it fair to say that the CL under review can proceed as-is (*) given that:
  • The ordering problem has existed *before* the CL under review (admittedly the CL under review adds 2 more cases to the list of problematic ordering scenarios)
  • Below it seems that we have a promising direction for solving the ordering problem (the solution probably requires a slightly bigger CL - one that also tackles the question of what hasDeclaration should do about AutoType, PointerType, etc.)
-Lukasz

(*) I know that I need to add documentation changes to mention ElaboratedType

On Tue, Oct 25, 2016 at 5:02 PM, Richard Smith <[hidden email]> wrote:
On 25 October 2016 at 16:47, Łukasz Anforowicz <[hidden email]> wrote:
I think I understand the concern - the concern is that desugaring done in HasDeclarationMatcher::matchesSpecialized(const QualType&, ...) *latches* onto desugared types in *arbitrary* order.  I agree that this is a problem (most obvious for me for the typeAliasDecl and classTemplateSpecializationDecl case).  I agree that the CL adds a new case to the set of ordered desugaring attempts done by this method (and therefore can be seen as making the old problem slightly worse).  

I think this problem can be fixed by getting rid of the *order* inside HasDeclarationMatcher::matchesSpecialized(const QualType&, ...) - instead of:

if (auto *TD = Node->getAsTagDecl())
  return matchesDecl(TD, Finder, Builder);
else if (auto *TT = Node->getAs<TypedefType>())
  return matchesDecl(TT->getDecl(), Finder, Builder);
/* the CL under review would add ElaboratedType and
   TemplateSpecializationType somewhere here */
...
else if (auto *ICNT = Node->getAs<InjectedClassNameType>())
  return matchesDecl(ICNT->getDecl(), Finder, Builder);
return false;

have something like this:

    if (auto *TD = Node->getAsTagDecl()) {
      if (matchesDecl(TD, Finder, Builder))
        return true;
    }

    if (auto *TT = Node->getAs<TypedefType>()) {
      if (matchesDecl(TT->getDecl(), Finder, Builder))
        return true;
    }

    if (auto *ET = Node->getAs<ElaboratedType>()) {
      if (matchesSpecialized(ET->getNamedType(), Finder, Builder))
        return true;
    }

    if (auto *TST = Node->getAs<TemplateSpecializationType>()) {
      if (matchesSpecialized(*TST, Finder, Builder))
        return true;
    }
...
    if (auto *ICNT = Node->getAs<InjectedClassNameType>()) {
      if (matchesDecl(ICNT->getDecl(), Finder, Builder))
        return true;
    }

    return false;

I think this is an interesting idea. It would certainly address my concerns. (You'd actually want something slightly different from the above, I think: given

  typedef int A;
  typedef A B;
  B x;

... varDecl(hasType(qualType(hasDeclaration(typedefNameDecl().bind("TD"))))) should match x /twice/: once with TD pointing at A and once with it pointing at B. So you probably want to repeatedly single-step desugar the type and try extracting a declaration from each level of type sugar.)

Oh, good point - I haven't thought about this scenario.  I think this direction (based on getAsSuger from Type.cpp) look promising (passes a test with 2 chained typedefs):

template <typename T, typename DeclMatcherT>
class HasDeclarationMatcher : public WrapperMatcherInterface<T> {
...
  /// \brief Extracts the TagDecl of a QualType and returns whether the inner
  /// matcher matches on it.
  bool matchesSpecialized(const QualType &Node, ASTMatchFinder *Finder,
                          BoundNodesTreeBuilder *Builder) const {
    if (Node.isNull())
      return false;

    // Do not use getAs<TemplateTypeParmType> instead of the direct dyn_cast.
    // Calling getAs will return the canonical type, but that type does not
    // store a TemplateTypeParmDecl. We *need* the uncanonical type, if it is
    // available, and using dyn_cast ensures that.
    if (auto *TTP = dyn_cast<TemplateTypeParmType>(Node.getTypePtr())) {
      if (matchesDecl(TTP->getDecl(), Finder, Builder))
        return true;
    }

    const Type* Cur = Node.getTypePtr();
    while (Cur) {
      switch (Cur->getTypeClass()) {
#define ABSTRACT_TYPE(Class, Parent)
#define TYPE(Class, Parent) \
      case Type::Class: { \
        const Class##Type *Ty = cast<Class##Type>(Cur); \
        if (matchesSpecialized(*Ty, Finder, Builder)) \
          return true; \
        if (!Ty->isSugared()) return false; \
        Cur = Ty->desugar().getTypePtr(); \
        break; \
      }
#include "clang/AST/TypeNodes.def"
      }
    }

    return false;
  }

BTW: I have to note that the code above is potentially O(N^2) where N is the length of the desugaring chain (because each desugaring step can potentially call back into the QualType overload).  This is obviously suboptimal.  We probably need to discuss this when we review / talk about landing something like this.

It's basically the same difference between has() and hasDescendant.
I think
a) if we go that route, we should make it explicit by having a matcher that has it in its name
b) we need to be able to memoize at each step, so this needs to become part of the visitor interface of the matchers; given the sheer number of types this seems like it'll get rather complex

I'll make sure to discuss this with Richard etc al in person next week when we're at LLVM conf.
 
 
but it opens a whole bunch of questions - what to do with other types - for example should hasDeclaration "jump over / drill holes in paint cans" for:
  • AutoType, TypeOfExprType, DecltypeType, TypeOfType 
  • LValueReferenceType, ValueReferenceType
  • PointerType, ReferenceType
  • And all the other types where I had to add a matchSpecialized overload for so that things compile (some of these obviously don't have a decl [and therefore shouldn't match hasDeclaration(...)] - like BuiltinType, but I am listing all of them for completeness): AtomicType, PipeType, ObjCObjectPointerType, ObjCObjectType, PackExpansionType, DependentTemplateSpecializationType, DependentNameType, AttributedType, UnaryTransformType, SubstTemplateTypeParmPackType, SubstTemplateTypeParmType, AdjustedType, DecayedType, ParenType, FunctionNoProtoType, FunctionProtoType, ExtVectorType, VectorType, DependentSizedExtVectorType, DependentSizedArrayType, VariableArrayType, IncompleteArrayType, ConstantArrayType, MemberPointerType, BlockPointerType, ComplexType, BuiltinType, 
Also - I am not quite sure if I understand if special-casing of TemplateTypeParmType is needed.  It probably is (because here we use dyn_cast rather than cast).  At any rate drilling into details here can probably wait until a real review in Phabricator...

 
The code above passes all tests from the clang-test ninja target + results in the following behavior for the 2 cases pointed out in an email:

TEST(HasDeclaration, Case1FromEmail) {
  std::string input =
      "template<typename T>\n"
      "class Template {\n"
      " public:\n"
      "  void Method() {}\n"
      "};\n"
      "template <typename U>\n"
      "void Function(Template<U> param) {\n"
      "  param.Method();\n"
      "};\n";

  // param type has-declaration matches class template decl?
  EXPECT_TRUE(matches(input, parmVarDecl(
      hasName("param"),
      hasType(qualType(
          hasDeclaration(decl(classTemplateDecl(hasName("Template")))))))));

  // param type has-declaration matches class decl?
  EXPECT_FALSE(matches(input, parmVarDecl(
      hasName("param"),
      hasType(qualType(
          hasDeclaration(decl(cxxRecordDecl(hasName("Template")))))))));
}

TEST(HasDeclaration, Case2FromEmail) {
  std::string input =
      "template<typename T>\n"
      "class Template {\n"
      " public:\n"
      "  void Method() {}\n"
      "};\n"
      "void Function(Template<int> param) {\n"
      "  param.Method();\n"
      "};\n";

  // param type has-declaration matches class template decl?
  EXPECT_TRUE(matches(input, parmVarDecl(
      hasName("param"),
      hasType(qualType(
          hasDeclaration(decl(classTemplateDecl(hasName("Template")))))))));

  // param type has-declaration matches class decl?
  EXPECT_TRUE(matches(input, parmVarDecl(
      hasName("param"),
      hasType(qualType(
          hasDeclaration(decl(cxxRecordDecl(hasName("Template")))))))));
}

If the test expectations above look reasonable, then we can just discuss how to land the code changes above (e.g. as a single CL or 2 CLs - I would prefer 2 separate CLs).  Otherwise, let's talk about the test expectations - what should they look like / how should they be different from the test snippet above?


On Tue, Oct 25, 2016 at 1:42 PM, Richard Smith <[hidden email]> wrote:
On 25 October 2016 at 01:57, Manuel Klimek <[hidden email]> wrote:
On Tue, Oct 25, 2016 at 3:25 AM Richard Smith <[hidden email]> wrote:
On 24 October 2016 at 16:41, Łukasz Anforowicz <[hidden email]> wrote:
I am not sure if we are making progress, but so far, we have seen 1) test cases that are broken and fixed by the CL under review and 2) test cases that are not changed by the CL under review and 3) no test cases that are made worse by the CL under review.  I've tried to gather the scenarios and the associated test cases in https://docs.google.com/document/d/1y1F6uKdulluYBFch680g6bmHe_zDe8mHFYjfXRoakNI/edit.  Could you please take a look and shout if I misrepresented anything (i.e. whether we agree that a specific test scenario shows a bug).

I've also added some comments below / inline.

On Mon, Oct 24, 2016 at 3:54 PM, Richard Smith <[hidden email]> wrote:
On 24 October 2016 at 15:25, Łukasz Anforowicz <[hidden email]> wrote:
On Mon, Oct 24, 2016 at 2:21 PM, Richard Smith <[hidden email]> wrote:
On 24 October 2016 at 11:23, Łukasz Anforowicz <[hidden email]> wrote:
I've run a few more experiments and I see that before any changes:
  • (1) Presence of ElaboratedType means that hasDeclaration doesn’t match *anything*.  This is clearly wrong.  I still want to fix that.
  • (2) Both examples from Richard’s email consistently match the class template decl (and not the class decl).  If Richard’s expectations are not met *prior* to my CL, then I don’t think these expectations should be fixed by my CL.
    • OTOH, maybe it is worth to open a bug to investigate this more - personally I would argue that hasDeclaration should match *any* decl - for example any of type alias / template class decl / class decl.
  • (3) If desugaring can result in both TemplateSpecializationType and TypedefType, then only one is arbitrarily picked - see test case TypedefTypeAndTemplateSpecializationTypeAreBothHere.  Again - this problem exists *prior* to my CL.
So - my plan right now is to just simplify/revert my patches so they only fix (1).  I don't think discussion of (2) should impact the CL, because the behavior that disagrees with Richard's expectations is happening *before* my changes.  I will also not attempt to fix (3), because this starts to interfere with (2) - sorry :-/.

QUESTION: Does the plan from the previous paragraph sound good to you?

Yes; I agree that failing to look through ElaboratedType here is just a bug.

Ack.  I've marked "elaborated type" section in the doc as something that is improved by the CL under review.
 
Experiments before my changes were done with the following unit tests:

// This testcase shows that Richard’s expectations are not met *prior* to my CL.
// (i.e. the testcase below passes before my CL;  the expectations in the testcase
// are the opposite of Richard’s expectations).
TEST(HasDeclaration, Case1FromEmail) {
  std::string input =
      "template<typename T>\n"
      "class Template {\n"
      " public:\n"
      "  void Method() {}\n"
      "};\n"
      "template <typename U>\n"
      "void Function(Template<U> param) {\n"
      "  param.Method();\n"

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Next steps for D24361 (patch to fix hasDeclaration wrt ElaboratedType)

Ivan Garramona via cfe-dev
On Fri, Oct 28, 2016 at 12:17 PM, Manuel Klimek <[hidden email]> wrote:

> Playing around with the code, we have 2 problems we need to solve:
> a) the confusion between type & qualType weirdness
> That got in by accident, I think, though the getDecl() specialization that
> behaves differently from the partial desugaring we have due to b)
> b) the partial desugaring we do
> This basically was born from the idea that some of the sugaring classes we
> have are really really weird for devs (like TemplateSpecializationType) or
> basically useless to match (like ElaboratedType, where we can match on the
> NNS if we need to).
>
> a) seems fixable by a more fundamental patch in HasDeclarationMatcher on
> which I'm working
> b) seems like something we should discuss

I agree, thank you for looking into this and working on it!

~Aaron

>
> On Fri, Oct 28, 2016 at 4:27 PM Manuel Klimek <[hidden email]> wrote:
>>
>> Now with the right cfe-dev
>>
>>
>> On Fri, Oct 28, 2016 at 4:14 PM Manuel Klimek <[hidden email]> wrote:
>>>
>>> Somehow we lost cfe-dev on this mail thread :( :(
>>>
>>> adding Aaron, who has also contributed to that part of the code
>>>
>>> On Fri, Oct 28, 2016 at 4:11 PM Manuel Klimek <[hidden email]> wrote:
>>>>
>>>> On Fri, Oct 28, 2016 at 3:54 PM Łukasz Anforowicz <[hidden email]>
>>>> wrote:
>>>>>
>>>>> Some notes from me (that hopefully will helpful to consider during the
>>>>> meeting):
>>>>>
>>>>> I agree that forcing hasDeclaration to consider all subclasses of Types
>>>>> (i.e. forcing via #include "clang/AST/TypeNodes.def" in the proposal below)
>>>>> seems to be a good thing.  The subset of types supported today seems to be
>>>>> rather arbitrary (I think;  please let me know if there is good reasoning +
>>>>> a way to describe the currently chosen subset of supported types).
>>>>> As a user of hasDeclaration, I have to "manually" drill through *some*
>>>>> can paints today.  For example in
>>>>> https://codereview.chromium.org/2256913002/patch/180001/190001 I am manually
>>>>> jumping over pointers and references (what I want to do is to tell whether a
>>>>> member dereference will end up using one of "my" classdecls where I am
>>>>> renaming methods and fields) - see |blink_qual_type_matcher| in that
>>>>> Chromium code review.  Manually stripping pointers and references is doable
>>>>> but 1) it is easy to get wrong (i.e. strip only one level) and 2) it is
>>>>> questionable whether other required stripping is not missed (i.e. auto
>>>>> type?),
>>>>> Today hasDeclaration doesn't match "the outermost can in our russian
>>>>> doll" as suggested (or proposed) by Manual.  Today desugaring is done in an
>>>>> arbitrary order of types - some (deep) types will be preferred over other
>>>>> (shallow) types as shown in an example in the separate doc.  In the example,
>>>>> even though the typeAliasDecl is the most shallow desugaring result, it will
>>>>> *never* be matched because hasDeclaration will first try (and latch onto)
>>>>> other types first.
>>>>> "has" vs "hasDescendant" is not a perfectly valid analogy.
>>>>> "hasDeclaration" is not like "has" today, because even today it can strip
>>>>> multiple layers of sugar (see the example from the previous bullet item).
>>>>>
>>>>> So:
>>>>>
>>>>> I would vote to make "hasDeclaration" match at every desugaring level
>>>>> (i.e. without splitting it into a "has" and a "hasDescendant"-like variant).
>>>>> Having "hasDeclaration" and "eventuallyDesugarsToDeclaration"
>>>>> ("hasDeepDeclaration"?) sounds like an okay option, but it seems to me that
>>>>> even today 1) hasDeclaration code and 2) users of hasDeclaration, behave
>>>>> like or expect behavior of "eventuallyDesugarsToDeclaration"
>>>>
>>>> I'm still looking into where we made the fundamental error that we're
>>>> running into those discussions now. There should be something simpler so
>>>> that the behavior more clearly matches the design of the clang AST.
>>>>>
>>>>> I hope that the discussion of "hasDeclaration" vs "hasDeepDeclaration"
>>>>> can be independent from, and not block the fixes from
>>>>> https://reviews.llvm.org/D24361 (i.e. being able to land them in the next
>>>>> 1-2 would be great).
>>>>
>>>> As Richard noted on the doc, it really depends on which direction we
>>>> want to go in, and whether the patch is a step in the right direction. Sorry
>>>> :(
>>>> But please continue pinging / making sure we drive this forward - I
>>>> think this is important, and I'm happy to spend some time to make sure we
>>>> get this right, and thanks for all the work you've already put into this, I
>>>> really appreciate it, and I think it's helping us getting to the root of the
>>>> problem.
>>>>
>>>>>
>>>>>
>>>>> On Thu, Oct 27, 2016 at 1:12 AM, Manuel Klimek <[hidden email]>
>>>>> wrote:
>>>>>>
>>>>>> +ben & sam
>>>>>>
>>>>>> On Wed, Oct 26, 2016 at 6:50 PM Łukasz Anforowicz <[hidden email]>
>>>>>> wrote:
>>>>>>>
>>>>>>> On Wed, Oct 26, 2016 at 9:46 AM, Łukasz Anforowicz
>>>>>>> <[hidden email]> wrote:
>>>>>>>>
>>>>>>>> So - is it fair to say that the CL under review can proceed as-is
>>>>>>>> (*) given that:
>>>>>>>>
>>>>>>>> The ordering problem has existed *before* the CL under review
>>>>>>>> (admittedly the CL under review adds 2 more cases to the list of problematic
>>>>>>>> ordering scenarios)
>>>>>>>> Below it seems that we have a promising direction for solving the
>>>>>>>> ordering problem (the solution probably requires a slightly bigger CL - one
>>>>>>>> that also tackles the question of what hasDeclaration should do about
>>>>>>>> AutoType, PointerType, etc.)
>>>>>>>>
>>>>>>>> -Lukasz
>>>>>>>>
>>>>>>>> (*) I know that I need to add documentation changes to mention
>>>>>>>> ElaboratedType
>>>>>>>>
>>>>>>>> On Tue, Oct 25, 2016 at 5:02 PM, Richard Smith
>>>>>>>> <[hidden email]> wrote:
>>>>>>>>>
>>>>>>>>> On 25 October 2016 at 16:47, Łukasz Anforowicz <[hidden email]>
>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>> I think I understand the concern - the concern is that desugaring
>>>>>>>>>> done in HasDeclarationMatcher::matchesSpecialized(const QualType&, ...)
>>>>>>>>>> *latches* onto desugared types in *arbitrary* order.  I agree that this is a
>>>>>>>>>> problem (most obvious for me for the typeAliasDecl and
>>>>>>>>>> classTemplateSpecializationDecl case).  I agree that the CL adds a new case
>>>>>>>>>> to the set of ordered desugaring attempts done by this method (and therefore
>>>>>>>>>> can be seen as making the old problem slightly worse).
>>>>>>>>>>
>>>>>>>>>> I think this problem can be fixed by getting rid of the *order*
>>>>>>>>>> inside HasDeclarationMatcher::matchesSpecialized(const QualType&, ...) -
>>>>>>>>>> instead of:
>>>>>>>>>>
>>>>>>>>>> if (auto *TD = Node->getAsTagDecl())
>>>>>>>>>>   return matchesDecl(TD, Finder, Builder);
>>>>>>>>>> else if (auto *TT = Node->getAs<TypedefType>())
>>>>>>>>>>   return matchesDecl(TT->getDecl(), Finder, Builder);
>>>>>>>>>>
>>>>>>>>>> /* the CL under review would add ElaboratedType and
>>>>>>>>>>    TemplateSpecializationType somewhere here */
>>>>>>>>>>
>>>>>>>>>> ...
>>>>>>>>>> else if (auto *ICNT = Node->getAs<InjectedClassNameType>())
>>>>>>>>>>   return matchesDecl(ICNT->getDecl(), Finder, Builder);
>>>>>>>>>> return false;
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> have something like this:
>>>>>>>>>>
>>>>>>>>>>     if (auto *TD = Node->getAsTagDecl()) {
>>>>>>>>>>       if (matchesDecl(TD, Finder, Builder))
>>>>>>>>>>         return true;
>>>>>>>>>>     }
>>>>>>>>>>
>>>>>>>>>>     if (auto *TT = Node->getAs<TypedefType>()) {
>>>>>>>>>>       if (matchesDecl(TT->getDecl(), Finder, Builder))
>>>>>>>>>>         return true;
>>>>>>>>>>     }
>>>>>>>>>>
>>>>>>>>>>     if (auto *ET = Node->getAs<ElaboratedType>()) {
>>>>>>>>>>       if (matchesSpecialized(ET->getNamedType(), Finder, Builder))
>>>>>>>>>>         return true;
>>>>>>>>>>     }
>>>>>>>>>>
>>>>>>>>>>     if (auto *TST = Node->getAs<TemplateSpecializationType>()) {
>>>>>>>>>>       if (matchesSpecialized(*TST, Finder, Builder))
>>>>>>>>>>         return true;
>>>>>>>>>>     }
>>>>>>>>>> ...
>>>>>>>>>>     if (auto *ICNT = Node->getAs<InjectedClassNameType>()) {
>>>>>>>>>>       if (matchesDecl(ICNT->getDecl(), Finder, Builder))
>>>>>>>>>>         return true;
>>>>>>>>>>     }
>>>>>>>>>>
>>>>>>>>>>     return false;
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I think this is an interesting idea. It would certainly address my
>>>>>>>>> concerns. (You'd actually want something slightly different from the above,
>>>>>>>>> I think: given
>>>>>>>>>
>>>>>>>>>   typedef int A;
>>>>>>>>>   typedef A B;
>>>>>>>>>   B x;
>>>>>>>>>
>>>>>>>>> ...
>>>>>>>>> varDecl(hasType(qualType(hasDeclaration(typedefNameDecl().bind("TD")))))
>>>>>>>>> should match x /twice/: once with TD pointing at A and once with it pointing
>>>>>>>>> at B. So you probably want to repeatedly single-step desugar the type and
>>>>>>>>> try extracting a declaration from each level of type sugar.)
>>>>>>>>
>>>>>>>>
>>>>>>>> Oh, good point - I haven't thought about this scenario.  I think
>>>>>>>> this direction (based on getAsSuger from Type.cpp) look promising (passes a
>>>>>>>> test with 2 chained typedefs):
>>>>>>>>
>>>>>>>> template <typename T, typename DeclMatcherT>
>>>>>>>> class HasDeclarationMatcher : public WrapperMatcherInterface<T> {
>>>>>>>> ...
>>>>>>>>
>>>>>>>>   /// \brief Extracts the TagDecl of a QualType and returns whether
>>>>>>>> the inner
>>>>>>>>   /// matcher matches on it.
>>>>>>>>   bool matchesSpecialized(const QualType &Node, ASTMatchFinder
>>>>>>>> *Finder,
>>>>>>>>                           BoundNodesTreeBuilder *Builder) const {
>>>>>>>>     if (Node.isNull())
>>>>>>>>       return false;
>>>>>>>>
>>>>>>>>     // Do not use getAs<TemplateTypeParmType> instead of the direct
>>>>>>>> dyn_cast.
>>>>>>>>     // Calling getAs will return the canonical type, but that type
>>>>>>>> does not
>>>>>>>>     // store a TemplateTypeParmDecl. We *need* the uncanonical type,
>>>>>>>> if it is
>>>>>>>>     // available, and using dyn_cast ensures that.
>>>>>>>>     if (auto *TTP =
>>>>>>>> dyn_cast<TemplateTypeParmType>(Node.getTypePtr())) {
>>>>>>>>       if (matchesDecl(TTP->getDecl(), Finder, Builder))
>>>>>>>>         return true;
>>>>>>>>     }
>>>>>>>>
>>>>>>>>     const Type* Cur = Node.getTypePtr();
>>>>>>>>     while (Cur) {
>>>>>>>>       switch (Cur->getTypeClass()) {
>>>>>>>> #define ABSTRACT_TYPE(Class, Parent)
>>>>>>>> #define TYPE(Class, Parent) \
>>>>>>>>       case Type::Class: { \
>>>>>>>>         const Class##Type *Ty = cast<Class##Type>(Cur); \
>>>>>>>>         if (matchesSpecialized(*Ty, Finder, Builder)) \
>>>>>>>>           return true; \
>>>>>>>>         if (!Ty->isSugared()) return false; \
>>>>>>>>         Cur = Ty->desugar().getTypePtr(); \
>>>>>>>>         break; \
>>>>>>>>       }
>>>>>>>> #include "clang/AST/TypeNodes.def"
>>>>>>>>       }
>>>>>>>>     }
>>>>>>>>
>>>>>>>>     return false;
>>>>>>>>   }
>>>>>>>>
>>>>>>>>
>>>>>>> BTW: I have to note that the code above is potentially O(N^2) where N
>>>>>>> is the length of the desugaring chain (because each desugaring step can
>>>>>>> potentially call back into the QualType overload).  This is obviously
>>>>>>> suboptimal.  We probably need to discuss this when we review / talk about
>>>>>>> landing something like this.
>>>>>>
>>>>>>
>>>>>> It's basically the same difference between has() and hasDescendant.
>>>>>> I think
>>>>>> a) if we go that route, we should make it explicit by having a matcher
>>>>>> that has it in its name
>>>>>> b) we need to be able to memoize at each step, so this needs to become
>>>>>> part of the visitor interface of the matchers; given the sheer number of
>>>>>> types this seems like it'll get rather complex
>>>>>>
>>>>>> I'll make sure to discuss this with Richard etc al in person next week
>>>>>> when we're at LLVM conf.
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> but it opens a whole bunch of questions - what to do with other
>>>>>>>> types - for example should hasDeclaration "jump over / drill holes in paint
>>>>>>>> cans" for:
>>>>>>>>
>>>>>>>> AutoType, TypeOfExprType, DecltypeType, TypeOfType
>>>>>>>> LValueReferenceType, ValueReferenceType
>>>>>>>> PointerType, ReferenceType
>>>>>>>> And all the other types where I had to add a matchSpecialized
>>>>>>>> overload for so that things compile (some of these obviously don't have a
>>>>>>>> decl [and therefore shouldn't match hasDeclaration(...)] - like BuiltinType,
>>>>>>>> but I am listing all of them for completeness): AtomicType, PipeType,
>>>>>>>> ObjCObjectPointerType, ObjCObjectType, PackExpansionType,
>>>>>>>> DependentTemplateSpecializationType, DependentNameType, AttributedType,
>>>>>>>> UnaryTransformType, SubstTemplateTypeParmPackType,
>>>>>>>> SubstTemplateTypeParmType, AdjustedType, DecayedType, ParenType,
>>>>>>>> FunctionNoProtoType, FunctionProtoType, ExtVectorType, VectorType,
>>>>>>>> DependentSizedExtVectorType, DependentSizedArrayType, VariableArrayType,
>>>>>>>> IncompleteArrayType, ConstantArrayType, MemberPointerType, BlockPointerType,
>>>>>>>> ComplexType, BuiltinType,
>>>>>>>>
>>>>>>>> Also - I am not quite sure if I understand if special-casing of
>>>>>>>> TemplateTypeParmType is needed.  It probably is (because here we use
>>>>>>>> dyn_cast rather than cast).  At any rate drilling into details here can
>>>>>>>> probably wait until a real review in Phabricator...
>>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> The code above passes all tests from the clang-test ninja target +
>>>>>>>>>> results in the following behavior for the 2 cases pointed out in an email:
>>>>>>>>>>
>>>>>>>>>> TEST(HasDeclaration, Case1FromEmail) {
>>>>>>>>>>   std::string input =
>>>>>>>>>>       "template<typename T>\n"
>>>>>>>>>>       "class Template {\n"
>>>>>>>>>>       " public:\n"
>>>>>>>>>>       "  void Method() {}\n"
>>>>>>>>>>       "};\n"
>>>>>>>>>>       "template <typename U>\n"
>>>>>>>>>>       "void Function(Template<U> param) {\n"
>>>>>>>>>>       "  param.Method();\n"
>>>>>>>>>>       "};\n";
>>>>>>>>>>
>>>>>>>>>>   // param type has-declaration matches class template decl?
>>>>>>>>>>   EXPECT_TRUE(matches(input, parmVarDecl(
>>>>>>>>>>       hasName("param"),
>>>>>>>>>>       hasType(qualType(
>>>>>>>>>>
>>>>>>>>>> hasDeclaration(decl(classTemplateDecl(hasName("Template")))))))));
>>>>>>>>>>
>>>>>>>>>>   // param type has-declaration matches class decl?
>>>>>>>>>>   EXPECT_FALSE(matches(input, parmVarDecl(
>>>>>>>>>>       hasName("param"),
>>>>>>>>>>       hasType(qualType(
>>>>>>>>>>
>>>>>>>>>> hasDeclaration(decl(cxxRecordDecl(hasName("Template")))))))));
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> TEST(HasDeclaration, Case2FromEmail) {
>>>>>>>>>>   std::string input =
>>>>>>>>>>       "template<typename T>\n"
>>>>>>>>>>       "class Template {\n"
>>>>>>>>>>       " public:\n"
>>>>>>>>>>       "  void Method() {}\n"
>>>>>>>>>>       "};\n"
>>>>>>>>>>       "void Function(Template<int> param) {\n"
>>>>>>>>>>       "  param.Method();\n"
>>>>>>>>>>       "};\n";
>>>>>>>>>>
>>>>>>>>>>   // param type has-declaration matches class template decl?
>>>>>>>>>>   EXPECT_TRUE(matches(input, parmVarDecl(
>>>>>>>>>>       hasName("param"),
>>>>>>>>>>       hasType(qualType(
>>>>>>>>>>
>>>>>>>>>> hasDeclaration(decl(classTemplateDecl(hasName("Template")))))))));
>>>>>>>>>>
>>>>>>>>>>   // param type has-declaration matches class decl?
>>>>>>>>>>   EXPECT_TRUE(matches(input, parmVarDecl(
>>>>>>>>>>       hasName("param"),
>>>>>>>>>>       hasType(qualType(
>>>>>>>>>>
>>>>>>>>>> hasDeclaration(decl(cxxRecordDecl(hasName("Template")))))))));
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> If the test expectations above look reasonable, then we can just
>>>>>>>>>> discuss how to land the code changes above (e.g. as a single CL or 2 CLs - I
>>>>>>>>>> would prefer 2 separate CLs).  Otherwise, let's talk about the test
>>>>>>>>>> expectations - what should they look like / how should they be different
>>>>>>>>>> from the test snippet above?
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On Tue, Oct 25, 2016 at 1:42 PM, Richard Smith
>>>>>>>>>> <[hidden email]> wrote:
>>>>>>>>>>>
>>>>>>>>>>> On 25 October 2016 at 01:57, Manuel Klimek <[hidden email]>
>>>>>>>>>>> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> On Tue, Oct 25, 2016 at 3:25 AM Richard Smith
>>>>>>>>>>>> <[hidden email]> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 24 October 2016 at 16:41, Łukasz Anforowicz
>>>>>>>>>>>>> <[hidden email]> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I am not sure if we are making progress, but so far, we have
>>>>>>>>>>>>>> seen 1) test cases that are broken and fixed by the CL under review and 2)
>>>>>>>>>>>>>> test cases that are not changed by the CL under review and 3) no test cases
>>>>>>>>>>>>>> that are made worse by the CL under review.  I've tried to gather the
>>>>>>>>>>>>>> scenarios and the associated test cases in
>>>>>>>>>>>>>> https://docs.google.com/document/d/1y1F6uKdulluYBFch680g6bmHe_zDe8mHFYjfXRoakNI/edit.
>>>>>>>>>>>>>> Could you please take a look and shout if I misrepresented anything (i.e.
>>>>>>>>>>>>>> whether we agree that a specific test scenario shows a bug).
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I've also added some comments below / inline.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On Mon, Oct 24, 2016 at 3:54 PM, Richard Smith
>>>>>>>>>>>>>> <[hidden email]> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 24 October 2016 at 15:25, Łukasz Anforowicz
>>>>>>>>>>>>>>> <[hidden email]> wrote:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On Mon, Oct 24, 2016 at 2:21 PM, Richard Smith
>>>>>>>>>>>>>>>> <[hidden email]> wrote:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> On 24 October 2016 at 11:23, Łukasz Anforowicz
>>>>>>>>>>>>>>>>> <[hidden email]> wrote:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> I've run a few more experiments and I see that before any
>>>>>>>>>>>>>>>>>> changes:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> (1) Presence of ElaboratedType means that hasDeclaration
>>>>>>>>>>>>>>>>>> doesn’t match *anything*.  This is clearly wrong.  I still want to fix that.
>>>>>>>>>>>>>>>>>> (2) Both examples from Richard’s email consistently match
>>>>>>>>>>>>>>>>>> the class template decl (and not the class decl).  If Richard’s expectations
>>>>>>>>>>>>>>>>>> are not met *prior* to my CL, then I don’t think these expectations should
>>>>>>>>>>>>>>>>>> be fixed by my CL.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> OTOH, maybe it is worth to open a bug to investigate this
>>>>>>>>>>>>>>>>>> more - personally I would argue that hasDeclaration should match *any* decl
>>>>>>>>>>>>>>>>>> - for example any of type alias / template class decl / class decl.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> (3) If desugaring can result in both
>>>>>>>>>>>>>>>>>> TemplateSpecializationType and TypedefType, then only one is arbitrarily
>>>>>>>>>>>>>>>>>> picked - see test case TypedefTypeAndTemplateSpecializationTypeAreBothHere.
>>>>>>>>>>>>>>>>>> Again - this problem exists *prior* to my CL.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> So - my plan right now is to just simplify/revert my
>>>>>>>>>>>>>>>>>> patches so they only fix (1).  I don't think discussion of (2) should impact
>>>>>>>>>>>>>>>>>> the CL, because the behavior that disagrees with Richard's expectations is
>>>>>>>>>>>>>>>>>> happening *before* my changes.  I will also not attempt to fix (3), because
>>>>>>>>>>>>>>>>>> this starts to interfere with (2) - sorry :-/.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> QUESTION: Does the plan from the previous paragraph sound
>>>>>>>>>>>>>>>>>> good to you?
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Yes; I agree that failing to look through ElaboratedType
>>>>>>>>>>>>>>>>> here is just a bug.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Ack.  I've marked "elaborated type" section in the doc as
>>>>>>>>>>>>>> something that is improved by the CL under review.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Experiments before my changes were done with the following
>>>>>>>>>>>>>>>>>> unit tests:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> // This testcase shows that Richard’s expectations are not
>>>>>>>>>>>>>>>>>> met *prior* to my CL.
>>>>>>>>>>>>>>>>>> // (i.e. the testcase below passes before my CL;  the
>>>>>>>>>>>>>>>>>> expectations in the testcase
>>>>>>>>>>>>>>>>>> // are the opposite of Richard’s expectations).
>>>>>>>>>>>>>>>>>> TEST(HasDeclaration, Case1FromEmail) {
>>>>>>>>>>>>>>>>>>   std::string input =
>>>>>>>>>>>>>>>>>>       "template<typename T>\n"
>>>>>>>>>>>>>>>>>>       "class Template {\n"
>>>>>>>>>>>>>>>>>>       " public:\n"
>>>>>>>>>>>>>>>>>>       "  void Method() {}\n"
>>>>>>>>>>>>>>>>>>       "};\n"
>>>>>>>>>>>>>>>>>>       "template <typename U>\n"
>>>>>>>>>>>>>>>>>>       "void Function(Template<U> param) {\n"
>>>>>>>>>>>>>>>>>>       "  param.Method();\n"
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Next steps for D24361 (patch to fix hasDeclaration wrt ElaboratedType)

Ivan Garramona via cfe-dev
On Mon, Oct 31, 2016 at 7:18 AM, Aaron Ballman <[hidden email]> wrote:
On Fri, Oct 28, 2016 at 12:17 PM, Manuel Klimek <[hidden email]> wrote:
> Playing around with the code, we have 2 problems we need to solve:
> a) the confusion between type & qualType weirdness
> That got in by accident, I think, though the getDecl() specialization that
> behaves differently from the partial desugaring we have due to b)
> b) the partial desugaring we do
> This basically was born from the idea that some of the sugaring classes we
> have are really really weird for devs (like TemplateSpecializationType) or
> basically useless to match (like ElaboratedType, where we can match on the
> NNS if we need to).
>
> a) seems fixable by a more fundamental patch in HasDeclarationMatcher on
> which I'm working
> b) seems like something we should discuss

I agree, thank you for looking into this and working on it!

~Aaron

>
> On Fri, Oct 28, 2016 at 4:27 PM Manuel Klimek <[hidden email]> wrote:
>>
>> Now with the right cfe-dev
>>
>>
>> On Fri, Oct 28, 2016 at 4:14 PM Manuel Klimek <[hidden email]> wrote:
>>>
>>> Somehow we lost cfe-dev on this mail thread :( :(
>>>
>>> adding Aaron, who has also contributed to that part of the code
>>>
>>> On Fri, Oct 28, 2016 at 4:11 PM Manuel Klimek <[hidden email]> wrote:
>>>>
>>>> On Fri, Oct 28, 2016 at 3:54 PM Łukasz Anforowicz <[hidden email]>
>>>> wrote:
>>>>>
>>>>> Some notes from me (that hopefully will helpful to consider during the
>>>>> meeting):
>>>>>
>>>>> I agree that forcing hasDeclaration to consider all subclasses of Types
>>>>> (i.e. forcing via #include "clang/AST/TypeNodes.def" in the proposal below)
>>>>> seems to be a good thing.  The subset of types supported today seems to be
>>>>> rather arbitrary (I think;  please let me know if there is good reasoning +
>>>>> a way to describe the currently chosen subset of supported types).
>>>>> As a user of hasDeclaration, I have to "manually" drill through *some*
>>>>> can paints today.  For example in
>>>>> https://codereview.chromium.org/2256913002/patch/180001/190001 I am manually
>>>>> jumping over pointers and references (what I want to do is to tell whether a
>>>>> member dereference will end up using one of "my" classdecls where I am
>>>>> renaming methods and fields) - see |blink_qual_type_matcher| in that
>>>>> Chromium code review.  Manually stripping pointers and references is doable
>>>>> but 1) it is easy to get wrong (i.e. strip only one level) and 2) it is
>>>>> questionable whether other required stripping is not missed (i.e. auto
>>>>> type?),
>>>>> Today hasDeclaration doesn't match "the outermost can in our russian
>>>>> doll" as suggested (or proposed) by Manual.  Today desugaring is done in an
>>>>> arbitrary order of types - some (deep) types will be preferred over other
>>>>> (shallow) types as shown in an example in the separate doc.  In the example,
>>>>> even though the typeAliasDecl is the most shallow desugaring result, it will
>>>>> *never* be matched because hasDeclaration will first try (and latch onto)
>>>>> other types first.
>>>>> "has" vs "hasDescendant" is not a perfectly valid analogy.
>>>>> "hasDeclaration" is not like "has" today, because even today it can strip
>>>>> multiple layers of sugar (see the example from the previous bullet item).
>>>>>
>>>>> So:
>>>>>
>>>>> I would vote to make "hasDeclaration" match at every desugaring level
>>>>> (i.e. without splitting it into a "has" and a "hasDescendant"-like variant).
>>>>> Having "hasDeclaration" and "eventuallyDesugarsToDeclaration"
>>>>> ("hasDeepDeclaration"?) sounds like an okay option, but it seems to me that
>>>>> even today 1) hasDeclaration code and 2) users of hasDeclaration, behave
>>>>> like or expect behavior of "eventuallyDesugarsToDeclaration"
>>>>
>>>> I'm still looking into where we made the fundamental error that we're
>>>> running into those discussions now. There should be something simpler so
>>>> that the behavior more clearly matches the design of the clang AST.
>>>>>
>>>>> I hope that the discussion of "hasDeclaration" vs "hasDeepDeclaration"
>>>>> can be independent from, and not block the fixes from
>>>>> https://reviews.llvm.org/D24361 (i.e. being able to land them in the next
>>>>> 1-2 would be great).
>>>>
>>>> As Richard noted on the doc, it really depends on which direction we
>>>> want to go in, and whether the patch is a step in the right direction. Sorry
>>>> :(
>>>> But please continue pinging / making sure we drive this forward - I
>>>> think this is important, and I'm happy to spend some time to make sure we
>>>> get this right, and thanks for all the work you've already put into this, I
>>>> really appreciate it, and I think it's helping us getting to the root of the
>>>> problem.

Ping :-)  Any progress here?  (both in 1) the general direction discussion as well as 2) the short-term planning/impact for ElaboratedType handling in https://reviews.llvm.org/D24361 ?) 

>>>>
>>>>>
>>>>>
>>>>> On Thu, Oct 27, 2016 at 1:12 AM, Manuel Klimek <[hidden email]>
>>>>> wrote:
>>>>>>
>>>>>> +ben & sam
>>>>>>
>>>>>> On Wed, Oct 26, 2016 at 6:50 PM Łukasz Anforowicz <[hidden email]>
>>>>>> wrote:
>>>>>>>
>>>>>>> On Wed, Oct 26, 2016 at 9:46 AM, Łukasz Anforowicz
>>>>>>> <[hidden email]> wrote:
>>>>>>>>
>>>>>>>> So - is it fair to say that the CL under review can proceed as-is
>>>>>>>> (*) given that:
>>>>>>>>
>>>>>>>> The ordering problem has existed *before* the CL under review
>>>>>>>> (admittedly the CL under review adds 2 more cases to the list of problematic
>>>>>>>> ordering scenarios)
>>>>>>>> Below it seems that we have a promising direction for solving the
>>>>>>>> ordering problem (the solution probably requires a slightly bigger CL - one
>>>>>>>> that also tackles the question of what hasDeclaration should do about
>>>>>>>> AutoType, PointerType, etc.)
>>>>>>>>
>>>>>>>> -Lukasz
>>>>>>>>
>>>>>>>> (*) I know that I need to add documentation changes to mention
>>>>>>>> ElaboratedType
>>>>>>>>
>>>>>>>> On Tue, Oct 25, 2016 at 5:02 PM, Richard Smith
>>>>>>>> <[hidden email]> wrote:
>>>>>>>>>
>>>>>>>>> On 25 October 2016 at 16:47, Łukasz Anforowicz <[hidden email]>
>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>> I think I understand the concern - the concern is that desugaring
>>>>>>>>>> done in HasDeclarationMatcher::matchesSpecialized(const QualType&, ...)
>>>>>>>>>> *latches* onto desugared types in *arbitrary* order.  I agree that this is a
>>>>>>>>>> problem (most obvious for me for the typeAliasDecl and
>>>>>>>>>> classTemplateSpecializationDecl case).  I agree that the CL adds a new case
>>>>>>>>>> to the set of ordered desugaring attempts done by this method (and therefore
>>>>>>>>>> can be seen as making the old problem slightly worse).
>>>>>>>>>>
>>>>>>>>>> I think this problem can be fixed by getting rid of the *order*
>>>>>>>>>> inside HasDeclarationMatcher::matchesSpecialized(const QualType&, ...) -
>>>>>>>>>> instead of:
>>>>>>>>>>
>>>>>>>>>> if (auto *TD = Node->getAsTagDecl())
>>>>>>>>>>   return matchesDecl(TD, Finder, Builder);
>>>>>>>>>> else if (auto *TT = Node->getAs<TypedefType>())
>>>>>>>>>>   return matchesDecl(TT->getDecl(), Finder, Builder);
>>>>>>>>>>
>>>>>>>>>> /* the CL under review would add ElaboratedType and
>>>>>>>>>>    TemplateSpecializationType somewhere here */
>>>>>>>>>>
>>>>>>>>>> ...
>>>>>>>>>> else if (auto *ICNT = Node->getAs<InjectedClassNameType>())
>>>>>>>>>>   return matchesDecl(ICNT->getDecl(), Finder, Builder);
>>>>>>>>>> return false;
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> have something like this:
>>>>>>>>>>
>>>>>>>>>>     if (auto *TD = Node->getAsTagDecl()) {
>>>>>>>>>>       if (matchesDecl(TD, Finder, Builder))
>>>>>>>>>>         return true;
>>>>>>>>>>     }
>>>>>>>>>>
>>>>>>>>>>     if (auto *TT = Node->getAs<TypedefType>()) {
>>>>>>>>>>       if (matchesDecl(TT->getDecl(), Finder, Builder))
>>>>>>>>>>         return true;
>>>>>>>>>>     }
>>>>>>>>>>
>>>>>>>>>>     if (auto *ET = Node->getAs<ElaboratedType>()) {
>>>>>>>>>>       if (matchesSpecialized(ET->getNamedType(), Finder, Builder))
>>>>>>>>>>         return true;
>>>>>>>>>>     }
>>>>>>>>>>
>>>>>>>>>>     if (auto *TST = Node->getAs<TemplateSpecializationType>()) {
>>>>>>>>>>       if (matchesSpecialized(*TST, Finder, Builder))
>>>>>>>>>>         return true;
>>>>>>>>>>     }
>>>>>>>>>> ...
>>>>>>>>>>     if (auto *ICNT = Node->getAs<InjectedClassNameType>()) {
>>>>>>>>>>       if (matchesDecl(ICNT->getDecl(), Finder, Builder))
>>>>>>>>>>         return true;
>>>>>>>>>>     }
>>>>>>>>>>
>>>>>>>>>>     return false;
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I think this is an interesting idea. It would certainly address my
>>>>>>>>> concerns. (You'd actually want something slightly different from the above,
>>>>>>>>> I think: given
>>>>>>>>>
>>>>>>>>>   typedef int A;
>>>>>>>>>   typedef A B;
>>>>>>>>>   B x;
>>>>>>>>>
>>>>>>>>> ...
>>>>>>>>> varDecl(hasType(qualType(hasDeclaration(typedefNameDecl().bind("TD")))))
>>>>>>>>> should match x /twice/: once with TD pointing at A and once with it pointing
>>>>>>>>> at B. So you probably want to repeatedly single-step desugar the type and
>>>>>>>>> try extracting a declaration from each level of type sugar.)
>>>>>>>>
>>>>>>>>
>>>>>>>> Oh, good point - I haven't thought about this scenario.  I think
>>>>>>>> this direction (based on getAsSuger from Type.cpp) look promising (passes a
>>>>>>>> test with 2 chained typedefs):
>>>>>>>>
>>>>>>>> template <typename T, typename DeclMatcherT>
>>>>>>>> class HasDeclarationMatcher : public WrapperMatcherInterface<T> {
>>>>>>>> ...
>>>>>>>>
>>>>>>>>   /// \brief Extracts the TagDecl of a QualType and returns whether
>>>>>>>> the inner
>>>>>>>>   /// matcher matches on it.
>>>>>>>>   bool matchesSpecialized(const QualType &Node, ASTMatchFinder
>>>>>>>> *Finder,
>>>>>>>>                           BoundNodesTreeBuilder *Builder) const {
>>>>>>>>     if (Node.isNull())
>>>>>>>>       return false;
>>>>>>>>
>>>>>>>>     // Do not use getAs<TemplateTypeParmType> instead of the direct
>>>>>>>> dyn_cast.
>>>>>>>>     // Calling getAs will return the canonical type, but that type
>>>>>>>> does not
>>>>>>>>     // store a TemplateTypeParmDecl. We *need* the uncanonical type,
>>>>>>>> if it is
>>>>>>>>     // available, and using dyn_cast ensures that.
>>>>>>>>     if (auto *TTP =
>>>>>>>> dyn_cast<TemplateTypeParmType>(Node.getTypePtr())) {
>>>>>>>>       if (matchesDecl(TTP->getDecl(), Finder, Builder))
>>>>>>>>         return true;
>>>>>>>>     }
>>>>>>>>
>>>>>>>>     const Type* Cur = Node.getTypePtr();
>>>>>>>>     while (Cur) {
>>>>>>>>       switch (Cur->getTypeClass()) {
>>>>>>>> #define ABSTRACT_TYPE(Class, Parent)
>>>>>>>> #define TYPE(Class, Parent) \
>>>>>>>>       case Type::Class: { \
>>>>>>>>         const Class##Type *Ty = cast<Class##Type>(Cur); \
>>>>>>>>         if (matchesSpecialized(*Ty, Finder, Builder)) \
>>>>>>>>           return true; \
>>>>>>>>         if (!Ty->isSugared()) return false; \
>>>>>>>>         Cur = Ty->desugar().getTypePtr(); \
>>>>>>>>         break; \
>>>>>>>>       }
>>>>>>>> #include "clang/AST/TypeNodes.def"
>>>>>>>>       }
>>>>>>>>     }
>>>>>>>>
>>>>>>>>     return false;
>>>>>>>>   }
>>>>>>>>
>>>>>>>>
>>>>>>> BTW: I have to note that the code above is potentially O(N^2) where N
>>>>>>> is the length of the desugaring chain (because each desugaring step can
>>>>>>> potentially call back into the QualType overload).  This is obviously
>>>>>>> suboptimal.  We probably need to discuss this when we review / talk about
>>>>>>> landing something like this.
>>>>>>
>>>>>>
>>>>>> It's basically the same difference between has() and hasDescendant.
>>>>>> I think
>>>>>> a) if we go that route, we should make it explicit by having a matcher
>>>>>> that has it in its name
>>>>>> b) we need to be able to memoize at each step, so this needs to become
>>>>>> part of the visitor interface of the matchers; given the sheer number of
>>>>>> types this seems like it'll get rather complex
>>>>>>
>>>>>> I'll make sure to discuss this with Richard etc al in person next week
>>>>>> when we're at LLVM conf.
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> but it opens a whole bunch of questions - what to do with other
>>>>>>>> types - for example should hasDeclaration "jump over / drill holes in paint
>>>>>>>> cans" for:
>>>>>>>>
>>>>>>>> AutoType, TypeOfExprType, DecltypeType, TypeOfType
>>>>>>>> LValueReferenceType, ValueReferenceType
>>>>>>>> PointerType, ReferenceType
>>>>>>>> And all the other types where I had to add a matchSpecialized
>>>>>>>> overload for so that things compile (some of these obviously don't have a
>>>>>>>> decl [and therefore shouldn't match hasDeclaration(...)] - like BuiltinType,
>>>>>>>> but I am listing all of them for completeness): AtomicType, PipeType,
>>>>>>>> ObjCObjectPointerType, ObjCObjectType, PackExpansionType,
>>>>>>>> DependentTemplateSpecializationType, DependentNameType, AttributedType,
>>>>>>>> UnaryTransformType, SubstTemplateTypeParmPackType,
>>>>>>>> SubstTemplateTypeParmType, AdjustedType, DecayedType, ParenType,
>>>>>>>> FunctionNoProtoType, FunctionProtoType, ExtVectorType, VectorType,
>>>>>>>> DependentSizedExtVectorType, DependentSizedArrayType, VariableArrayType,
>>>>>>>> IncompleteArrayType, ConstantArrayType, MemberPointerType, BlockPointerType,
>>>>>>>> ComplexType, BuiltinType,
>>>>>>>>
>>>>>>>> Also - I am not quite sure if I understand if special-casing of
>>>>>>>> TemplateTypeParmType is needed.  It probably is (because here we use
>>>>>>>> dyn_cast rather than cast).  At any rate drilling into details here can
>>>>>>>> probably wait until a real review in Phabricator...
>>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> The code above passes all tests from the clang-test ninja target +
>>>>>>>>>> results in the following behavior for the 2 cases pointed out in an email:
>>>>>>>>>>
>>>>>>>>>> TEST(HasDeclaration, Case1FromEmail) {
>>>>>>>>>>   std::string input =
>>>>>>>>>>       "template<typename T>\n"
>>>>>>>>>>       "class Template {\n"
>>>>>>>>>>       " public:\n"
>>>>>>>>>>       "  void Method() {}\n"
>>>>>>>>>>       "};\n"
>>>>>>>>>>       "template <typename U>\n"
>>>>>>>>>>       "void Function(Template<U> param) {\n"
>>>>>>>>>>       "  param.Method();\n"
>>>>>>>>>>       "};\n";
>>>>>>>>>>
>>>>>>>>>>   // param type has-declaration matches class template decl?
>>>>>>>>>>   EXPECT_TRUE(matches(input, parmVarDecl(
>>>>>>>>>>       hasName("param"),
>>>>>>>>>>       hasType(qualType(
>>>>>>>>>>
>>>>>>>>>> hasDeclaration(decl(classTemplateDecl(hasName("Template")))))))));
>>>>>>>>>>
>>>>>>>>>>   // param type has-declaration matches class decl?
>>>>>>>>>>   EXPECT_FALSE(matches(input, parmVarDecl(
>>>>>>>>>>       hasName("param"),
>>>>>>>>>>       hasType(qualType(
>>>>>>>>>>
>>>>>>>>>> hasDeclaration(decl(cxxRecordDecl(hasName("Template")))))))));
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> TEST(HasDeclaration, Case2FromEmail) {
>>>>>>>>>>   std::string input =
>>>>>>>>>>       "template<typename T>\n"
>>>>>>>>>>       "class Template {\n"
>>>>>>>>>>       " public:\n"
>>>>>>>>>>       "  void Method() {}\n"
>>>>>>>>>>       "};\n"
>>>>>>>>>>       "void Function(Template<int> param) {\n"
>>>>>>>>>>       "  param.Method();\n"
>>>>>>>>>>       "};\n";
>>>>>>>>>>
>>>>>>>>>>   // param type has-declaration matches class template decl?
>>>>>>>>>>   EXPECT_TRUE(matches(input, parmVarDecl(
>>>>>>>>>>       hasName("param"),
>>>>>>>>>>       hasType(qualType(
>>>>>>>>>>
>>>>>>>>>> hasDeclaration(decl(classTemplateDecl(hasName("Template")))))))));
>>>>>>>>>>
>>>>>>>>>>   // param type has-declaration matches class decl?
>>>>>>>>>>   EXPECT_TRUE(matches(input, parmVarDecl(
>>>>>>>>>>       hasName("param"),
>>>>>>>>>>       hasType(qualType(
>>>>>>>>>>
>>>>>>>>>> hasDeclaration(decl(cxxRecordDecl(hasName("Template")))))))));
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> If the test expectations above look reasonable, then we can just
>>>>>>>>>> discuss how to land the code changes above (e.g. as a single CL or 2 CLs - I
>>>>>>>>>> would prefer 2 separate CLs).  Otherwise, let's talk about the test
>>>>>>>>>> expectations - what should they look like / how should they be different
>>>>>>>>>> from the test snippet above?
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On Tue, Oct 25, 2016 at 1:42 PM, Richard Smith
>>>>>>>>>> <[hidden email]> wrote:
>>>>>>>>>>>
>>>>>>>>>>> On 25 October 2016 at 01:57, Manuel Klimek <[hidden email]>
>>>>>>>>>>> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> On Tue, Oct 25, 2016 at 3:25 AM Richard Smith
>>>>>>>>>>>> <[hidden email]> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 24 October 2016 at 16:41, Łukasz Anforowicz
>>>>>>>>>>>>> <[hidden email]> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I am not sure if we are making progress, but so far, we have
>>>>>>>>>>>>>> seen 1) test cases that are broken and fixed by the CL under review and 2)
>>>>>>>>>>>>>> test cases that are not changed by the CL under review and 3) no test cases
>>>>>>>>>>>>>> that are made worse by the CL under review.  I've tried to gather the
>>>>>>>>>>>>>> scenarios and the associated test cases in
>>>>>>>>>>>>>> https://docs.google.com/document/d/1y1F6uKdulluYBFch680g6bmHe_zDe8mHFYjfXRoakNI/edit.
>>>>>>>>>>>>>> Could you please take a look and shout if I misrepresented anything (i.e.
>>>>>>>>>>>>>> whether we agree that a specific test scenario shows a bug).
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I've also added some comments below / inline.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On Mon, Oct 24, 2016 at 3:54 PM, Richard Smith
>>>>>>>>>>>>>> <[hidden email]> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 24 October 2016 at 15:25, Łukasz Anforowicz
>>>>>>>>>>>>>>> <[hidden email]> wrote:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On Mon, Oct 24, 2016 at 2:21 PM, Richard Smith
>>>>>>>>>>>>>>>> <[hidden email]> wrote:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> On 24 October 2016 at 11:23, Łukasz Anforowicz
>>>>>>>>>>>>>>>>> <[hidden email]> wrote:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> I've run a few more experiments and I see that before any
>>>>>>>>>>>>>>>>>> changes:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> (1) Presence of ElaboratedType means that hasDeclaration
>>>>>>>>>>>>>>>>>> doesn’t match *anything*.  This is clearly wrong.  I still want to fix that.
>>>>>>>>>>>>>>>>>> (2) Both examples from Richard’s email consistently match
>>>>>>>>>>>>>>>>>> the class template decl (and not the class decl).  If Richard’s expectations
>>>>>>>>>>>>>>>>>> are not met *prior* to my CL, then I don’t think these expectations should
>>>>>>>>>>>>>>>>>> be fixed by my CL.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> OTOH, maybe it is worth to open a bug to investigate this
>>>>>>>>>>>>>>>>>> more - personally I would argue that hasDeclaration should match *any* decl
>>>>>>>>>>>>>>>>>> - for example any of type alias / template class decl / class decl.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> (3) If desugaring can result in both
>>>>>>>>>>>>>>>>>> TemplateSpecializationType and TypedefType, then only one is arbitrarily
>>>>>>>>>>>>>>>>>> picked - see test case TypedefTypeAndTemplateSpecializationTypeAreBothHere.
>>>>>>>>>>>>>>>>>> Again - this problem exists *prior* to my CL.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> So - my plan right now is to just simplify/revert my
>>>>>>>>>>>>>>>>>> patches so they only fix (1).  I don't think discussion of (2) should impact
>>>>>>>>>>>>>>>>>> the CL, because the behavior that disagrees with Richard's expectations is
>>>>>>>>>>>>>>>>>> happening *before* my changes.  I will also not attempt to fix (3), because
>>>>>>>>>>>>>>>>>> this starts to interfere with (2) - sorry :-/.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> QUESTION: Does the plan from the previous paragraph sound
>>>>>>>>>>>>>>>>>> good to you?
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Yes; I agree that failing to look through ElaboratedType
>>>>>>>>>>>>>>>>> here is just a bug.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Ack.  I've marked "elaborated type" section in the doc as
>>>>>>>>>>>>>> something that is improved by the CL under review.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Experiments before my changes were done with the following
>>>>>>>>>>>>>>>>>> unit tests:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> // This testcase shows that Richard’s expectations are not
>>>>>>>>>>>>>>>>>> met *prior* to my CL.
>>>>>>>>>>>>>>>>>> // (i.e. the testcase below passes before my CL;  the
>>>>>>>>>>>>>>>>>> expectations in the testcase
>>>>>>>>>>>>>>>>>> // are the opposite of Richard’s expectations).
>>>>>>>>>>>>>>>>>> TEST(HasDeclaration, Case1FromEmail) {
>>>>>>>>>>>>>>>>>>   std::string input =
>>>>>>>>>>>>>>>>>>       "template<typename T>\n"
>>>>>>>>>>>>>>>>>>       "class Template {\n"
>>>>>>>>>>>>>>>>>>       " public:\n"
>>>>>>>>>>>>>>>>>>       "  void Method() {}\n"
>>>>>>>>>>>>>>>>>>       "};\n"
>>>>>>>>>>>>>>>>>>       "template <typename U>\n"
>>>>>>>>>>>>>>>>>>       "void Function(Template<U> param) {\n"
>>>>>>>>>>>>>>>>>>       "  param.Method();\n"



--
Thanks,

Lukasz

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Next steps for D24361 (patch to fix hasDeclaration wrt ElaboratedType)

Ivan Garramona via cfe-dev
I'm still working on a patch. Sorry for the delay, I'm currently traveling...

On Mon, Nov 7, 2016 at 6:46 PM Łukasz Anforowicz <[hidden email]> wrote:
On Mon, Oct 31, 2016 at 7:18 AM, Aaron Ballman <[hidden email]> wrote:
On Fri, Oct 28, 2016 at 12:17 PM, Manuel Klimek <[hidden email]> wrote:
> Playing around with the code, we have 2 problems we need to solve:
> a) the confusion between type & qualType weirdness
> That got in by accident, I think, though the getDecl() specialization that
> behaves differently from the partial desugaring we have due to b)
> b) the partial desugaring we do
> This basically was born from the idea that some of the sugaring classes we
> have are really really weird for devs (like TemplateSpecializationType) or
> basically useless to match (like ElaboratedType, where we can match on the
> NNS if we need to).
>
> a) seems fixable by a more fundamental patch in HasDeclarationMatcher on
> which I'm working
> b) seems like something we should discuss

I agree, thank you for looking into this and working on it!

~Aaron

>
> On Fri, Oct 28, 2016 at 4:27 PM Manuel Klimek <[hidden email]> wrote:
>>
>> Now with the right cfe-dev
>>
>>
>> On Fri, Oct 28, 2016 at 4:14 PM Manuel Klimek <[hidden email]> wrote:
>>>
>>> Somehow we lost cfe-dev on this mail thread :( :(
>>>
>>> adding Aaron, who has also contributed to that part of the code
>>>
>>> On Fri, Oct 28, 2016 at 4:11 PM Manuel Klimek <[hidden email]> wrote:
>>>>
>>>> On Fri, Oct 28, 2016 at 3:54 PM Łukasz Anforowicz <[hidden email]>
>>>> wrote:
>>>>>
>>>>> Some notes from me (that hopefully will helpful to consider during the
>>>>> meeting):
>>>>>
>>>>> I agree that forcing hasDeclaration to consider all subclasses of Types
>>>>> (i.e. forcing via #include "clang/AST/TypeNodes.def" in the proposal below)
>>>>> seems to be a good thing.  The subset of types supported today seems to be
>>>>> rather arbitrary (I think;  please let me know if there is good reasoning +
>>>>> a way to describe the currently chosen subset of supported types).
>>>>> As a user of hasDeclaration, I have to "manually" drill through *some*
>>>>> can paints today.  For example in
>>>>> https://codereview.chromium.org/2256913002/patch/180001/190001 I am manually
>>>>> jumping over pointers and references (what I want to do is to tell whether a
>>>>> member dereference will end up using one of "my" classdecls where I am
>>>>> renaming methods and fields) - see |blink_qual_type_matcher| in that
>>>>> Chromium code review.  Manually stripping pointers and references is doable
>>>>> but 1) it is easy to get wrong (i.e. strip only one level) and 2) it is
>>>>> questionable whether other required stripping is not missed (i.e. auto
>>>>> type?),
>>>>> Today hasDeclaration doesn't match "the outermost can in our russian
>>>>> doll" as suggested (or proposed) by Manual.  Today desugaring is done in an
>>>>> arbitrary order of types - some (deep) types will be preferred over other
>>>>> (shallow) types as shown in an example in the separate doc.  In the example,
>>>>> even though the typeAliasDecl is the most shallow desugaring result, it will
>>>>> *never* be matched because hasDeclaration will first try (and latch onto)
>>>>> other types first.
>>>>> "has" vs "hasDescendant" is not a perfectly valid analogy.
>>>>> "hasDeclaration" is not like "has" today, because even today it can strip
>>>>> multiple layers of sugar (see the example from the previous bullet item).
>>>>>
>>>>> So:
>>>>>
>>>>> I would vote to make "hasDeclaration" match at every desugaring level
>>>>> (i.e. without splitting it into a "has" and a "hasDescendant"-like variant).
>>>>> Having "hasDeclaration" and "eventuallyDesugarsToDeclaration"
>>>>> ("hasDeepDeclaration"?) sounds like an okay option, but it seems to me that
>>>>> even today 1) hasDeclaration code and 2) users of hasDeclaration, behave
>>>>> like or expect behavior of "eventuallyDesugarsToDeclaration"
>>>>
>>>> I'm still looking into where we made the fundamental error that we're
>>>> running into those discussions now. There should be something simpler so
>>>> that the behavior more clearly matches the design of the clang AST.
>>>>>
>>>>> I hope that the discussion of "hasDeclaration" vs "hasDeepDeclaration"
>>>>> can be independent from, and not block the fixes from
>>>>> https://reviews.llvm.org/D24361 (i.e. being able to land them in the next
>>>>> 1-2 would be great).
>>>>
>>>> As Richard noted on the doc, it really depends on which direction we
>>>> want to go in, and whether the patch is a step in the right direction. Sorry
>>>> :(
>>>> But please continue pinging / making sure we drive this forward - I
>>>> think this is important, and I'm happy to spend some time to make sure we
>>>> get this right, and thanks for all the work you've already put into this, I
>>>> really appreciate it, and I think it's helping us getting to the root of the
>>>> problem.

Ping :-)  Any progress here?  (both in 1) the general direction discussion as well as 2) the short-term planning/impact for ElaboratedType handling in https://reviews.llvm.org/D24361 ?) 

>>>>
>>>>>
>>>>>
>>>>> On Thu, Oct 27, 2016 at 1:12 AM, Manuel Klimek <[hidden email]>
>>>>> wrote:
>>>>>>
>>>>>> +ben & sam
>>>>>>
>>>>>> On Wed, Oct 26, 2016 at 6:50 PM Łukasz Anforowicz <[hidden email]>
>>>>>> wrote:
>>>>>>>
>>>>>>> On Wed, Oct 26, 2016 at 9:46 AM, Łukasz Anforowicz
>>>>>>> <[hidden email]> wrote:
>>>>>>>>
>>>>>>>> So - is it fair to say that the CL under review can proceed as-is
>>>>>>>> (*) given that:
>>>>>>>>
>>>>>>>> The ordering problem has existed *before* the CL under review
>>>>>>>> (admittedly the CL under review adds 2 more cases to the list of problematic
>>>>>>>> ordering scenarios)
>>>>>>>> Below it seems that we have a promising direction for solving the
>>>>>>>> ordering problem (the solution probably requires a slightly bigger CL - one
>>>>>>>> that also tackles the question of what hasDeclaration should do about
>>>>>>>> AutoType, PointerType, etc.)
>>>>>>>>
>>>>>>>> -Lukasz
>>>>>>>>
>>>>>>>> (*) I know that I need to add documentation changes to mention
>>>>>>>> ElaboratedType
>>>>>>>>
>>>>>>>> On Tue, Oct 25, 2016 at 5:02 PM, Richard Smith
>>>>>>>> <[hidden email]> wrote:
>>>>>>>>>
>>>>>>>>> On 25 October 2016 at 16:47, Łukasz Anforowicz <[hidden email]>
>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>> I think I understand the concern - the concern is that desugaring
>>>>>>>>>> done in HasDeclarationMatcher::matchesSpecialized(const QualType&, ...)
>>>>>>>>>> *latches* onto desugared types in *arbitrary* order.  I agree that this is a
>>>>>>>>>> problem (most obvious for me for the typeAliasDecl and
>>>>>>>>>> classTemplateSpecializationDecl case).  I agree that the CL adds a new case
>>>>>>>>>> to the set of ordered desugaring attempts done by this method (and therefore
>>>>>>>>>> can be seen as making the old problem slightly worse).
>>>>>>>>>>
>>>>>>>>>> I think this problem can be fixed by getting rid of the *order*
>>>>>>>>>> inside HasDeclarationMatcher::matchesSpecialized(const QualType&, ...) -
>>>>>>>>>> instead of:
>>>>>>>>>>
>>>>>>>>>> if (auto *TD = Node->getAsTagDecl())
>>>>>>>>>>   return matchesDecl(TD, Finder, Builder);
>>>>>>>>>> else if (auto *TT = Node->getAs<TypedefType>())
>>>>>>>>>>   return matchesDecl(TT->getDecl(), Finder, Builder);
>>>>>>>>>>
>>>>>>>>>> /* the CL under review would add ElaboratedType and
>>>>>>>>>>    TemplateSpecializationType somewhere here */
>>>>>>>>>>
>>>>>>>>>> ...
>>>>>>>>>> else if (auto *ICNT = Node->getAs<InjectedClassNameType>())
>>>>>>>>>>   return matchesDecl(ICNT->getDecl(), Finder, Builder);
>>>>>>>>>> return false;
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> have something like this:
>>>>>>>>>>
>>>>>>>>>>     if (auto *TD = Node->getAsTagDecl()) {
>>>>>>>>>>       if (matchesDecl(TD, Finder, Builder))
>>>>>>>>>>         return true;
>>>>>>>>>>     }
>>>>>>>>>>
>>>>>>>>>>     if (auto *TT = Node->getAs<TypedefType>()) {
>>>>>>>>>>       if (matchesDecl(TT->getDecl(), Finder, Builder))
>>>>>>>>>>         return true;
>>>>>>>>>>     }
>>>>>>>>>>
>>>>>>>>>>     if (auto *ET = Node->getAs<ElaboratedType>()) {
>>>>>>>>>>       if (matchesSpecialized(ET->getNamedType(), Finder, Builder))
>>>>>>>>>>         return true;
>>>>>>>>>>     }
>>>>>>>>>>
>>>>>>>>>>     if (auto *TST = Node->getAs<TemplateSpecializationType>()) {
>>>>>>>>>>       if (matchesSpecialized(*TST, Finder, Builder))
>>>>>>>>>>         return true;
>>>>>>>>>>     }
>>>>>>>>>> ...
>>>>>>>>>>     if (auto *ICNT = Node->getAs<InjectedClassNameType>()) {
>>>>>>>>>>       if (matchesDecl(ICNT->getDecl(), Finder, Builder))
>>>>>>>>>>         return true;
>>>>>>>>>>     }
>>>>>>>>>>
>>>>>>>>>>     return false;
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I think this is an interesting idea. It would certainly address my
>>>>>>>>> concerns. (You'd actually want something slightly different from the above,
>>>>>>>>> I think: given
>>>>>>>>>
>>>>>>>>>   typedef int A;
>>>>>>>>>   typedef A B;
>>>>>>>>>   B x;
>>>>>>>>>
>>>>>>>>> ...
>>>>>>>>> varDecl(hasType(qualType(hasDeclaration(typedefNameDecl().bind("TD")))))
>>>>>>>>> should match x /twice/: once with TD pointing at A and once with it pointing
>>>>>>>>> at B. So you probably want to repeatedly single-step desugar the type and
>>>>>>>>> try extracting a declaration from each level of type sugar.)
>>>>>>>>
>>>>>>>>
>>>>>>>> Oh, good point - I haven't thought about this scenario.  I think
>>>>>>>> this direction (based on getAsSuger from Type.cpp) look promising (passes a
>>>>>>>> test with 2 chained typedefs):
>>>>>>>>
>>>>>>>> template <typename T, typename DeclMatcherT>
>>>>>>>> class HasDeclarationMatcher : public WrapperMatcherInterface<T> {
>>>>>>>> ...
>>>>>>>>
>>>>>>>>   /// \brief Extracts the TagDecl of a QualType and returns whether
>>>>>>>> the inner
>>>>>>>>   /// matcher matches on it.
>>>>>>>>   bool matchesSpecialized(const QualType &Node, ASTMatchFinder
>>>>>>>> *Finder,
>>>>>>>>                           BoundNodesTreeBuilder *Builder) const {
>>>>>>>>     if (Node.isNull())
>>>>>>>>       return false;
>>>>>>>>
>>>>>>>>     // Do not use getAs<TemplateTypeParmType> instead of the direct
>>>>>>>> dyn_cast.
>>>>>>>>     // Calling getAs will return the canonical type, but that type
>>>>>>>> does not
>>>>>>>>     // store a TemplateTypeParmDecl. We *need* the uncanonical type,
>>>>>>>> if it is
>>>>>>>>     // available, and using dyn_cast ensures that.
>>>>>>>>     if (auto *TTP =
>>>>>>>> dyn_cast<TemplateTypeParmType>(Node.getTypePtr())) {
>>>>>>>>       if (matchesDecl(TTP->getDecl(), Finder, Builder))
>>>>>>>>         return true;
>>>>>>>>     }
>>>>>>>>
>>>>>>>>     const Type* Cur = Node.getTypePtr();
>>>>>>>>     while (Cur) {
>>>>>>>>       switch (Cur->getTypeClass()) {
>>>>>>>> #define ABSTRACT_TYPE(Class, Parent)
>>>>>>>> #define TYPE(Class, Parent) \
>>>>>>>>       case Type::Class: { \
>>>>>>>>         const Class##Type *Ty = cast<Class##Type>(Cur); \
>>>>>>>>         if (matchesSpecialized(*Ty, Finder, Builder)) \
>>>>>>>>           return true; \
>>>>>>>>         if (!Ty->isSugared()) return false; \
>>>>>>>>         Cur = Ty->desugar().getTypePtr(); \
>>>>>>>>         break; \
>>>>>>>>       }
>>>>>>>> #include "clang/AST/TypeNodes.def"
>>>>>>>>       }
>>>>>>>>     }
>>>>>>>>
>>>>>>>>     return false;
>>>>>>>>   }
>>>>>>>>
>>>>>>>>
>>>>>>> BTW: I have to note that the code above is potentially O(N^2) where N
>>>>>>> is the length of the desugaring chain (because each desugaring step can
>>>>>>> potentially call back into the QualType overload).  This is obviously
>>>>>>> suboptimal.  We probably need to discuss this when we review / talk about
>>>>>>> landing something like this.
>>>>>>
>>>>>>
>>>>>> It's basically the same difference between has() and hasDescendant.
>>>>>> I think
>>>>>> a) if we go that route, we should make it explicit by having a matcher
>>>>>> that has it in its name
>>>>>> b) we need to be able to memoize at each step, so this needs to become
>>>>>> part of the visitor interface of the matchers; given the sheer number of
>>>>>> types this seems like it'll get rather complex
>>>>>>
>>>>>> I'll make sure to discuss this with Richard etc al in person next week
>>>>>> when we're at LLVM conf.
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> but it opens a whole bunch of questions - what to do with other
>>>>>>>> types - for example should hasDeclaration "jump over / drill holes in paint
>>>>>>>> cans" for:
>>>>>>>>
>>>>>>>> AutoType, TypeOfExprType, DecltypeType, TypeOfType
>>>>>>>> LValueReferenceType, ValueReferenceType
>>>>>>>> PointerType, ReferenceType
>>>>>>>> And all the other types where I had to add a matchSpecialized
>>>>>>>> overload for so that things compile (some of these obviously don't have a
>>>>>>>> decl [and therefore shouldn't match hasDeclaration(...)] - like BuiltinType,
>>>>>>>> but I am listing all of them for completeness): AtomicType, PipeType,
>>>>>>>> ObjCObjectPointerType, ObjCObjectType, PackExpansionType,
>>>>>>>> DependentTemplateSpecializationType, DependentNameType, AttributedType,
>>>>>>>> UnaryTransformType, SubstTemplateTypeParmPackType,
>>>>>>>> SubstTemplateTypeParmType, AdjustedType, DecayedType, ParenType,
>>>>>>>> FunctionNoProtoType, FunctionProtoType, ExtVectorType, VectorType,
>>>>>>>> DependentSizedExtVectorType, DependentSizedArrayType, VariableArrayType,
>>>>>>>> IncompleteArrayType, ConstantArrayType, MemberPointerType, BlockPointerType,
>>>>>>>> ComplexType, BuiltinType,
>>>>>>>>
>>>>>>>> Also - I am not quite sure if I understand if special-casing of
>>>>>>>> TemplateTypeParmType is needed.  It probably is (because here we use
>>>>>>>> dyn_cast rather than cast).  At any rate drilling into details here can
>>>>>>>> probably wait until a real review in Phabricator...
>>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> The code above passes all tests from the clang-test ninja target +
>>>>>>>>>> results in the following behavior for the 2 cases pointed out in an email:
>>>>>>>>>>
>>>>>>>>>> TEST(HasDeclaration, Case1FromEmail) {
>>>>>>>>>>   std::string input =
>>>>>>>>>>       "template<typename T>\n"
>>>>>>>>>>       "class Template {\n"
>>>>>>>>>>       " public:\n"
>>>>>>>>>>       "  void Method() {}\n"
>>>>>>>>>>       "};\n"
>>>>>>>>>>       "template <typename U>\n"
>>>>>>>>>>       "void Function(Template<U> param) {\n"
>>>>>>>>>>       "  param.Method();\n"
>>>>>>>>>>       "};\n";
>>>>>>>>>>
>>>>>>>>>>   // param type has-declaration matches class template decl?
>>>>>>>>>>   EXPECT_TRUE(matches(input, parmVarDecl(
>>>>>>>>>>       hasName("param"),
>>>>>>>>>>       hasType(qualType(
>>>>>>>>>>
>>>>>>>>>> hasDeclaration(decl(classTemplateDecl(hasName("Template")))))))));
>>>>>>>>>>
>>>>>>>>>>   // param type has-declaration matches class decl?
>>>>>>>>>>   EXPECT_FALSE(matches(input, parmVarDecl(
>>>>>>>>>>       hasName("param"),
>>>>>>>>>>       hasType(qualType(
>>>>>>>>>>
>>>>>>>>>> hasDeclaration(decl(cxxRecordDecl(hasName("Template")))))))));
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> TEST(HasDeclaration, Case2FromEmail) {
>>>>>>>>>>   std::string input =
>>>>>>>>>>       "template<typename T>\n"
>>>>>>>>>>       "class Template {\n"
>>>>>>>>>>       " public:\n"
>>>>>>>>>>       "  void Method() {}\n"
>>>>>>>>>>       "};\n"
>>>>>>>>>>       "void Function(Template<int> param) {\n"
>>>>>>>>>>       "  param.Method();\n"
>>>>>>>>>>       "};\n";
>>>>>>>>>>
>>>>>>>>>>   // param type has-declaration matches class template decl?
>>>>>>>>>>   EXPECT_TRUE(matches(input, parmVarDecl(
>>>>>>>>>>       hasName("param"),
>>>>>>>>>>       hasType(qualType(
>>>>>>>>>>
>>>>>>>>>> hasDeclaration(decl(classTemplateDecl(hasName("Template")))))))));
>>>>>>>>>>
>>>>>>>>>>   // param type has-declaration matches class decl?
>>>>>>>>>>   EXPECT_TRUE(matches(input, parmVarDecl(
>>>>>>>>>>       hasName("param"),
>>>>>>>>>>       hasType(qualType(
>>>>>>>>>>
>>>>>>>>>> hasDeclaration(decl(cxxRecordDecl(hasName("Template")))))))));
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> If the test expectations above look reasonable, then we can just
>>>>>>>>>> discuss how to land the code changes above (e.g. as a single CL or 2 CLs - I
>>>>>>>>>> would prefer 2 separate CLs).  Otherwise, let's talk about the test
>>>>>>>>>> expectations - what should they look like / how should they be different
>>>>>>>>>> from the test snippet above?
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On Tue, Oct 25, 2016 at 1:42 PM, Richard Smith
>>>>>>>>>> <[hidden email]> wrote:
>>>>>>>>>>>
>>>>>>>>>>> On 25 October 2016 at 01:57, Manuel Klimek <[hidden email]>
>>>>>>>>>>> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> On Tue, Oct 25, 2016 at 3:25 AM Richard Smith
>>>>>>>>>>>> <[hidden email]> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 24 October 2016 at 16:41, Łukasz Anforowicz
>>>>>>>>>>>>> <[hidden email]> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I am not sure if we are making progress, but so far, we have
>>>>>>>>>>>>>> seen 1) test cases that are broken and fixed by the CL under review and 2)
>>>>>>>>>>>>>> test cases that are not changed by the CL under review and 3) no test cases
>>>>>>>>>>>>>> that are made worse by the CL under review.  I've tried to gather the
>>>>>>>>>>>>>> scenarios and the associated test cases in
>>>>>>>>>>>>>> https://docs.google.com/document/d/1y1F6uKdulluYBFch680g6bmHe_zDe8mHFYjfXRoakNI/edit.
>>>>>>>>>>>>>> Could you please take a look and shout if I misrepresented anything (i.e.
>>>>>>>>>>>>>> whether we agree that a specific test scenario shows a bug).
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I've also added some comments below / inline.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On Mon, Oct 24, 2016 at 3:54 PM, Richard Smith
>>>>>>>>>>>>>> <[hidden email]> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 24 October 2016 at 15:25, Łukasz Anforowicz
>>>>>>>>>>>>>>> <[hidden email]> wrote:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On Mon, Oct 24, 2016 at 2:21 PM, Richard Smith
>>>>>>>>>>>>>>>> <[hidden email]> wrote:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> On 24 October 2016 at 11:23, Łukasz Anforowicz
>>>>>>>>>>>>>>>>> <[hidden email]> wrote:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> I've run a few more experiments and I see that before any
>>>>>>>>>>>>>>>>>> changes:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> (1) Presence of ElaboratedType means that hasDeclaration
>>>>>>>>>>>>>>>>>> doesn’t match *anything*.  This is clearly wrong.  I still want to fix that.
>>>>>>>>>>>>>>>>>> (2) Both examples from Richard’s email consistently match
>>>>>>>>>>>>>>>>>> the class template decl (and not the class decl).  If Richard’s expectations
>>>>>>>>>>>>>>>>>> are not met *prior* to my CL, then I don’t think these expectations should
>>>>>>>>>>>>>>>>>> be fixed by my CL.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> OTOH, maybe it is worth to open a bug to investigate this
>>>>>>>>>>>>>>>>>> more - personally I would argue that hasDeclaration should match *any* decl
>>>>>>>>>>>>>>>>>> - for example any of type alias / template class decl / class decl.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> (3) If desugaring can result in both
>>>>>>>>>>>>>>>>>> TemplateSpecializationType and TypedefType, then only one is arbitrarily
>>>>>>>>>>>>>>>>>> picked - see test case TypedefTypeAndTemplateSpecializationTypeAreBothHere.
>>>>>>>>>>>>>>>>>> Again - this problem exists *prior* to my CL.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> So - my plan right now is to just simplify/revert my
>>>>>>>>>>>>>>>>>> patches so they only fix (1).  I don't think discussion of (2) should impact
>>>>>>>>>>>>>>>>>> the CL, because the behavior that disagrees with Richard's expectations is
>>>>>>>>>>>>>>>>>> happening *before* my changes.  I will also not attempt to fix (3), because
>>>>>>>>>>>>>>>>>> this starts to interfere with (2) - sorry :-/.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> QUESTION: Does the plan from the previous paragraph sound
>>>>>>>>>>>>>>>>>> good to you?
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Yes; I agree that failing to look through ElaboratedType
>>>>>>>>>>>>>>>>> here is just a bug.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Ack.  I've marked "elaborated type" section in the doc as
>>>>>>>>>>>>>> something that is improved by the CL under review.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Experiments before my changes were done with the following
>>>>>>>>>>>>>>>>>> unit tests:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> // This testcase shows that Richard’s expectations are not
>>>>>>>>>>>>>>>>>> met *prior* to my CL.
>>>>>>>>>>>>>>>>>> // (i.e. the testcase below passes before my CL;  the
>>>>>>>>>>>>>>>>>> expectations in the testcase
>>>>>>>>>>>>>>>>>> // are the opposite of Richard’s expectations).
>>>>>>>>>>>>>>>>>> TEST(HasDeclaration, Case1FromEmail) {
>>>>>>>>>>>>>>>>>>   std::string input =
>>>>>>>>>>>>>>>>>>       "template<typename T>\n"
>>>>>>>>>>>>>>>>>>       "class Template {\n"
>>>>>>>>>>>>>>>>>>       " public:\n"
>>>>>>>>>>>>>>>>>>       "  void Method() {}\n"
>>>>>>>>>>>>>>>>>>       "};\n"
>>>>>>>>>>>>>>>>>>       "template <typename U>\n"
>>>>>>>>>>>>>>>>>>       "void Function(Template<U> param) {\n"
>>>>>>>>>>>>>>>>>>       "  param.Method();\n"



--
Thanks,

Lukasz

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Next steps for D24361 (patch to fix hasDeclaration wrt ElaboratedType)

Ivan Garramona via cfe-dev
finished up my proposal here:
https://reviews.llvm.org/D27104

I don't think it resolves all of the issues yet, but I think it is the first step we should take...

On Tue, Nov 8, 2016 at 6:00 AM Manuel Klimek <[hidden email]> wrote:
I'm still working on a patch. Sorry for the delay, I'm currently traveling...

On Mon, Nov 7, 2016 at 6:46 PM Łukasz Anforowicz <[hidden email]> wrote:
On Mon, Oct 31, 2016 at 7:18 AM, Aaron Ballman <[hidden email]> wrote:
On Fri, Oct 28, 2016 at 12:17 PM, Manuel Klimek <[hidden email]> wrote:
> Playing around with the code, we have 2 problems we need to solve:
> a) the confusion between type & qualType weirdness
> That got in by accident, I think, though the getDecl() specialization that
> behaves differently from the partial desugaring we have due to b)
> b) the partial desugaring we do
> This basically was born from the idea that some of the sugaring classes we
> have are really really weird for devs (like TemplateSpecializationType) or
> basically useless to match (like ElaboratedType, where we can match on the
> NNS if we need to).
>
> a) seems fixable by a more fundamental patch in HasDeclarationMatcher on
> which I'm working
> b) seems like something we should discuss

I agree, thank you for looking into this and working on it!

~Aaron

>
> On Fri, Oct 28, 2016 at 4:27 PM Manuel Klimek <[hidden email]> wrote:
>>
>> Now with the right cfe-dev
>>
>>
>> On Fri, Oct 28, 2016 at 4:14 PM Manuel Klimek <[hidden email]> wrote:
>>>
>>> Somehow we lost cfe-dev on this mail thread :( :(
>>>
>>> adding Aaron, who has also contributed to that part of the code
>>>
>>> On Fri, Oct 28, 2016 at 4:11 PM Manuel Klimek <[hidden email]> wrote:
>>>>
>>>> On Fri, Oct 28, 2016 at 3:54 PM Łukasz Anforowicz <[hidden email]>
>>>> wrote:
>>>>>
>>>>> Some notes from me (that hopefully will helpful to consider during the
>>>>> meeting):
>>>>>
>>>>> I agree that forcing hasDeclaration to consider all subclasses of Types
>>>>> (i.e. forcing via #include "clang/AST/TypeNodes.def" in the proposal below)
>>>>> seems to be a good thing.  The subset of types supported today seems to be
>>>>> rather arbitrary (I think;  please let me know if there is good reasoning +
>>>>> a way to describe the currently chosen subset of supported types).
>>>>> As a user of hasDeclaration, I have to "manually" drill through *some*
>>>>> can paints today.  For example in
>>>>> https://codereview.chromium.org/2256913002/patch/180001/190001 I am manually
>>>>> jumping over pointers and references (what I want to do is to tell whether a
>>>>> member dereference will end up using one of "my" classdecls where I am
>>>>> renaming methods and fields) - see |blink_qual_type_matcher| in that
>>>>> Chromium code review.  Manually stripping pointers and references is doable
>>>>> but 1) it is easy to get wrong (i.e. strip only one level) and 2) it is
>>>>> questionable whether other required stripping is not missed (i.e. auto
>>>>> type?),
>>>>> Today hasDeclaration doesn't match "the outermost can in our russian
>>>>> doll" as suggested (or proposed) by Manual.  Today desugaring is done in an
>>>>> arbitrary order of types - some (deep) types will be preferred over other
>>>>> (shallow) types as shown in an example in the separate doc.  In the example,
>>>>> even though the typeAliasDecl is the most shallow desugaring result, it will
>>>>> *never* be matched because hasDeclaration will first try (and latch onto)
>>>>> other types first.
>>>>> "has" vs "hasDescendant" is not a perfectly valid analogy.
>>>>> "hasDeclaration" is not like "has" today, because even today it can strip
>>>>> multiple layers of sugar (see the example from the previous bullet item).
>>>>>
>>>>> So:
>>>>>
>>>>> I would vote to make "hasDeclaration" match at every desugaring level
>>>>> (i.e. without splitting it into a "has" and a "hasDescendant"-like variant).
>>>>> Having "hasDeclaration" and "eventuallyDesugarsToDeclaration"
>>>>> ("hasDeepDeclaration"?) sounds like an okay option, but it seems to me that
>>>>> even today 1) hasDeclaration code and 2) users of hasDeclaration, behave
>>>>> like or expect behavior of "eventuallyDesugarsToDeclaration"
>>>>
>>>> I'm still looking into where we made the fundamental error that we're
>>>> running into those discussions now. There should be something simpler so
>>>> that the behavior more clearly matches the design of the clang AST.
>>>>>
>>>>> I hope that the discussion of "hasDeclaration" vs "hasDeepDeclaration"
>>>>> can be independent from, and not block the fixes from
>>>>> https://reviews.llvm.org/D24361 (i.e. being able to land them in the next
>>>>> 1-2 would be great).
>>>>
>>>> As Richard noted on the doc, it really depends on which direction we
>>>> want to go in, and whether the patch is a step in the right direction. Sorry
>>>> :(
>>>> But please continue pinging / making sure we drive this forward - I
>>>> think this is important, and I'm happy to spend some time to make sure we
>>>> get this right, and thanks for all the work you've already put into this, I
>>>> really appreciate it, and I think it's helping us getting to the root of the
>>>> problem.

Ping :-)  Any progress here?  (both in 1) the general direction discussion as well as 2) the short-term planning/impact for ElaboratedType handling in https://reviews.llvm.org/D24361 ?) 

>>>>
>>>>>
>>>>>
>>>>> On Thu, Oct 27, 2016 at 1:12 AM, Manuel Klimek <[hidden email]>
>>>>> wrote:
>>>>>>
>>>>>> +ben & sam
>>>>>>
>>>>>> On Wed, Oct 26, 2016 at 6:50 PM Łukasz Anforowicz <[hidden email]>
>>>>>> wrote:
>>>>>>>
>>>>>>> On Wed, Oct 26, 2016 at 9:46 AM, Łukasz Anforowicz
>>>>>>> <[hidden email]> wrote:
>>>>>>>>
>>>>>>>> So - is it fair to say that the CL under review can proceed as-is
>>>>>>>> (*) given that:
>>>>>>>>
>>>>>>>> The ordering problem has existed *before* the CL under review
>>>>>>>> (admittedly the CL under review adds 2 more cases to the list of problematic
>>>>>>>> ordering scenarios)
>>>>>>>> Below it seems that we have a promising direction for solving the
>>>>>>>> ordering problem (the solution probably requires a slightly bigger CL - one
>>>>>>>> that also tackles the question of what hasDeclaration should do about
>>>>>>>> AutoType, PointerType, etc.)
>>>>>>>>
>>>>>>>> -Lukasz
>>>>>>>>
>>>>>>>> (*) I know that I need to add documentation changes to mention
>>>>>>>> ElaboratedType
>>>>>>>>
>>>>>>>> On Tue, Oct 25, 2016 at 5:02 PM, Richard Smith
>>>>>>>> <[hidden email]> wrote:
>>>>>>>>>
>>>>>>>>> On 25 October 2016 at 16:47, Łukasz Anforowicz <[hidden email]>
>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>> I think I understand the concern - the concern is that desugaring
>>>>>>>>>> done in HasDeclarationMatcher::matchesSpecialized(const QualType&, ...)
>>>>>>>>>> *latches* onto desugared types in *arbitrary* order.  I agree that this is a
>>>>>>>>>> problem (most obvious for me for the typeAliasDecl and
>>>>>>>>>> classTemplateSpecializationDecl case).  I agree that the CL adds a new case
>>>>>>>>>> to the set of ordered desugaring attempts done by this method (and therefore
>>>>>>>>>> can be seen as making the old problem slightly worse).
>>>>>>>>>>
>>>>>>>>>> I think this problem can be fixed by getting rid of the *order*
>>>>>>>>>> inside HasDeclarationMatcher::matchesSpecialized(const QualType&, ...) -
>>>>>>>>>> instead of:
>>>>>>>>>>
>>>>>>>>>> if (auto *TD = Node->getAsTagDecl())
>>>>>>>>>>   return matchesDecl(TD, Finder, Builder);
>>>>>>>>>> else if (auto *TT = Node->getAs<TypedefType>())
>>>>>>>>>>   return matchesDecl(TT->getDecl(), Finder, Builder);
>>>>>>>>>>
>>>>>>>>>> /* the CL under review would add ElaboratedType and
>>>>>>>>>>    TemplateSpecializationType somewhere here */
>>>>>>>>>>
>>>>>>>>>> ...
>>>>>>>>>> else if (auto *ICNT = Node->getAs<InjectedClassNameType>())
>>>>>>>>>>   return matchesDecl(ICNT->getDecl(), Finder, Builder);
>>>>>>>>>> return false;
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> have something like this:
>>>>>>>>>>
>>>>>>>>>>     if (auto *TD = Node->getAsTagDecl()) {
>>>>>>>>>>       if (matchesDecl(TD, Finder, Builder))
>>>>>>>>>>         return true;
>>>>>>>>>>     }
>>>>>>>>>>
>>>>>>>>>>     if (auto *TT = Node->getAs<TypedefType>()) {
>>>>>>>>>>       if (matchesDecl(TT->getDecl(), Finder, Builder))
>>>>>>>>>>         return true;
>>>>>>>>>>     }
>>>>>>>>>>
>>>>>>>>>>     if (auto *ET = Node->getAs<ElaboratedType>()) {
>>>>>>>>>>       if (matchesSpecialized(ET->getNamedType(), Finder, Builder))
>>>>>>>>>>         return true;
>>>>>>>>>>     }
>>>>>>>>>>
>>>>>>>>>>     if (auto *TST = Node->getAs<TemplateSpecializationType>()) {
>>>>>>>>>>       if (matchesSpecialized(*TST, Finder, Builder))
>>>>>>>>>>         return true;
>>>>>>>>>>     }
>>>>>>>>>> ...
>>>>>>>>>>     if (auto *ICNT = Node->getAs<InjectedClassNameType>()) {
>>>>>>>>>>       if (matchesDecl(ICNT->getDecl(), Finder, Builder))
>>>>>>>>>>         return true;
>>>>>>>>>>     }
>>>>>>>>>>
>>>>>>>>>>     return false;
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I think this is an interesting idea. It would certainly address my
>>>>>>>>> concerns. (You'd actually want something slightly different from the above,
>>>>>>>>> I think: given
>>>>>>>>>
>>>>>>>>>   typedef int A;
>>>>>>>>>   typedef A B;
>>>>>>>>>   B x;
>>>>>>>>>
>>>>>>>>> ...
>>>>>>>>> varDecl(hasType(qualType(hasDeclaration(typedefNameDecl().bind("TD")))))
>>>>>>>>> should match x /twice/: once with TD pointing at A and once with it pointing
>>>>>>>>> at B. So you probably want to repeatedly single-step desugar the type and
>>>>>>>>> try extracting a declaration from each level of type sugar.)
>>>>>>>>
>>>>>>>>
>>>>>>>> Oh, good point - I haven't thought about this scenario.  I think
>>>>>>>> this direction (based on getAsSuger from Type.cpp) look promising (passes a
>>>>>>>> test with 2 chained typedefs):
>>>>>>>>
>>>>>>>> template <typename T, typename DeclMatcherT>
>>>>>>>> class HasDeclarationMatcher : public WrapperMatcherInterface<T> {
>>>>>>>> ...
>>>>>>>>
>>>>>>>>   /// \brief Extracts the TagDecl of a QualType and returns whether
>>>>>>>> the inner
>>>>>>>>   /// matcher matches on it.
>>>>>>>>   bool matchesSpecialized(const QualType &Node, ASTMatchFinder
>>>>>>>> *Finder,
>>>>>>>>                           BoundNodesTreeBuilder *Builder) const {
>>>>>>>>     if (Node.isNull())
>>>>>>>>       return false;
>>>>>>>>
>>>>>>>>     // Do not use getAs<TemplateTypeParmType> instead of the direct
>>>>>>>> dyn_cast.
>>>>>>>>     // Calling getAs will return the canonical type, but that type
>>>>>>>> does not
>>>>>>>>     // store a TemplateTypeParmDecl. We *need* the uncanonical type,
>>>>>>>> if it is
>>>>>>>>     // available, and using dyn_cast ensures that.
>>>>>>>>     if (auto *TTP =
>>>>>>>> dyn_cast<TemplateTypeParmType>(Node.getTypePtr())) {
>>>>>>>>       if (matchesDecl(TTP->getDecl(), Finder, Builder))
>>>>>>>>         return true;
>>>>>>>>     }
>>>>>>>>
>>>>>>>>     const Type* Cur = Node.getTypePtr();
>>>>>>>>     while (Cur) {
>>>>>>>>       switch (Cur->getTypeClass()) {
>>>>>>>> #define ABSTRACT_TYPE(Class, Parent)
>>>>>>>> #define TYPE(Class, Parent) \
>>>>>>>>       case Type::Class: { \
>>>>>>>>         const Class##Type *Ty = cast<Class##Type>(Cur); \
>>>>>>>>         if (matchesSpecialized(*Ty, Finder, Builder)) \
>>>>>>>>           return true; \
>>>>>>>>         if (!Ty->isSugared()) return false; \
>>>>>>>>         Cur = Ty->desugar().getTypePtr(); \
>>>>>>>>         break; \
>>>>>>>>       }
>>>>>>>> #include "clang/AST/TypeNodes.def"
>>>>>>>>       }
>>>>>>>>     }
>>>>>>>>
>>>>>>>>     return false;
>>>>>>>>   }
>>>>>>>>
>>>>>>>>
>>>>>>> BTW: I have to note that the code above is potentially O(N^2) where N
>>>>>>> is the length of the desugaring chain (because each desugaring step can
>>>>>>> potentially call back into the QualType overload).  This is obviously
>>>>>>> suboptimal.  We probably need to discuss this when we review / talk about
>>>>>>> landing something like this.
>>>>>>
>>>>>>
>>>>>> It's basically the same difference between has() and hasDescendant.
>>>>>> I think
>>>>>> a) if we go that route, we should make it explicit by having a matcher
>>>>>> that has it in its name
>>>>>> b) we need to be able to memoize at each step, so this needs to become
>>>>>> part of the visitor interface of the matchers; given the sheer number of
>>>>>> types this seems like it'll get rather complex
>>>>>>
>>>>>> I'll make sure to discuss this with Richard etc al in person next week
>>>>>> when we're at LLVM conf.
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> but it opens a whole bunch of questions - what to do with other
>>>>>>>> types - for example should hasDeclaration "jump over / drill holes in paint
>>>>>>>> cans" for:
>>>>>>>>
>>>>>>>> AutoType, TypeOfExprType, DecltypeType, TypeOfType
>>>>>>>> LValueReferenceType, ValueReferenceType
>>>>>>>> PointerType, ReferenceType
>>>>>>>> And all the other types where I had to add a matchSpecialized
>>>>>>>> overload for so that things compile (some of these obviously don't have a
>>>>>>>> decl [and therefore shouldn't match hasDeclaration(...)] - like BuiltinType,
>>>>>>>> but I am listing all of them for completeness): AtomicType, PipeType,
>>>>>>>> ObjCObjectPointerType, ObjCObjectType, PackExpansionType,
>>>>>>>> DependentTemplateSpecializationType, DependentNameType, AttributedType,
>>>>>>>> UnaryTransformType, SubstTemplateTypeParmPackType,
>>>>>>>> SubstTemplateTypeParmType, AdjustedType, DecayedType, ParenType,
>>>>>>>> FunctionNoProtoType, FunctionProtoType, ExtVectorType, VectorType,
>>>>>>>> DependentSizedExtVectorType, DependentSizedArrayType, VariableArrayType,
>>>>>>>> IncompleteArrayType, ConstantArrayType, MemberPointerType, BlockPointerType,
>>>>>>>> ComplexType, BuiltinType,
>>>>>>>>
>>>>>>>> Also - I am not quite sure if I understand if special-casing of
>>>>>>>> TemplateTypeParmType is needed.  It probably is (because here we use
>>>>>>>> dyn_cast rather than cast).  At any rate drilling into details here can
>>>>>>>> probably wait until a real review in Phabricator...
>>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> The code above passes all tests from the clang-test ninja target +
>>>>>>>>>> results in the following behavior for the 2 cases pointed out in an email:
>>>>>>>>>>
>>>>>>>>>> TEST(HasDeclaration, Case1FromEmail) {
>>>>>>>>>>   std::string input =
>>>>>>>>>>       "template<typename T>\n"
>>>>>>>>>>       "class Template {\n"
>>>>>>>>>>       " public:\n"
>>>>>>>>>>       "  void Method() {}\n"
>>>>>>>>>>       "};\n"
>>>>>>>>>>       "template <typename U>\n"
>>>>>>>>>>       "void Function(Template<U> param) {\n"
>>>>>>>>>>       "  param.Method();\n"
>>>>>>>>>>       "};\n";
>>>>>>>>>>
>>>>>>>>>>   // param type has-declaration matches class template decl?
>>>>>>>>>>   EXPECT_TRUE(matches(input, parmVarDecl(
>>>>>>>>>>       hasName("param"),
>>>>>>>>>>       hasType(qualType(
>>>>>>>>>>
>>>>>>>>>> hasDeclaration(decl(classTemplateDecl(hasName("Template")))))))));
>>>>>>>>>>
>>>>>>>>>>   // param type has-declaration matches class decl?
>>>>>>>>>>   EXPECT_FALSE(matches(input, parmVarDecl(
>>>>>>>>>>       hasName("param"),
>>>>>>>>>>       hasType(qualType(
>>>>>>>>>>
>>>>>>>>>> hasDeclaration(decl(cxxRecordDecl(hasName("Template")))))))));
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> TEST(HasDeclaration, Case2FromEmail) {
>>>>>>>>>>   std::string input =
>>>>>>>>>>       "template<typename T>\n"
>>>>>>>>>>       "class Template {\n"
>>>>>>>>>>       " public:\n"
>>>>>>>>>>       "  void Method() {}\n"
>>>>>>>>>>       "};\n"
>>>>>>>>>>       "void Function(Template<int> param) {\n"
>>>>>>>>>>       "  param.Method();\n"
>>>>>>>>>>       "};\n";
>>>>>>>>>>
>>>>>>>>>>   // param type has-declaration matches class template decl?
>>>>>>>>>>   EXPECT_TRUE(matches(input, parmVarDecl(
>>>>>>>>>>       hasName("param"),
>>>>>>>>>>       hasType(qualType(
>>>>>>>>>>
>>>>>>>>>> hasDeclaration(decl(classTemplateDecl(hasName("Template")))))))));
>>>>>>>>>>
>>>>>>>>>>   // param type has-declaration matches class decl?
>>>>>>>>>>   EXPECT_TRUE(matches(input, parmVarDecl(
>>>>>>>>>>       hasName("param"),
>>>>>>>>>>       hasType(qualType(
>>>>>>>>>>
>>>>>>>>>> hasDeclaration(decl(cxxRecordDecl(hasName("Template")))))))));
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> If the test expectations above look reasonable, then we can just
>>>>>>>>>> discuss how to land the code changes above (e.g. as a single CL or 2 CLs - I
>>>>>>>>>> would prefer 2 separate CLs).  Otherwise, let's talk about the test
>>>>>>>>>> expectations - what should they look like / how should they be different
>>>>>>>>>> from the test snippet above?
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On Tue, Oct 25, 2016 at 1:42 PM, Richard Smith
>>>>>>>>>> <[hidden email]> wrote:
>>>>>>>>>>>
>>>>>>>>>>> On 25 October 2016 at 01:57, Manuel Klimek <[hidden email]>
>>>>>>>>>>> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> On Tue, Oct 25, 2016 at 3:25 AM Richard Smith
>>>>>>>>>>>> <[hidden email]> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 24 October 2016 at 16:41, Łukasz Anforowicz
>>>>>>>>>>>>> <[hidden email]> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I am not sure if we are making progress, but so far, we have
>>>>>>>>>>>>>> seen 1) test cases that are broken and fixed by the CL under review and 2)
>>>>>>>>>>>>>> test cases that are not changed by the CL under review and 3) no test cases
>>>>>>>>>>>>>> that are made worse by the CL under review.  I've tried to gather the
>>>>>>>>>>>>>> scenarios and the associated test cases in
>>>>>>>>>>>>>> https://docs.google.com/document/d/1y1F6uKdulluYBFch680g6bmHe_zDe8mHFYjfXRoakNI/edit.
>>>>>>>>>>>>>> Could you please take a look and shout if I misrepresented anything (i.e.
>>>>>>>>>>>>>> whether we agree that a specific test scenario shows a bug).
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I've also added some comments below / inline.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On Mon, Oct 24, 2016 at 3:54 PM, Richard Smith
>>>>>>>>>>>>>> <[hidden email]> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 24 October 2016 at 15:25, Łukasz Anforowicz
>>>>>>>>>>>>>>> <[hidden email]> wrote:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On Mon, Oct 24, 2016 at 2:21 PM, Richard Smith
>>>>>>>>>>>>>>>> <[hidden email]> wrote:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> On 24 October 2016 at 11:23, Łukasz Anforowicz
>>>>>>>>>>>>>>>>> <[hidden email]> wrote:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> I've run a few more experiments and I see that before any
>>>>>>>>>>>>>>>>>> changes:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> (1) Presence of ElaboratedType means that hasDeclaration
>>>>>>>>>>>>>>>>>> doesn’t match *anything*.  This is clearly wrong.  I still want to fix that.
>>>>>>>>>>>>>>>>>> (2) Both examples from Richard’s email consistently match
>>>>>>>>>>>>>>>>>> the class template decl (and not the class decl).  If Richard’s expectations
>>>>>>>>>>>>>>>>>> are not met *prior* to my CL, then I don’t think these expectations should
>>>>>>>>>>>>>>>>>> be fixed by my CL.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> OTOH, maybe it is worth to open a bug to investigate this
>>>>>>>>>>>>>>>>>> more - personally I would argue that hasDeclaration should match *any* decl
>>>>>>>>>>>>>>>>>> - for example any of type alias / template class decl / class decl.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> (3) If desugaring can result in both
>>>>>>>>>>>>>>>>>> TemplateSpecializationType and TypedefType, then only one is arbitrarily
>>>>>>>>>>>>>>>>>> picked - see test case TypedefTypeAndTemplateSpecializationTypeAreBothHere.
>>>>>>>>>>>>>>>>>> Again - this problem exists *prior* to my CL.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> So - my plan right now is to just simplify/revert my
>>>>>>>>>>>>>>>>>> patches so they only fix (1).  I don't think discussion of (2) should impact
>>>>>>>>>>>>>>>>>> the CL, because the behavior that disagrees with Richard's expectations is
>>>>>>>>>>>>>>>>>> happening *before* my changes.  I will also not attempt to fix (3), because
>>>>>>>>>>>>>>>>>> this starts to interfere with (2) - sorry :-/.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> QUESTION: Does the plan from the previous paragraph sound
>>>>>>>>>>>>>>>>>> good to you?
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Yes; I agree that failing to look through ElaboratedType
>>>>>>>>>>>>>>>>> here is just a bug.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Ack.  I've marked "elaborated type" section in the doc as
>>>>>>>>>>>>>> something that is improved by the CL under review.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Experiments before my changes were done with the following
>>>>>>>>>>>>>>>>>> unit tests:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> // This testcase shows that Richard’s expectations are not
>>>>>>>>>>>>>>>>>> met *prior* to my CL.
>>>>>>>>>>>>>>>>>> // (i.e. the testcase below passes before my CL;  the
>>>>>>>>>>>>>>>>>> expectations in the testcase
>>>>>>>>>>>>>>>>>> // are the opposite of Richard’s expectations).
>>>>>>>>>>>>>>>>>> TEST(HasDeclaration, Case1FromEmail) {
>>>>>>>>>>>>>>>>>>   std::string input =
>>>>>>>>>>>>>>>>>>       "template<typename T>\n"
>>>>>>>>>>>>>>>>>>       "class Template {\n"
>>>>>>>>>>>>>>>>>>       " public:\n"
>>>>>>>>>>>>>>>>>>       "  void Method() {}\n"
>>>>>>>>>>>>>>>>>>       "};\n"
>>>>>>>>>>>>>>>>>>       "template <typename U>\n"
>>>>>>>>>>>>>>>>>>       "void Function(Template<U> param) {\n"
>>>>>>>>>>>>>>>>>>       "  param.Method();\n"



--
Thanks,

Lukasz

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Next steps for D24361 (patch to fix hasDeclaration wrt ElaboratedType)

Ivan Garramona via cfe-dev
+Sam who has also run into this.

On Thu, Nov 24, 2016 at 4:26 PM Manuel Klimek <[hidden email]> wrote:
finished up my proposal here:
https://reviews.llvm.org/D27104

I don't think it resolves all of the issues yet, but I think it is the first step we should take...

On Tue, Nov 8, 2016 at 6:00 AM Manuel Klimek <[hidden email]> wrote:
I'm still working on a patch. Sorry for the delay, I'm currently traveling...

On Mon, Nov 7, 2016 at 6:46 PM Łukasz Anforowicz <[hidden email]> wrote:
On Mon, Oct 31, 2016 at 7:18 AM, Aaron Ballman <[hidden email]> wrote:
On Fri, Oct 28, 2016 at 12:17 PM, Manuel Klimek <[hidden email]> wrote:
> Playing around with the code, we have 2 problems we need to solve:
> a) the confusion between type & qualType weirdness
> That got in by accident, I think, though the getDecl() specialization that
> behaves differently from the partial desugaring we have due to b)
> b) the partial desugaring we do
> This basically was born from the idea that some of the sugaring classes we
> have are really really weird for devs (like TemplateSpecializationType) or
> basically useless to match (like ElaboratedType, where we can match on the
> NNS if we need to).
>
> a) seems fixable by a more fundamental patch in HasDeclarationMatcher on
> which I'm working
> b) seems like something we should discuss

I agree, thank you for looking into this and working on it!

~Aaron

>
> On Fri, Oct 28, 2016 at 4:27 PM Manuel Klimek <[hidden email]> wrote:
>>
>> Now with the right cfe-dev
>>
>>
>> On Fri, Oct 28, 2016 at 4:14 PM Manuel Klimek <[hidden email]> wrote:
>>>
>>> Somehow we lost cfe-dev on this mail thread :( :(
>>>
>>> adding Aaron, who has also contributed to that part of the code
>>>
>>> On Fri, Oct 28, 2016 at 4:11 PM Manuel Klimek <[hidden email]> wrote:
>>>>
>>>> On Fri, Oct 28, 2016 at 3:54 PM Łukasz Anforowicz <[hidden email]>
>>>> wrote:
>>>>>
>>>>> Some notes from me (that hopefully will helpful to consider during the
>>>>> meeting):
>>>>>
>>>>> I agree that forcing hasDeclaration to consider all subclasses of Types
>>>>> (i.e. forcing via #include "clang/AST/TypeNodes.def" in the proposal below)
>>>>> seems to be a good thing.  The subset of types supported today seems to be
>>>>> rather arbitrary (I think;  please let me know if there is good reasoning +
>>>>> a way to describe the currently chosen subset of supported types).
>>>>> As a user of hasDeclaration, I have to "manually" drill through *some*
>>>>> can paints today.  For example in
>>>>> https://codereview.chromium.org/2256913002/patch/180001/190001 I am manually
>>>>> jumping over pointers and references (what I want to do is to tell whether a
>>>>> member dereference will end up using one of "my" classdecls where I am
>>>>> renaming methods and fields) - see |blink_qual_type_matcher| in that
>>>>> Chromium code review.  Manually stripping pointers and references is doable
>>>>> but 1) it is easy to get wrong (i.e. strip only one level) and 2) it is
>>>>> questionable whether other required stripping is not missed (i.e. auto
>>>>> type?),
>>>>> Today hasDeclaration doesn't match "the outermost can in our russian
>>>>> doll" as suggested (or proposed) by Manual.  Today desugaring is done in an
>>>>> arbitrary order of types - some (deep) types will be preferred over other
>>>>> (shallow) types as shown in an example in the separate doc.  In the example,
>>>>> even though the typeAliasDecl is the most shallow desugaring result, it will
>>>>> *never* be matched because hasDeclaration will first try (and latch onto)
>>>>> other types first.
>>>>> "has" vs "hasDescendant" is not a perfectly valid analogy.
>>>>> "hasDeclaration" is not like "has" today, because even today it can strip
>>>>> multiple layers of sugar (see the example from the previous bullet item).
>>>>>
>>>>> So:
>>>>>
>>>>> I would vote to make "hasDeclaration" match at every desugaring level
>>>>> (i.e. without splitting it into a "has" and a "hasDescendant"-like variant).
>>>>> Having "hasDeclaration" and "eventuallyDesugarsToDeclaration"
>>>>> ("hasDeepDeclaration"?) sounds like an okay option, but it seems to me that
>>>>> even today 1) hasDeclaration code and 2) users of hasDeclaration, behave
>>>>> like or expect behavior of "eventuallyDesugarsToDeclaration"
>>>>
>>>> I'm still looking into where we made the fundamental error that we're
>>>> running into those discussions now. There should be something simpler so
>>>> that the behavior more clearly matches the design of the clang AST.
>>>>>
>>>>> I hope that the discussion of "hasDeclaration" vs "hasDeepDeclaration"
>>>>> can be independent from, and not block the fixes from
>>>>> https://reviews.llvm.org/D24361 (i.e. being able to land them in the next
>>>>> 1-2 would be great).
>>>>
>>>> As Richard noted on the doc, it really depends on which direction we
>>>> want to go in, and whether the patch is a step in the right direction. Sorry
>>>> :(
>>>> But please continue pinging / making sure we drive this forward - I
>>>> think this is important, and I'm happy to spend some time to make sure we
>>>> get this right, and thanks for all the work you've already put into this, I
>>>> really appreciate it, and I think it's helping us getting to the root of the
>>>> problem.

Ping :-)  Any progress here?  (both in 1) the general direction discussion as well as 2) the short-term planning/impact for ElaboratedType handling in https://reviews.llvm.org/D24361 ?) 

>>>>
>>>>>
>>>>>
>>>>> On Thu, Oct 27, 2016 at 1:12 AM, Manuel Klimek <[hidden email]>
>>>>> wrote:
>>>>>>
>>>>>> +ben & sam
>>>>>>
>>>>>> On Wed, Oct 26, 2016 at 6:50 PM Łukasz Anforowicz <[hidden email]>
>>>>>> wrote:
>>>>>>>
>>>>>>> On Wed, Oct 26, 2016 at 9:46 AM, Łukasz Anforowicz
>>>>>>> <[hidden email]> wrote:
>>>>>>>>
>>>>>>>> So - is it fair to say that the CL under review can proceed as-is
>>>>>>>> (*) given that:
>>>>>>>>
>>>>>>>> The ordering problem has existed *before* the CL under review
>>>>>>>> (admittedly the CL under review adds 2 more cases to the list of problematic
>>>>>>>> ordering scenarios)
>>>>>>>> Below it seems that we have a promising direction for solving the
>>>>>>>> ordering problem (the solution probably requires a slightly bigger CL - one
>>>>>>>> that also tackles the question of what hasDeclaration should do about
>>>>>>>> AutoType, PointerType, etc.)
>>>>>>>>
>>>>>>>> -Lukasz
>>>>>>>>
>>>>>>>> (*) I know that I need to add documentation changes to mention
>>>>>>>> ElaboratedType
>>>>>>>>
>>>>>>>> On Tue, Oct 25, 2016 at 5:02 PM, Richard Smith
>>>>>>>> <[hidden email]> wrote:
>>>>>>>>>
>>>>>>>>> On 25 October 2016 at 16:47, Łukasz Anforowicz <[hidden email]>
>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>> I think I understand the concern - the concern is that desugaring
>>>>>>>>>> done in HasDeclarationMatcher::matchesSpecialized(const QualType&, ...)
>>>>>>>>>> *latches* onto desugared types in *arbitrary* order.  I agree that this is a
>>>>>>>>>> problem (most obvious for me for the typeAliasDecl and
>>>>>>>>>> classTemplateSpecializationDecl case).  I agree that the CL adds a new case
>>>>>>>>>> to the set of ordered desugaring attempts done by this method (and therefore
>>>>>>>>>> can be seen as making the old problem slightly worse).
>>>>>>>>>>
>>>>>>>>>> I think this problem can be fixed by getting rid of the *order*
>>>>>>>>>> inside HasDeclarationMatcher::matchesSpecialized(const QualType&, ...) -
>>>>>>>>>> instead of:
>>>>>>>>>>
>>>>>>>>>> if (auto *TD = Node->getAsTagDecl())
>>>>>>>>>>   return matchesDecl(TD, Finder, Builder);
>>>>>>>>>> else if (auto *TT = Node->getAs<TypedefType>())
>>>>>>>>>>   return matchesDecl(TT->getDecl(), Finder, Builder);
>>>>>>>>>>
>>>>>>>>>> /* the CL under review would add ElaboratedType and
>>>>>>>>>>    TemplateSpecializationType somewhere here */
>>>>>>>>>>
>>>>>>>>>> ...
>>>>>>>>>> else if (auto *ICNT = Node->getAs<InjectedClassNameType>())
>>>>>>>>>>   return matchesDecl(ICNT->getDecl(), Finder, Builder);
>>>>>>>>>> return false;
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> have something like this:
>>>>>>>>>>
>>>>>>>>>>     if (auto *TD = Node->getAsTagDecl()) {
>>>>>>>>>>       if (matchesDecl(TD, Finder, Builder))
>>>>>>>>>>         return true;
>>>>>>>>>>     }
>>>>>>>>>>
>>>>>>>>>>     if (auto *TT = Node->getAs<TypedefType>()) {
>>>>>>>>>>       if (matchesDecl(TT->getDecl(), Finder, Builder))
>>>>>>>>>>         return true;
>>>>>>>>>>     }
>>>>>>>>>>
>>>>>>>>>>     if (auto *ET = Node->getAs<ElaboratedType>()) {
>>>>>>>>>>       if (matchesSpecialized(ET->getNamedType(), Finder, Builder))
>>>>>>>>>>         return true;
>>>>>>>>>>     }
>>>>>>>>>>
>>>>>>>>>>     if (auto *TST = Node->getAs<TemplateSpecializationType>()) {
>>>>>>>>>>       if (matchesSpecialized(*TST, Finder, Builder))
>>>>>>>>>>         return true;
>>>>>>>>>>     }
>>>>>>>>>> ...
>>>>>>>>>>     if (auto *ICNT = Node->getAs<InjectedClassNameType>()) {
>>>>>>>>>>       if (matchesDecl(ICNT->getDecl(), Finder, Builder))
>>>>>>>>>>         return true;
>>>>>>>>>>     }
>>>>>>>>>>
>>>>>>>>>>     return false;
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I think this is an interesting idea. It would certainly address my
>>>>>>>>> concerns. (You'd actually want something slightly different from the above,
>>>>>>>>> I think: given
>>>>>>>>>
>>>>>>>>>   typedef int A;
>>>>>>>>>   typedef A B;
>>>>>>>>>   B x;
>>>>>>>>>
>>>>>>>>> ...
>>>>>>>>> varDecl(hasType(qualType(hasDeclaration(typedefNameDecl().bind("TD")))))
>>>>>>>>> should match x /twice/: once with TD pointing at A and once with it pointing
>>>>>>>>> at B. So you probably want to repeatedly single-step desugar the type and
>>>>>>>>> try extracting a declaration from each level of type sugar.)
>>>>>>>>
>>>>>>>>
>>>>>>>> Oh, good point - I haven't thought about this scenario.  I think
>>>>>>>> this direction (based on getAsSuger from Type.cpp) look promising (passes a
>>>>>>>> test with 2 chained typedefs):
>>>>>>>>
>>>>>>>> template <typename T, typename DeclMatcherT>
>>>>>>>> class HasDeclarationMatcher : public WrapperMatcherInterface<T> {
>>>>>>>> ...
>>>>>>>>
>>>>>>>>   /// \brief Extracts the TagDecl of a QualType and returns whether
>>>>>>>> the inner
>>>>>>>>   /// matcher matches on it.
>>>>>>>>   bool matchesSpecialized(const QualType &Node, ASTMatchFinder
>>>>>>>> *Finder,
>>>>>>>>                           BoundNodesTreeBuilder *Builder) const {
>>>>>>>>     if (Node.isNull())
>>>>>>>>       return false;
>>>>>>>>
>>>>>>>>     // Do not use getAs<TemplateTypeParmType> instead of the direct
>>>>>>>> dyn_cast.
>>>>>>>>     // Calling getAs will return the canonical type, but that type
>>>>>>>> does not
>>>>>>>>     // store a TemplateTypeParmDecl. We *need* the uncanonical type,
>>>>>>>> if it is
>>>>>>>>     // available, and using dyn_cast ensures that.
>>>>>>>>     if (auto *TTP =
>>>>>>>> dyn_cast<TemplateTypeParmType>(Node.getTypePtr())) {
>>>>>>>>       if (matchesDecl(TTP->getDecl(), Finder, Builder))
>>>>>>>>         return true;
>>>>>>>>     }
>>>>>>>>
>>>>>>>>     const Type* Cur = Node.getTypePtr();
>>>>>>>>     while (Cur) {
>>>>>>>>       switch (Cur->getTypeClass()) {
>>>>>>>> #define ABSTRACT_TYPE(Class, Parent)
>>>>>>>> #define TYPE(Class, Parent) \
>>>>>>>>       case Type::Class: { \
>>>>>>>>         const Class##Type *Ty = cast<Class##Type>(Cur); \
>>>>>>>>         if (matchesSpecialized(*Ty, Finder, Builder)) \
>>>>>>>>           return true; \
>>>>>>>>         if (!Ty->isSugared()) return false; \
>>>>>>>>         Cur = Ty->desugar().getTypePtr(); \
>>>>>>>>         break; \
>>>>>>>>       }
>>>>>>>> #include "clang/AST/TypeNodes.def"
>>>>>>>>       }
>>>>>>>>     }
>>>>>>>>
>>>>>>>>     return false;
>>>>>>>>   }
>>>>>>>>
>>>>>>>>
>>>>>>> BTW: I have to note that the code above is potentially O(N^2) where N
>>>>>>> is the length of the desugaring chain (because each desugaring step can
>>>>>>> potentially call back into the QualType overload).  This is obviously
>>>>>>> suboptimal.  We probably need to discuss this when we review / talk about
>>>>>>> landing something like this.
>>>>>>
>>>>>>
>>>>>> It's basically the same difference between has() and hasDescendant.
>>>>>> I think
>>>>>> a) if we go that route, we should make it explicit by having a matcher
>>>>>> that has it in its name
>>>>>> b) we need to be able to memoize at each step, so this needs to become
>>>>>> part of the visitor interface of the matchers; given the sheer number of
>>>>>> types this seems like it'll get rather complex
>>>>>>
>>>>>> I'll make sure to discuss this with Richard etc al in person next week
>>>>>> when we're at LLVM conf.
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> but it opens a whole bunch of questions - what to do with other
>>>>>>>> types - for example should hasDeclaration "jump over / drill holes in paint
>>>>>>>> cans" for:
>>>>>>>>
>>>>>>>> AutoType, TypeOfExprType, DecltypeType, TypeOfType
>>>>>>>> LValueReferenceType, ValueReferenceType
>>>>>>>> PointerType, ReferenceType
>>>>>>>> And all the other types where I had to add a matchSpecialized
>>>>>>>> overload for so that things compile (some of these obviously don't have a
>>>>>>>> decl [and therefore shouldn't match hasDeclaration(...)] - like BuiltinType,
>>>>>>>> but I am listing all of them for completeness): AtomicType, PipeType,
>>>>>>>> ObjCObjectPointerType, ObjCObjectType, PackExpansionType,
>>>>>>>> DependentTemplateSpecializationType, DependentNameType, AttributedType,
>>>>>>>> UnaryTransformType, SubstTemplateTypeParmPackType,
>>>>>>>> SubstTemplateTypeParmType, AdjustedType, DecayedType, ParenType,
>>>>>>>> FunctionNoProtoType, FunctionProtoType, ExtVectorType, VectorType,
>>>>>>>> DependentSizedExtVectorType, DependentSizedArrayType, VariableArrayType,
>>>>>>>> IncompleteArrayType, ConstantArrayType, MemberPointerType, BlockPointerType,
>>>>>>>> ComplexType, BuiltinType,
>>>>>>>>
>>>>>>>> Also - I am not quite sure if I understand if special-casing of
>>>>>>>> TemplateTypeParmType is needed.  It probably is (because here we use
>>>>>>>> dyn_cast rather than cast).  At any rate drilling into details here can
>>>>>>>> probably wait until a real review in Phabricator...
>>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> The code above passes all tests from the clang-test ninja target +
>>>>>>>>>> results in the following behavior for the 2 cases pointed out in an email:
>>>>>>>>>>
>>>>>>>>>> TEST(HasDeclaration, Case1FromEmail) {
>>>>>>>>>>   std::string input =
>>>>>>>>>>       "template<typename T>\n"
>>>>>>>>>>       "class Template {\n"
>>>>>>>>>>       " public:\n"
>>>>>>>>>>       "  void Method() {}\n"
>>>>>>>>>>       "};\n"
>>>>>>>>>>       "template <typename U>\n"
>>>>>>>>>>       "void Function(Template<U> param) {\n"
>>>>>>>>>>       "  param.Method();\n"
>>>>>>>>>>       "};\n";
>>>>>>>>>>
>>>>>>>>>>   // param type has-declaration matches class template decl?
>>>>>>>>>>   EXPECT_TRUE(matches(input, parmVarDecl(
>>>>>>>>>>       hasName("param"),
>>>>>>>>>>       hasType(qualType(
>>>>>>>>>>
>>>>>>>>>> hasDeclaration(decl(classTemplateDecl(hasName("Template")))))))));
>>>>>>>>>>
>>>>>>>>>>   // param type has-declaration matches class decl?
>>>>>>>>>>   EXPECT_FALSE(matches(input, parmVarDecl(
>>>>>>>>>>       hasName("param"),
>>>>>>>>>>       hasType(qualType(
>>>>>>>>>>
>>>>>>>>>> hasDeclaration(decl(cxxRecordDecl(hasName("Template")))))))));
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> TEST(HasDeclaration, Case2FromEmail) {
>>>>>>>>>>   std::string input =
>>>>>>>>>>       "template<typename T>\n"
>>>>>>>>>>       "class Template {\n"
>>>>>>>>>>       " public:\n"
>>>>>>>>>>       "  void Method() {}\n"
>>>>>>>>>>       "};\n"
>>>>>>>>>>       "void Function(Template<int> param) {\n"
>>>>>>>>>>       "  param.Method();\n"
>>>>>>>>>>       "};\n";
>>>>>>>>>>
>>>>>>>>>>   // param type has-declaration matches class template decl?
>>>>>>>>>>   EXPECT_TRUE(matches(input, parmVarDecl(
>>>>>>>>>>       hasName("param"),
>>>>>>>>>>       hasType(qualType(
>>>>>>>>>>
>>>>>>>>>> hasDeclaration(decl(classTemplateDecl(hasName("Template")))))))));
>>>>>>>>>>
>>>>>>>>>>   // param type has-declaration matches class decl?
>>>>>>>>>>   EXPECT_TRUE(matches(input, parmVarDecl(
>>>>>>>>>>       hasName("param"),
>>>>>>>>>>       hasType(qualType(
>>>>>>>>>>
>>>>>>>>>> hasDeclaration(decl(cxxRecordDecl(hasName("Template")))))))));
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> If the test expectations above look reasonable, then we can just
>>>>>>>>>> discuss how to land the code changes above (e.g. as a single CL or 2 CLs - I
>>>>>>>>>> would prefer 2 separate CLs).  Otherwise, let's talk about the test
>>>>>>>>>> expectations - what should they look like / how should they be different
>>>>>>>>>> from the test snippet above?
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On Tue, Oct 25, 2016 at 1:42 PM, Richard Smith
>>>>>>>>>> <[hidden email]> wrote:
>>>>>>>>>>>
>>>>>>>>>>> On 25 October 2016 at 01:57, Manuel Klimek <[hidden email]>
>>>>>>>>>>> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> On Tue, Oct 25, 2016 at 3:25 AM Richard Smith
>>>>>>>>>>>> <[hidden email]> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 24 October 2016 at 16:41, Łukasz Anforowicz
>>>>>>>>>>>>> <[hidden email]> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I am not sure if we are making progress, but so far, we have
>>>>>>>>>>>>>> seen 1) test cases that are broken and fixed by the CL under review and 2)
>>>>>>>>>>>>>> test cases that are not changed by the CL under review and 3) no test cases
>>>>>>>>>>>>>> that are made worse by the CL under review.  I've tried to gather the
>>>>>>>>>>>>>> scenarios and the associated test cases in
>>>>>>>>>>>>>> https://docs.google.com/document/d/1y1F6uKdulluYBFch680g6bmHe_zDe8mHFYjfXRoakNI/edit.
>>>>>>>>>>>>>> Could you please take a look and shout if I misrepresented anything (i.e.
>>>>>>>>>>>>>> whether we agree that a specific test scenario shows a bug).
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I've also added some comments below / inline.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On Mon, Oct 24, 2016 at 3:54 PM, Richard Smith
>>>>>>>>>>>>>> <[hidden email]> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 24 October 2016 at 15:25, Łukasz Anforowicz
>>>>>>>>>>>>>>> <[hidden email]> wrote:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On Mon, Oct 24, 2016 at 2:21 PM, Richard Smith
>>>>>>>>>>>>>>>> <[hidden email]> wrote:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> On 24 October 2016 at 11:23, Łukasz Anforowicz
>>>>>>>>>>>>>>>>> <[hidden email]> wrote:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> I've run a few more experiments and I see that before any
>>>>>>>>>>>>>>>>>> changes:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> (1) Presence of ElaboratedType means that hasDeclaration
>>>>>>>>>>>>>>>>>> doesn’t match *anything*.  This is clearly wrong.  I still want to fix that.
>>>>>>>>>>>>>>>>>> (2) Both examples from Richard’s email consistently match
>>>>>>>>>>>>>>>>>> the class template decl (and not the class decl).  If Richard’s expectations
>>>>>>>>>>>>>>>>>> are not met *prior* to my CL, then I don’t think these expectations should
>>>>>>>>>>>>>>>>>> be fixed by my CL.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> OTOH, maybe it is worth to open a bug to investigate this
>>>>>>>>>>>>>>>>>> more - personally I would argue that hasDeclaration should match *any* decl
>>>>>>>>>>>>>>>>>> - for example any of type alias / template class decl / class decl.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> (3) If desugaring can result in both
>>>>>>>>>>>>>>>>>> TemplateSpecializationType and TypedefType, then only one is arbitrarily
>>>>>>>>>>>>>>>>>> picked - see test case TypedefTypeAndTemplateSpecializationTypeAreBothHere.
>>>>>>>>>>>>>>>>>> Again - this problem exists *prior* to my CL.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> So - my plan right now is to just simplify/revert my
>>>>>>>>>>>>>>>>>> patches so they only fix (1).  I don't think discussion of (2) should impact
>>>>>>>>>>>>>>>>>> the CL, because the behavior that disagrees with Richard's expectations is
>>>>>>>>>>>>>>>>>> happening *before* my changes.  I will also not attempt to fix (3), because
>>>>>>>>>>>>>>>>>> this starts to interfere with (2) - sorry :-/.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> QUESTION: Does the plan from the previous paragraph sound
>>>>>>>>>>>>>>>>>> good to you?
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Yes; I agree that failing to look through ElaboratedType
>>>>>>>>>>>>>>>>> here is just a bug.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Ack.  I've marked "elaborated type" section in the doc as
>>>>>>>>>>>>>> something that is improved by the CL under review.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Experiments before my changes were done with the following
>>>>>>>>>>>>>>>>>> unit tests:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> // This testcase shows that Richard’s expectations are not
>>>>>>>>>>>>>>>>>> met *prior* to my CL.
>>>>>>>>>>>>>>>>>> // (i.e. the testcase below passes before my CL;  the
>>>>>>>>>>>>>>>>>> expectations in the testcase
>>>>>>>>>>>>>>>>>> // are the opposite of Richard’s expectations).
>>>>>>>>>>>>>>>>>> TEST(HasDeclaration, Case1FromEmail) {
>>>>>>>>>>>>>>>>>>   std::string input =
>>>>>>>>>>>>>>>>>>       "template<typename T>\n"
>>>>>>>>>>>>>>>>>>       "class Template {\n"
>>>>>>>>>>>>>>>>>>       " public:\n"
>>>>>>>>>>>>>>>>>>       "  void Method() {}\n"
>>>>>>>>>>>>>>>>>>       "};\n"
>>>>>>>>>>>>>>>>>>       "template <typename U>\n"
>>>>>>>>>>>>>>>>>>       "void Function(Template<U> param) {\n"
>>>>>>>>>>>>>>>>>>       "  param.Method();\n"



--
Thanks,

Lukasz

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Loading...