[cfe-dev] Improving retention of type sugar

[Sam McCall via cfe-dev]

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.

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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20211118/d44d66b7/attachment-0001.html>