Page MenuHomePhabricator

[clang] Implement P0692R1 from C++20 (access checking on specializations and instantiations)
Needs ReviewPublic

Authored by aorlov on Nov 24 2020, 5:59 AM.

Details

Summary

This patch implements paper P0692R1 from the C++20 standard. Disable usual access checking rules to template argument names in a declaration of partial specializations, explicit instantiation or explicit specialization (C++20 13.7.5/10, 13.9.1/6).

Fixes: https://llvm.org/PR37424

This patch also implements option *A* from this paper P0692R1 from the C++20 standard.

This patch follows the @rsmith suggestion from D78404.

Diff Detail

Event Timeline

aorlov created this revision.Nov 24 2020, 5:59 AM
aorlov requested review of this revision.Nov 24 2020, 5:59 AM
aorlov updated this revision to Diff 307461.Nov 24 2020, 3:14 PM
aorlov retitled this revision from [clang] Partially implement P0692R1 from C++20 (access checking on specializations) to [clang] Implement P0692R1 from C++20 (access checking on specializations and instantiations).
aorlov edited the summary of this revision. (Show Details)
aorlov updated this revision to Diff 308373.Nov 30 2020, 8:07 AM

Improved solution. Added more tests.

aorlov updated this revision to Diff 309252.Dec 3 2020, 7:32 AM

Fixed typos. Made minor changes in test cases.

The subject line says "access checking on specializations and instantiations," but I don't see any tests for explicit instantiations here — just specializations. In particular, I'm very interested to know if P0692 is intended to have any effect on the legality of https://godbolt.org/z/fqfo8q

template<int*> struct T {};
template struct T<&S::private_static_data>;

It would also be good to document which of the two proposed wordings from http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0692r1.html is actually being adopted in this patch.

aorlov edited the summary of this revision. (Show Details)Dec 4 2020, 11:24 PM
aorlov added a comment.EditedDec 4 2020, 11:28 PM

The subject line says "access checking on specializations and instantiations," but I don't see any tests for explicit instantiations here — just specializations. In particular, I'm very interested to know if P0692 is intended to have any effect on the legality of https://godbolt.org/z/fqfo8q

Thank you for your important suggestion! I'll add test cases for explicit instantiations.

It would also be good to document which of the two proposed wordings from http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0692r1.html is actually being adopted in this patch.

AFAIK there are four options in the paper. This patch covers option *A*. I'll mention this in the summary.
Both wordings relate to option A only, thus both wordings have been adopted.

aorlov updated this revision to Diff 309959.Dec 7 2020, 10:59 AM

Added test cases for explicit instantiations.

@Quuxplusone,
In particular, I'm very interested to know if P0692 is intended to have any effect on the legality of https://godbolt.org/z/fqfo8q

I've checked your particular case. It hasn't been affected by this patch. It works as before. I also added it to func.spec.cpp.

Quuxplusone added inline comments.Dec 7 2020, 11:14 AM
clang/lib/Parse/ParseTemplate.cpp
172

I don't know the purpose of this code, but this seems like a super important TODO.

The comment "look ahead until {" is also scary because

template<int I, int J> struct A {};
template<int J> struct A<int{}, J> {};

is also a partial specialization. Why do we need to know isSpecialization, who's supposed to compute it, and why does their answer need to be fixed-up right here?

And is this specific piece of the patch perhaps severable into a separate review? Again, I don't know this code, but... It seems like you've got one part of the patch — "add a SuppressAccessGuard around the call to ParseDeclarator" — which is either a 100% correct or 100% incorrect implementation of P0692R1. And then you've got this other piece, a parser hack, which looks much more heuristic and incomplete in nature, and looks orthogonal to P0692R1.

Btw, I'm not an approver on this (and shouldn't be); if you want better review you might want to ping someone who's touched this code lately (according to git blame).

aorlov added inline comments.Dec 8 2020, 8:29 AM
clang/lib/Parse/ParseTemplate.cpp
172

@Quuxplusone

I don't know the purpose of this code, but this seems like a super important TODO.

This is a deprecated hint, I'll remove it.

The comment "look ahead until {" is also scary

Nice case! I'll improve the patch.

Why do we need to know isSpecialization

This flag helps to turn off usual access rules further:

Parser::ParseClassSpecifier(... const ParsedTemplateInfo &TemplateInfo, ...) {
...
  bool shouldDelayDiagsInTag =
    (TemplateInfo.Kind == ParsedTemplateInfo::ExplicitInstantiation ||
     TemplateInfo.Kind == ParsedTemplateInfo::ExplicitSpecialization);
  SuppressAccessChecks diagsFromTag(*this, shouldDelayDiagsInTag);
...
}

