Page MenuHomePhabricator

[clang] retain type sugar in auto / template argument deduction
Needs ReviewPublic

Authored by mizvekov on Sep 21 2021, 7:31 PM.

Details

Summary

This implements the following changes:

  • AutoType retains sugared deduced-as-type.
  • Template argument deduction machinery analyses the sugared type all the way

down. It would previously lose the sugar on first recursion.

  • Undeduced AutoType will be properly canonicalized, including the constraint

template arguments.

  • Remove the decltype node created from the decltype(auto) deduction.

As a result, we start seeing sugared types in a lot more test cases,
including some which showed very unfriendly type-parameter-*-* types.

Depends on D110210

Signed-off-by: Matheus Izvekov <mizvekov@gmail.com>

Diff Detail

Unit TestsFailed

TimeTest
2,560 msx64 debian > libFuzzer.libFuzzer::fuzzer-finalstats.test
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -O2 -gline-tables-only -fsanitize=address,fuzzer -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/lib/fuzzer -m64 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/fuzzer/SimpleTest.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/fuzzer-finalstats.test.tmp-SimpleTest

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
nridge added inline comments.Sun, Sep 26, 11:55 PM
clang-tools-extra/clangd/unittests/InlayHintTests.cpp
470

This test expectation change is suspicious. The type is being printed with PrintingPolicy::SuppressScope=true, shouldn't that still be respected?

mizvekov marked an inline comment as done.Mon, Sep 27, 3:26 AM
mizvekov added inline comments.
clang-tools-extra/clangd/unittests/InlayHintTests.cpp
470

Thanks for pointing that out, I will take a look, but I suspect this to be some TypePrinter issue.

mizvekov updated this revision to Diff 375270.EditedMon, Sep 27, 8:14 AM
mizvekov edited the summary of this revision. (Show Details)

Fix ElaboratedType printer not respecting SuppressScope policy.

mizvekov marked 4 inline comments as done.Mon, Sep 27, 8:16 AM

FYI I just realized some other tests I never run locally relied on the former behavior of the ElaboratedType printer.

I will see what looks more reasonable, but I think the defaults are tuned so the type printer will output code that looks close to what it was as written,
and this InlayHints was relying on behavior that was not actually supported there. We might have to split this off into a separate policy flag or something like that.

mizvekov updated this revision to Diff 375314.Mon, Sep 27, 9:51 AM
mizvekov edited the summary of this revision. (Show Details)
  • Revert TypePrinter change.
  • Make clang-tidy happy.
mizvekov marked an inline comment as not done.Mon, Sep 27, 10:11 AM
mizvekov added inline comments.
clang-tools-extra/clangd/unittests/InlayHintTests.cpp
470

Could you explain to me why the former behavior is more desirable here?

I initially understood that this hint should provide the type written in a form that would actually work if it replaced the 'auto'.
It is strange, but if it is just meant as a hint, it's still fine I guess.

Or maybe this was broken in the first place and just was just missing a FIXME?

nridge added inline comments.Mon, Sep 27, 5:09 PM
clang-tools-extra/clangd/unittests/InlayHintTests.cpp
470

The type is being printed with a PrintingPolicy which has the non-default SuppressScope flag set.

The documentation of this flag says "Suppresses printing of scope specifiers", which I understand to mean both namespace and class scope specifiers.

If you're asking why we are using this flag for inlay hints, it's to keep the hints short so they don't take up too much horizontal space. Since the hints are displayed inline, hints that are too long can result in the line of code visually extending past the width of the editor window, and we try to minimize the impact of this.

mizvekov added inline comments.Mon, Sep 27, 5:45 PM
clang-tools-extra/clangd/unittests/InlayHintTests.cpp
470

I see.

The problem is that the ElaboratedType sugar does not respect this when printing the nested name specifier it contains.
The quick fix of trying to make it respect it shows that there are a bunch of tests that expect it not to.
It seems that specific policy is used in places that need it to recover the type as written.

