[cfe-dev] Next steps for D24361 (patch to fix hasDeclaration wrt ElaboratedType)

[Manuel Klimek 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 <klimek at google.com> wrote:

> Now with the right cfe-dev
>
>
> On Fri, Oct 28, 2016 at 4:14 PM Manuel Klimek <klimek at google.com> 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 <klimek at google.com> wrote:
>
> On Fri, Oct 28, 2016 at 3:54 PM Łukasz Anforowicz <lukasza at google.com>
> 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
>    <https://docs.google.com/document/d/1y1F6uKdulluYBFch680g6bmHe_zDe8mHFYjfXRoakNI/edit#heading=h.teax79qcqv70>
>    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 <klimek at google.com> wrote:
>
> +ben & sam
>
> On Wed, Oct 26, 2016 at 6:50 PM Łukasz Anforowicz <lukasza at google.com>
> wrote:
>
> On Wed, Oct 26, 2016 at 9:46 AM, Łukasz Anforowicz <lukasza at google.com>
> 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 <richardsmith at google.com>
> wrote:
>
> On 25 October 2016 at 16:47, Łukasz Anforowicz <lukasza at google.com> 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
> <https://docs.google.com/document/d/1y1F6uKdulluYBFch680g6bmHe_zDe8mHFYjfXRoakNI/edit#heading=h.teax79qcqv70>).
> 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 <richardsmith at google.com>
> wrote:
>
> On 25 October 2016 at 01:57, Manuel Klimek <klimek at google.com> wrote:
>
> On Tue, Oct 25, 2016 at 3:25 AM Richard Smith <richardsmith at google.com>
> wrote:
>
> On 24 October 2016 at 16:41, Łukasz Anforowicz <lukasza at google.com> 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 <richardsmith at google.com>
> wrote:
>
> On 24 October 2016 at 15:25, Łukasz Anforowicz <lukasza at google.com> wrote:
>
> On Mon, Oct 24, 2016 at 2:21 PM, Richard Smith <richardsmith at google.com>
> wrote:
>
> On 24 October 2016 at 11:23, Łukasz Anforowicz <lukasza at google.com> 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
> <https://docs.google.com/document/d/1y1F6uKdulluYBFch680g6bmHe_zDe8mHFYjfXRoakNI/edit#heading=h.d5khzityt7x>"
> 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"
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20161028/970082f6/attachment.html>