And then you've got this other piece, a parser hack,

Totally agree. It also looks like a hack for me. The point is that the principle of parser's workflow is that it analyzes tokens and then consume them one by one at the same time. So we are not confident whether we are parsing a primary template or specialization. Thus TemplateInfo.Kind is not always correct. To define a specialization we should look forward at the whole declaraion without consuming any tokens to leave them for further parsing but set a correct 'Kind'.
On the other side we can somehow store the context of we are parsing a template declaration and throw it through a bunch of calls. But I found this way much more complicated, than of looking at some next tokens. BTW @rsmith suggested this approach in the comment https://reviews.llvm.org/D78404#inline-717620

Btw, I'm not an approver on this (and shouldn't be); if you want better review you might want to ping someone who's touched this code lately (according to git blame).

Thank you. I've already added all related people in the list of reviewers.

aorlov updated this revision to Diff 310416.Dec 8 2020, 9:04 PM

Simplify the patch.
@Quuxplusone,
Actually you've pushed me to some thinking of what more syntactic cases it could be. And I came to that we can get rid of this hack and simplify the patch pretty much. Hope, this fix will be more admissible.

And I came to that we can get rid of this hack and simplify the patch pretty much

Wow, this certainly looks massively better than it was before! :) Glad I could help. ;)

aorlov updated this revision to Diff 311431.Dec 13 2020, 12:55 AM
aorlov added reviewers: mibintc, hokein.
aorlov added subscribers: mibintc, hokein.

Updated. Disabled function parameters access checking in function templates.
Hi, @broadwaylamb, @rsmith, @saar.raz, @doug.gregor, @mibintc, @hokein. Could you please look at this patch?

mibintc resigned from this revision.Dec 14 2020, 7:21 AM

I don't have expertise in this area.

Hi, community. I kindly ask you to review this patch.

One more ping!

Please, somebody look at this patch.

Quuxplusone added inline comments.Dec 23 2020, 10:03 AM
clang/lib/Parse/ParseTemplate.cpp
316

Spelling: /supresses/suppresses/, /dignostics kinds/kinds of diagnostics/