So if we want this behavior, we probably need to extend the printing policy with a new flag here.

nridge added inline comments.Mon, Sep 27, 5:49 PM
clang-tools-extra/clangd/unittests/InlayHintTests.cpp
470

Thanks for the explanation!

I think this test expectation change is fine then, and would just suggest adding a "// FIXME: SuppressScope is not respected in this case" comment perhaps.

Thanks, this is very exciting, and the diagnostic changes in the test suite look great!

clang-tools-extra/clangd/unittests/ASTTests.cpp
180

Very true. But probably not worth keeping the comment. :)

clang-tools-extra/test/clang-tidy/checkers/cert-static-object-exception.cpp
276 ↗(On Diff #375314)

What happened here? It looks like nothing this expression does can actually throw. (This lambda's operator() isn't noexcept, but the functions it calls all are.) Was this only avoiding a false positive by accident before?

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-owning-memory.cpp
94–96

@JonasToth What was the intent here -- is there a specification for how gsl::owner is supposed to interact with auto deduction? That is, is this a bug fix or a regression?

clang/lib/Sema/SemaTemplateDeduction.cpp
242

I wonder if we can come up with a good heuristic for determining which of X and Y we want to return here. One thing that immediately springs to mind is: if either type is canonical, we should return the other one.

1037–1038

Hm, do we need the getUnqualifiedType() calls here? I'd expect that we'd have the TDF_IgnoreQualifiers flag set in this case, so we should be ignoring qualifiers anyway. Perhaps DeduceTemplateArgumentsByTypeMatch should be removing qualifiers itself if that flag is set?

1334

Par is an unusual name for a parameter type; Parm or Param would be more common and I'd find either of those to be clearer. (Ideally use the same spelling for Param and ParDesug.) Given that the description in the standard uses P and A as the names of these types, those names would be fine here too if you want something short.

1334–1346

Instead of tracking two types here, I think it'd be clearer to follow the more usual pattern in Clang: track only Param and Arg, use Arg->getAs<T>() instead of dyn_cast<T>(ArgDesug) to identify if Arg is sugar for a T, and use Arg->getAs<T>() instead of cast<T>(ArgDesug) to get a minimally-desugared T* from Arg.

The only place where I think we'd want something different from that is in the switch (Param->getTypeClass()) below, where I think we should switch on the type class of the canonical type (or equivalently on the type class of the desugared type, but it's cheaper and more obviously correct to ask for the type class of the canonical type).

1523–1524

Can we remove these? It looks like we're almost not using them at all, and that we shouldn't need to use them: any type matching should work equally well with the desugared type, and we never want to include these in our deduced results or our diagnostics.

1594–1599

This looks wrong to me: we should be comparing the types, not how they're written. Context.hasSameType(Param, Arg) (or Context.hasSameUnqualifiedType(Param, Arg) in the TDF_IgnoreQualifiers case) would be appropriate here.

1742–1743

Ignoring qualifiers here doesn't seem right to me. For example:

template<typename T> void f(T(T));
const int g(int);
void h() { f(g); }

... should result in a deduction failure, because we should get T = const int from the return type but T = int from the parameter type.

2183

Presumably this indentation change is unintended?

2250–2261

This suggests that we're passing in unresolved template arguments to this function. That seems problematic to me: those unresolved template arguments (as-written) will not be properly converted to the template's parameter type, will miss default arguments, and might not have had arguments properly combined into packs.

For *type* template arguments, I think it's fine to pass the argument as written if we're sure we know what it is, because basically no conversions apply for type template parameters and there may be meaningful sugar we want to preserve, but otherwise I think we should be passing in the resolved template argument.

4686

Hm, I see, adding the type sugar here presumably means that we get better diagnostic quality: we can complain that we can't deduce (eg) auto* against int instead of complaining that we can't deduce type-parameter-0-0* against int, right?

