[cfe-dev] Improving retention of type sugar

[Sam McCall via cfe-dev]

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/20211118/e69fc2f1/attachment-0001.html>