[cfe-dev] Improving retention of type sugar

[Sam McCall via cfe-dev]

I have a first cut of a UsingType at https://reviews.llvm.org/D114251
(It passes tests, but I need to write some new ones - I'm not confident in
the coverage here.)
I have a couple of questions open there about unifying with other node
types & handling of templates, any thoughts appreciated (probably on the
patch).

On Thu, Nov 18, 2021 at 5:09 PM Sam McCall <sammccall at google.com> wrote:

> Thanks a lot for the pointers! I'm hacking this up now, this will save
> some time.
>
> (Seems like the scope might be slightly wider than UsingShadowDecl - based
> on the precedent in DeclRefExpr::getFoundDecl() maybe we should accommodate
> ObjCCompatibleAliasDecl too, and so expose the founddecl as NamedDecl* too)
>
> On Thu, Nov 18, 2021 at 3:40 PM David Rector <davrecthreads at gmail.com>
> wrote:
>
>>
>>
>> On Nov 18, 2021, at 8:45 AM, Sam McCall via cfe-dev <
>> cfe-dev at lists.llvm.org> wrote:
>>
>> Hi Matheus,
>>
>> Thanks for working on improving sugar! In clangd, this seems to be mostly
>> a spray of minor improvements where the behavior wasn't great but fixing it
>> wasn't a huge priority.
>> I appreciate it's a lot of work to do something about all the test cases
>> in various projects, feel free to ping me on reviews to look at test
>> failures for clangd if you prefer.
>> I don't have a lot of feedback on the specific changes beyond what I
>> already raised on the patch.
>>
>> I do have another type-sugar problem/proposal you might have an opinion
>> on: a sugar type to indicate when a type was resolved via a UsingShadowDecl.
>>
>> ```
>> namespace a { enum X{}; }
>> namespace b { using a::X; /*UsingDecl + UsingShadowDecl */ }
>> b::X m;
>> ```
>> Today I believe there's no way in the AST to get from the VarDecl to find
>> either the UsingDecl or UsingShadowDecl that `b::X` looked up.
>> The type is an ElaboratedType{NNS=b::, Underlying=EnumType{decl=a::X}}
>> While the ElaboratedType.NNS seems like it provides that information, the
>> ElaboratedType refers instead to syntax: even with D112374 we get no NNS if
>> X is written unqualified within namespace b. Moreover we can't actually
>> find the UsingDecl this way, which is critical to our use case.
>> By comparison, for values rather than types we have
>> DeclRefExpr.getFoundDecl().
>>
>> The most obvious solution seems to be a sugar type, e.g. the example
>> would become
>> ElaboratedType{NNS=b::, Underlying=UsingDeclType{Decl=b::X,
>> Underlying=EnumType{decl=a::X}}}
>> Does this make sense?
>>
>> I don't think it would add disproportionate bulk to the AST: I think
>> finding typedefs/aliases are more common and already have TypedefType
>> nodes, and we wouldn't introduce any nodes in case of finding names via
>> UsingDirectiveDecl (just like DeclRefExpr doesn't).
>> And I suppose it makes sense to unify this thing with UnresolvedUsingType
>> (so like TemplateTypeParmType, it could be either canonical or sugar).
>>
>> Our use case for this is diagnosing unused headers, where not recognizing
>> uses of such UsingDecls leads to headers like <cstddef> being falsely
>> marked as unused. This is critical enough that we're happy to work on
>> changing the AST if it seems appropriate.
>>
>> Any thoughts/tips appreciated.
>>
>>
>> Great point - indeed I think a UsingDeclType is the only type sugar
>> "missing" from the AST right now, if we accept that the AST should allow a
>> user to navigate from any node to any of its dependencies.
>>
>> Some time ago I was working on an extension of RecursiveASTVisitor that
>> could also visit dependencies, rather than just the children of a node.
>>
>> The motivating test was, you should be able to select some subset of AST
>> nodes, add in all their declaration dependencies via recursive visitation,
>> print this subset of declarations to a new file, and that new file should
>> compile without any #includes.
>>
>> The only AST change needed to make this work, i.e. to get the file to
>> compile, at least for standard library dependencies (e.g. printing a
>> variable whose type was a std::vector) was to add a UsingDeclType to the
>> sugar.
>>
>> Doing so was straightforward — all the required information is available
>> in the LookupResult at lookup.  It’s been awhile and I was only hacking at
>> this but I think these are the specific places I believe the sugar needs to
>> be introduced.
>> Assuming you have a set up a Context.getUsingDeclType method which
>> creates a UsingDeclType sugar node:
>>
>> 1. Change
>> https://github.com/llvm-mirror/clang/blob/master/lib/Sema/SemaDecl.cpp#L482
>>  to
>> ```
>> T = Context.getTypeDeclType(TD);
>> // If we used a UsingShadowDecls to look up this result,
>> // wrap the result in a UsingDeclType:
>> if (*Result.begin() != IIDecl) {
>>   assert(isa<UsingShadowDecl>(*Result.begin()) &&
>>          "Expected that, for a TypeDecl, this "
>>          "would always be a UsingShadowDecl");
>>   assert(IIDecl == IIDecl->getUnderlyingDecl());
>>   T = Context.getUsingDeclType(
>>       cast<UsingShadowDecl>(*Result.begin()), T.getCanonicalType());
>> }
>> ```
>>
>> 2. Change
>> https://github.com/llvm-mirror/clang/blob/master/lib/Sema/SemaDecl.cpp#L1125
>>  to
>> ```
>> QualType T = Context.getTypeDeclType(Type);
>> // If we used a UsingShadowDecls to look up this result,
>> // wrap the result in a UsingDeclType:
>> if (*Result.begin() != FirstDecl) {
>>   assert(isa<UsingShadowDecl>(*Result.begin()) &&
>>          "Expected that, for a TypeDecl, this "
>>          "would always be a UsingShadowDecl");
>>   T = Context.getUsingDeclType(
>>       cast<UsingShadowDecl>(*Result.begin()), T.getCanonicalType());
>> }
>> ```
>>
>> 3. Change
>> https://github.com/llvm-mirror/clang/blob/master/lib/Sema/TreeTransform.h#L13081
>>  to
>> ```
>> return SemaRef.Context.getUsingType(*Using->shadow_begin());
>> ```
>>
>>
>> On Mon, Nov 8, 2021 at 11:59 PM Matheus Izvekov via cfe-dev <
>> cfe-dev at lists.llvm.org> wrote:
>>
>>> Hi all,
>>>
>>> We have been working on fixes for some areas in clang where type sugar
>>> was not being retained.
>>>
>>> The first part of that work, on D110216, is about expanding
>>> DeducedType so it's able to hold a non-canonical deduced-as type, and
>>> reworking the template argument deduction machinery so it can deliver
>>> that sugar to AutoType deduction.
>>>
>>> Besides the diagnostics improvements which can be seen on the DR, this
>>> is something that I believe will enable further improvements to other
>>> analysis, especially for clang-tidy checkers. And this brings me to
>>> one of the reasons for writing this email, I want to let folks working
>>> on those areas know so we can take advantage of this when it's merged.
>>>
>>> One example of such analysis which is improved in that patch is
>>> "cppcoreguidelines-owning-memory", where now it's possible to deduce
>>> the `gsl::owner` wrapper. So you can use `auto` here:
>>> ```
>>> gsl::owner<int*> Owner1 = function_that_returns_owner(); // Good.
>>> auto Owner2 = function_that_returns_owner(); // This was Bad, but is now
>>> Good.
>>> ````
>>> The improvement above is hitching a ride on D110216, though it hasn't
>>> been expanded to take advantage of return type deduction yet.
>>>
>>> With just D110216, we consider only the type sugar of the first
>>> argument when a type is deduced from multiple arguments (or return
>>> statements in the case of return type deduction).
>>>
>>> With the next patch in the stack, D111283, we implement a way of
>>> folding the type sugar from multiple arguments, into a sort of 'lowest
>>> common denominator'. Example:
>>> ```
>>> using Man = int;
>>> using Socrates = Man;
>>> using Archimedes = Man;
>>> auto f() {
>>>   if (0)
>>>     return Socrates();
>>>   return Archimedes();
>>> }
>>>
>>> auto X = f(); // X's type will be deduced as `Man` (with AutoType sugar
>>> on top).
>>> ```
>>> And this maps nicely to cppcoreguidelines-owning-memory's
>>> sugar-as-privilege model. If one of the return statements does not
>>> have the owner wrapper, then the return type will never be deduced as
>>> an owner as well.
>>>
>>> There are other things of more consequence which will be retained:
>>> AttributedType nodes, which are always sugar. So for example returning
>>> a pointer with extra alignment in a function with deduced return type
>>> will actually work as would be expected, the return type will not miss
>>> that alignment. This should fix PR51282, as was pointed out in the
>>> review of D110216.
>>>
>>> We also might have to update `readability-qualified-auto` to take this
>>> into account.
>>> Example:
>>> ```
>>> using Man = int;
>>> using ManPtr = Man*;
>>>
>>> auto X = ManPtr(); // X deduced as `ManPtr`.
>>> auto *Y = ManPtr(); // Y deduced as `Man*`, the top level typedef is
>>> stripped off in order to get to the pointer.
>>> ```
>>> So it will not be a good idea anymore to recommend the `auto` >
>>> `auto*` transformation in case the argument type is sugar to pointer.
>>> This would be especially bad if there was some important attribute
>>> attached to ManPtr.
>>>
>>> It would be interesting to know what other checkers could be improved
>>> here, and other ideas for improvements folks have. Also please take a
>>> look at the other patches in the same stack for more improvements in
>>> the type sugar department. These patches end up causing a lot of test
>>> churn, some far from the things I am familiar with. I have given my
>>> best effort to deal with those, but more input is always welcome.
>>>
>>> Thanks!
>>> _______________________________________________
>>> cfe-dev mailing list
>>> cfe-dev at lists.llvm.org
>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>>
>> _______________________________________________
>> cfe-dev mailing list
>> cfe-dev at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20211119/bb5ea1a5/attachment-0001.html>