Can we remove the UseTypeSugar flag entirely now, or is it used for something else too?

clang/test/SemaCXX/cxx1z-class-template-argument-deduction.cpp
565

It'd be nice to check that this deduces the right type for T. I think this case would also be useful to test:

template<class T> void foo0(fptr1<T>) {
   using ExpectedT = const char*; 
   using ExpectedT = T;
}
void bar0(const char *const volatile __restrict);
void t0() { foo0(&bar0); }
569

Interesting, GCC rejects this due to the restrict qualifier mismatch. But I think accepting is correct.

clang/test/SemaTemplate/attributes.cpp
127–129

Looks like we'll need to use a different mechanism to remove the type sugar in this test. (Maybe one day we'll run out of ways to remove type sugar and we won't need this test any more!)

Thanks for working on this! How hard would it be to support:

using size_t = __SIZE_TYPE__;
template<typename T> struct Id { typedef T type; };
int main() {
  struct S {} s;
  Id<size_t>::type f = s; // just 'unsigned long', 'size_t' sugar has been lost
}

Thanks for working on this!How hard would it be to support:

using size_t = __SIZE_TYPE__;
template<typename T> struct Id { typedef T type; };
int main() {
  struct S {} s;
  Id<size_t>::type f = s; // just 'unsigned long', 'size_t' sugar has been lost
}

Actually supporting that is in my radar :)

Getting the basic idea up is not too complicated.
We need to support 'alias' template specializations which can carry non-canonical arguments,
and they would just point to the canonical template specializations.
We would then have to implement our desugaring of 'SubstTemplateTypeParmType' so it's able to access some context where it can get the correctly sugared ReplacementType for the given replaced 'TemplateTypeParmType'.

But there are some little details that need to be implemented so that it is reasonably usable.
For example, in the general case we cannot currently, for any given declaration, figure out the template arguments used to instantiate it. We can do it for class and function templates, but not for alias and variable templates.
We need to make anything that carries template arguments also be a declaration context, so we can walk up from any declaration and see those.

There is also the trouble that some elements are not properly parented. For example in a requires clause:
template <class T> requires WHATEVER struct X {}, WHATEVER is not currently parented to the class template, so we would not be able to recover the instantiation arguments there unless we fixed it.

And who knows what other non-obvious problems might be lurking :)

A lot of what I said also overlaps with what we need to properly support lazy substitution, such as required by C++20 concepts, which is also something I am working on.

Getting the 'template argument deduction' machinery to preserve the sugar, which we do in this patch, certainly benefits when we get there.

Thanks, this is very exciting, and the diagnostic changes in the test suite look great!

Thank you, much appreciated, and also thanks for the review!

clang-tools-extra/test/clang-tidy/checkers/cert-static-object-exception.cpp
276 ↗(On Diff #375314)

I haven't debugged it, I was hoping @aaron.ballman would know something about it and it would save me the trouble :)

clang/lib/Sema/SemaTemplateDeduction.cpp
242

I was thinking about the opposite actually, something along the lines of returning the common sugar between them. Starting from the cannonical type and going 'backwards' (adding back the sugar), we return the type just before adding sugar back would make them different.

1037–1038

The case I am worried about is checking the parameters in the function prototype case, and TDF_IgnoreQualifiers is neither set there, nor it would help if it was set anyway.
I will take a look again, but this was the simplest solution I could come up at the time.

1742–1743

Right, I must have simply assumed we had the same situation here with regards to function prototype parameters.

This feels like a standard defect to me, we should not be looking at top level qualifiers on return types :)

2183

arc lint decided to add it automatically, and I decided not to fight it :-)

2250–2261

Sure, I will revert that, presumably because this would be a separate can of worms we should not deal in this patch, but ideally we would get back to this so we can actually see some sugar here?

4686