clang/test/CXX/class.access/class.friend/p1.cpp
290 ↗(On Diff #311431)

This whitespace diff is gratuitous.

294 ↗(On Diff #311431)

IMHO it would make sense to add several other variations here too:

template <> A::I g2<char>(A::I i); // expected-error {{is a private member of}}
template <> int g2<A::I>(int i);  // expected-error??
template <> A::I g2<A::I*>(A::I i);  // expected-error??
template A::I g2<char>(A::I i); // expected-error {{is a private member of}}
template int g2<A::I**>(int i);  // OK
template A::I g2<A::I***>(A::I i);  // OK??

It might also be better to use a well-formed template template<class> int g3(int) { return 0; } instead of g2 for the explicit specializations, and to use yet another template template<class> int g4(int) { return 0; } instead of g2 for the explicit instantiation declarations. That way there's no accidental "cross-talk" between the various declarations that might affect what diagnostics get printed.

clang/test/CXX/temp/temp.spec/part.spec.cpp
11

I wonder if this test can be shortened and/or removed, now that you're not doing weird parser tricks that need exhaustive testing. E.g. at least the stuff with the final keyword could be removed now, right?

aorlov updated this revision to Diff 315840.Jan 11 2021, 9:29 AM

@Quuxplusone
Thank you for your comments. I updated the patch according to your suggestions.
Does anyone else from the review list want to waste some time to look at this patch? I would appreciate this.

clang/test/CXX/class.access/class.friend/p1.cpp
290 ↗(On Diff #311431)

This is done by clang-format. I think we should keep it here.

Just a ping.

Please, look at my solution. Is it worth to be accepted?

One more ping. Please, pay attention to this patch.

aorlov added a comment.Feb 8 2021, 6:26 AM

Hi, community. I kindly ask you to review this patch.

Ping! Please, don't pass by this patch. I need your competent evaluation to load it!

Just one more ping!

Ping! Please, review this patch!

krisb added a subscriber: krisb.Apr 7 2021, 2:46 AM
krisb added inline comments.
clang/lib/Parse/ParseDecl.cpp
3001

It seems to be a change unrelated to the patch.

clang/lib/Parse/ParseDeclCXX.cpp
1414

The comment needs to be updated.

clang/test/CXX/temp/temp.spec/func.spec.cpp
96

Formatting issue here.

106

Before this patch clang diagnosed cases like

class A { class C {}; };
template <typename T> void func(A::C) {}

Why is it no longer the case?

@krisb Thank you for your comments. I will consider them.

clang/test/CXX/temp/temp.spec/func.spec.cpp
106

Your example generates an error error: 'C' is a private member of 'A' on latest clang.
This patch is intended to implement a proposal p0692r1 and consider this example as valid.
Did I understand your question correctly?

krisb added a comment.Apr 8 2021, 2:09 PM

Do we still need the following tests:

  • clang/test/CXX/temp/temp.spec/temp.explicit/p11.cpp
  • clang/test/CXX/temp/temp.spec/temp.explicit/p12.cpp

?

clang/test/CXX/temp/temp.spec/func.spec.cpp
106

It doesn't seem the aforementioned example falls under any of the cases from P0692R1 unless I misinterpreted its wordings.
It's a primary template, right?
Consider also the following example for which clang issues the error in both cases (before and after the patch):

class A { class C {}; };
template <class T> A::C func();

I'm also wondering about something like:

class A { class C {}; };
template <typename T> T func() {}
template <> A::C func<A::C>();
clang/test/CXX/temp/temp.spec/part.spec.cpp
4

It seems this test checks more than just partial specialization cases.
Could you please align the comments with what the test actually tests?

@krisb

Do we still need the following tests:

  • clang/test/CXX/temp/temp.spec/temp.explicit/p11.cpp
  • clang/test/CXX/temp/temp.spec/temp.explicit/p12.cpp

?

We can remove p11.cpp and partially p12.cpp, because p12 has another kind of test in namespace test0{ ... }.

aorlov added inline comments.Apr 14 2021, 4:27 AM
clang/test/CXX/temp/temp.spec/func.spec.cpp
106

Oh, I caught the point.
The paper says that the usual access checking rules do not apply to:

  • partial specialization
  • explicit instantiation
  • explicit specialization

Also it comes out from the Standard that the usual access checking rules applies to primary templates, only if they are not declared as friends.

class A { class C {}; };
template <typename T> void func(A::C) {}

It's a primary template, right?

Yes, it is a primary template. The access checking rules should work here. But they do not after this patch. This is a mistake I will sort out.

Consider also the following example for which clang issues the error in both cases (before and after the patch):

class A { class C {}; };
template <class T> A::C func();

This is also a primary template and should be handled with the access rules. The patch turns it off. This is a mistake. I'll handle.

I'm also wondering about something like:

class A { class C {}; };
template <typename T> T func() {}
template <> A::C func<A::C>();

This is an explicit specialization. It should not issue any errors here. The patch handles it correctly. It's OK.

aorlov updated this revision to Diff 340095.Apr 23 2021, 10:04 AM
aorlov updated this revision to Diff 340099.Apr 23 2021, 10:11 AM

Updated. @krisb, please, verify.

krisb added a comment.Apr 28 2021, 3:07 AM

Thank you! It looks more consistent now.

clang/lib/Parse/ParseDecl.cpp
3012

Basically, if __attribute((unavailable)) should trigger the error for any use of an unavailable class, we have it already broken.
For example, for this code clang-12 doesn't produce any diagnostics:

class __attribute((unavailable)) X {
  template <typename T> class __attribute((unavailable)) Y {};
};
class __attribute((unavailable)) A { 
    class __attribute((unavailable)) C {}; 
};
template <> class X::Y<A::C> {};

So, I don't see much sense in inventing something new to workaround only the cases that come with this patch. It's better to either fix it globally or leave it broken atm with the corresponding FIXME.

clang/test/CXX/temp/temp.spec/func.spec.cpp
55

Formatting seems broken here and below.

@krisb
Thanks for the review.

clang/lib/Parse/ParseDecl.cpp
3012

Anyway, I tried to remove the access diagnostics only. Who knows which diagnostics may be needed. Or you strongly prefer using SuppressAccessChecks instead?

clang/test/CXX/temp/temp.spec/func.spec.cpp
55

This is clang-format fault. I wrote it at different lines. Just has to add a line between.

krisb added inline comments.May 3 2021, 4:27 AM
clang/lib/Parse/ParseDecl.cpp
3012

Basically, I'd prefer consistency. Having two different ways of doing (almost) the same things doesn't look good to me. But I'm not sure about SuppressAccessChecks as turns all kinds of diagnostics off. Did you consider a possibility to improve SuppressAccessChecks to disable only a particular (specified) type of diagnostics?

aorlov updated this revision to Diff 346289.May 18 2021, 3:53 PM

Simplified the solution. Replaced RemoveDiagnosticsFromPool with SuppressAccessChecks.
@krisb, please, look.

Just a ping!

aorlov updated this revision to Diff 354193.Jun 24 2021, 3:40 AM