[cfe-dev] Next steps for D24361 (patch to fix hasDeclaration wrt ElaboratedType)
Manuel Klimek via cfe-dev
cfe-dev at lists.llvm.org
Fri Oct 28 07:27:56 PDT 2016
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"
> "};\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...
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20161028/37a18127/attachment.html>
More information about the cfe-dev
mailing list