The primary reason for it was moving in the direction of removing this flag.
We don't need it anymore in this case, although there are other users of it that need to be cleaned up. This one was just flipping the flag here, so I said why the heck not.
But other users are a bit more complicated, and I was afraid the patch was getting too big.

This could help improving the diagnostics in the future I suppose, but right now we are not using the result of the template argument deduction here very much, just for inconsistent deduction from initializer lists, the rest we defer to a generic variable 'foo' with type 'bar' has incompatible initializer of type 'baz'.

mizvekov added inline comments.Thu, Sep 30, 3:59 PM
clang/lib/Sema/SemaTemplateDeduction.cpp
1594–1599

You are right, but the reason we don't get into any troubles here is because this is dead code anyway, the non-dependent case will always be handled above :)

Although perhaps, I wonder if we should dig down into non-dependent types anyway, in case the types are too complex and it's not immediately obvious what does not match, we could perhaps improve the diagnostic?

I will experiment a little bit with this idea.

mizvekov added inline comments.Thu, Sep 30, 5:01 PM
clang/lib/Sema/SemaTemplateDeduction.cpp
1594–1599

Here are the results of this experiment:

error: 'note' diagnostics expected but not seen:
  File SemaCXX\cxx1z-noexcept-function-type.cpp Line 21: could not match 'void (I) noexcept(false)' (aka 'void (int) noexcept(false)') against 'void (int) noexcept'
error: 'note' diagnostics seen but not expected:
  File SemaCXX\cxx1z-noexcept-function-type.cpp Line 21: candidate template ignored: failed template argument deduction

error: 'note' diagnostics expected but not seen:
  File SemaCXX\deduced-return-type-cxx14.cpp Line 146: candidate template ignored: could not match 'auto ()' against 'int ()'
  File SemaCXX\deduced-return-type-cxx14.cpp Line 161: candidate template ignored: could not match 'auto ()' against 'void ()'
error: 'note' diagnostics seen but not expected:
  File SemaCXX\deduced-return-type-cxx14.cpp Line 146: candidate template ignored: could not match 'auto' against 'int'
  File SemaCXX\deduced-return-type-cxx14.cpp Line 161: candidate template ignored: could not match 'auto' against 'void'

error: 'note' diagnostics expected but not seen:
  File SemaCXX\pass-object-size.cpp Line 62 (directive at SemaCXX\pass-object-size.cpp:69): candidate address cannot be taken because parameter 1 has pass_object_size attribute
  File SemaCXX\pass-object-size.cpp Line 56 (directive at SemaCXX\pass-object-size.cpp:74): candidate address cannot be taken because parameter 1 has pass_object_size attribute
  File SemaCXX\pass-object-size.cpp Line 62 (directive at SemaCXX\pass-object-size.cpp:75): candidate address cannot be taken because parameter 1 has pass_object_size attribute
error: 'note' diagnostics seen but not expected:
  File SemaCXX\pass-object-size.cpp Line 56: candidate template ignored: failed template argument deduction
  File SemaCXX\pass-object-size.cpp Line 62: candidate template ignored: failed template argument deduction
  File SemaCXX\pass-object-size.cpp Line 62: candidate template ignored: failed template argument deduction

error: 'note' diagnostics expected but not seen:
  File SemaTemplate\deduction.cpp Line 316: deduced non-type template argument does not have the same type as the corresponding template parameter ('std::nullptr_t' vs 'int *')
  File SemaTemplate\deduction.cpp Line 323: values of conflicting types
error: 'note' diagnostics seen but not expected:
  File SemaTemplate\deduction.cpp Line 275: candidate template ignored: could not match 'const int' against 'int'
  File SemaTemplate\deduction.cpp Line 316: candidate template ignored: could not match 'int *' against 'std::nullptr_t'
  File SemaTemplate\deduction.cpp Line 323: candidate template ignored: could not match 'int *' against 'std::nullptr_t'

