[cfe-dev] Improving retention of type sugar
David Rector via cfe-dev
cfe-dev at lists.llvm.org
Thu Nov 18 06:40:08 PST 2021
> 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 <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 <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 <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 <mailto: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 <mailto:cfe-dev at lists.llvm.org>
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev <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/cc14dcba/attachment.html>
More information about the cfe-dev
mailing list