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

Now with the right 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

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

> 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

>
>>
>> Now with the right cfe-dev
>>
>>
>>>
>>> Somehow we lost cfe-dev on this mail thread :frowning: :frowning:
>>>
>>> adding Aaron, who has also contributed to that part of the code
>>>
>>>>
>>>>>
>>>>> 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
>>>>> tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp - Issue 2256913002: Handling of DependentScopeDeclRefExpr and CXXDependentScopeMemberExpr nodes. - Code Review 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
>>>> :frowning:
>>>> 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 :slight_smile: 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 ?)

I’m still working on a patch. Sorry for the delay, I’m currently traveling…

I 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…

+Sam who has also run into this.