error: 'note' diagnostics expected but not seen:
  File SemaTemplate\explicit-instantiation.cpp Line 64: candidate template ignored: could not match 'void ()' against 'void (float *)'
  File SemaTemplate\explicit-instantiation.cpp Line 70: candidate template ignored: could not match 'void (int *)' against 'void (float *)'
error: 'note' diagnostics seen but not expected:
  File SemaTemplate\explicit-instantiation.cpp Line 70: candidate template ignored: could not match 'int' against 'float'
  File SemaTemplate\explicit-instantiation.cpp Line 64: candidate template ignored: failed template argument deduction

It's interesting to note that it reveals several cases we give too generic 'failed template argument deduction' errors, like the different noexcept specifications, function prototypes with different amount of parameters, and the 'pass_object_size attribute' case.

mizvekov updated this revision to Diff 376657.Fri, Oct 1, 5:41 PM
  • Address most comments / requests.

Still missing:

  • Maybe do something about ASTImporter regression.
  • Debug change in behavior in cert-static-object-exception.cpp.
mizvekov marked 12 inline comments as done.Fri, Oct 1, 5:54 PM
mizvekov added inline comments.
clang-tools-extra/clangd/unittests/ASTTests.cpp
180

Such a party pooper =)

clang/lib/Sema/SemaTemplateDeduction.cpp
1334–1346

There was one small catch in your suggestion:
Using getAs on TemplateSpecializationType is a bit tricky because that type can be sugar as well, so you might end up with a type alias instead of the underlying thing you wanted. I think that was the only tricky type though.
And it was productive that I ran into this problem, because I ended up discovering some cases where we were losing some sugar there, and there was a tiny diagnostic improvement in one of the test cases as a result.

mizvekov marked 2 inline comments as done.Sat, Oct 2, 1:19 PM
mizvekov updated this revision to Diff 376724.Sat, Oct 2, 4:03 PM
  • Fix issue with ASTMatcher only looking through one layer of DeducedType. This fix makes cert-static-object-exception.cpp not affected by this patch anymore.
mizvekov updated this revision to Diff 376725.Sat, Oct 2, 4:51 PM
  • Fix ASTImporter regression.

Thanks for working on this!How hard would it be to support:

using size_t = __SIZE_TYPE__;
template<typename T> struct Id { typedef T type; };
int main() {
  struct S {} s;
  Id<size_t>::type f = s; // just 'unsigned long', 'size_t' sugar has been lost
}

Actually supporting that is in my radar :)

Over the years we had some interest from people but never actually got implemented. Here were some ideas @rsmith and I discussed over the years. If that is helpful, let me know if I should dig a bit more into private email exchange.

Over the years we had some interest from people but never actually got implemented. Here were some ideas @rsmith and I discussed over the years. If that is helpful, let me know if I should dig a bit more into private email exchange.

Sure, that is helpful :)

There is other lower hanging fruit where we are losing sugar, and it would be a shame if we implemented this but then did not get much benefit from it because the sugar never got into the template argument in the first place.

One such example is that we do not mark as 'elaborate' types which are written bare, without any scope specifiers.

So for example in a case like this:

namespace foo {
  struct Foo {};
  Foo x = 0;
};

We would still diagnose that assignment with the type of the variable printed as 'foo::Foo' instead of just 'Foo', as it was written, because the parser will have produced a type that is not wrapped in an ElaboratedType (or perhaps some other cheaper mechanism).

Over the years we had some interest from people but never actually got implemented. Here were some ideas @rsmith and I discussed over the years. If that is helpful, let me know if I should dig a bit more into private email exchange.

Sure, that is helpful :)

There is other lower hanging fruit where we are losing sugar, and it would be a shame if we implemented this but then did not get much benefit from it because the sugar never got into the template argument in the first place.

One such example is that we do not mark as 'elaborate' types which are written bare, without any scope specifiers.

So for example in a case like this:

namespace foo {
  struct Foo {};
  Foo x = 0;
};

We would still diagnose that assignment with the type of the variable printed as 'foo::Foo' instead of just 'Foo', as it was written, because the parser will have produced a type that is not wrapped in an ElaboratedType (or perhaps some other cheaper mechanism).

Handling that case is nice. I am more interested in retaining the sugar in template instantiations as it is essential for an optimization in our I/O system which uses clang as a library. If implement the instantiation diagnostic we can get rid of several hacky patches: https://github.com/vgvassilev/clang/commit/d87e2bbc8a266e295ee5a2065f1e587b325d4284 https://github.com/vgvassilev/clang/commit/fcc492fcab14e8b8dc156688dda6f237a04563a7 and https://github.com/vgvassilev/clang/commit/fe17b953325530267643f3391bfd59ac1519ef39

clang/lib/Sema/SemaTemplateDeduction.cpp
1414–1416
1778
mizvekov updated this revision to Diff 376855.Mon, Oct 4, 4:01 AM
  • Address v.g.vassilev's suggestions.
mizvekov marked 2 inline comments as done.Mon, Oct 4, 4:05 AM
craig.topper added inline comments.
clang/include/clang/ASTMatchers/ASTMatchersInternal.h
1032–1033

gcc 8.4.1 doesn't like T being the same name as a template parameter for this class.

Looks like this fixes PR51282.

mizvekov marked an inline comment as done.EditedMon, Oct 4, 12:21 PM

Looks like this fixes PR51282.

Okay, interesting. This could be just papering over the bug.
I will take a careful look at it later.

clang/include/clang/ASTMatchers/ASTMatchersInternal.h
1032–1033

Oh wow, didn't even realize, thanks!!

mizvekov updated this revision to Diff 377006.Mon, Oct 4, 12:40 PM
mizvekov marked an inline comment as done.
  • Rename variable so it doesn't shadow template parameter.

Looks like this fixes PR51282.

I guess it does fix it, but the underlying implementation of alignment is very fragile. The alignment is stored in AttributedType nodes, which are always sugar, so they are not considered part of the type in a structural sense...

Looks like this fixes PR51282.

I guess it does fix it, but the underlying implementation of alignment is very fragile. The alignment is stored in AttributedType nodes, which are always sugar, so they are not considered part of the type in a structural sense...

Unless I'm missing something, alignment on aliases is itself fragile. Let's say you have some template, then the instantiation with the alias is of course the same as the instantiation with the canonical type, so if that template has a member or local variable of that type parameter that won't get the alignment.

Unless I'm missing something, alignment on aliases is itself fragile. Let's say you have some template, then the instantiation with the alias is of course the same as the instantiation with the canonical type, so if that template has a member or local variable of that type parameter that won't get the alignment.

If the attribute was not just type sugar and could be made part of the canonical type, then the attribute would have to be attached to the canonical type of the alias when building it,
in a similar way to how you don't lose other structural parts of the type when canonicalizing it.

But then you would have to deal with the whole mess this would cause in the rest of clang :)

mizvekov added inline comments.Thu, Oct 7, 10:01 AM
clang/lib/Sema/SemaTemplateDeduction.cpp
242

I have a proposal for what to do here in this patch: https://reviews.llvm.org/D111283

I think it's ready except that it needs more manual labor to support digging down into more types.

Anyway, this patch above refines the behavior of the present one, by implementing a criteria to fold the type sugar when deduction happens from multiple arguments,
or in the case of deducing from auto returning function, to fold them from the multiple return statements.

Otherwise, as in the present patch, the sugar is considered only from the first argument deduced from.

So any stakeholders, please subscribe and provide feedback about the rules as they are implemented :)

mizvekov updated this revision to Diff 378836.Mon, Oct 11, 5:24 PM
mizvekov edited the summary of this revision. (Show Details)

rebase.