From 1a2d5c6f5a7fb4f5b88fbc821c39e7e4485464c7 Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Mon, 24 Sep 2018 23:17:44 +0000 Subject: [PATCH] P0962R1: only use the member form of 'begin' and 'end' in a range-based for loop if both members exist. This resolves a DR whereby an errant 'begin' or 'end' member in a base class could result in a derived class not being usable as a range with non-member 'begin' and 'end'. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@342925 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/Basic/DiagnosticSemaKinds.td | 4 +- lib/Sema/SemaStmt.cpp | 146 ++++++++++++------ .../stmt.stmt/stmt.iter/stmt.ranged/p1.cpp | 59 ++++++- test/SemaCXX/for-range-dereference.cpp | 6 +- 4 files changed, 157 insertions(+), 58 deletions(-) diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td index c279d56b62..a4c0f973fa 100644 --- a/include/clang/Basic/DiagnosticSemaKinds.td +++ b/include/clang/Basic/DiagnosticSemaKinds.td @@ -2254,8 +2254,6 @@ def err_for_range_incomplete_type : Error< "cannot use incomplete type %0 as a range">; def err_for_range_iter_deduction_failure : Error< "cannot use type %0 as an iterator">; -def err_for_range_member_begin_end_mismatch : Error< - "range type %0 has '%select{begin|end}1' member but no '%select{end|begin}1' member">; def ext_for_range_begin_end_types_differ : ExtWarn< "'begin' and 'end' returning different types (%0 and %1) is a C++17 extension">, InGroup; @@ -2268,6 +2266,8 @@ def note_in_for_range: Note< def err_for_range_invalid: Error< "invalid range expression of type %0; no viable '%select{begin|end}1' " "function available">; +def note_for_range_member_begin_end_ignored : Note< + "member is not a candidate because range type %0 has no '%select{end|begin}1' member">; def err_range_on_array_parameter : Error< "cannot build range expression with array function parameter %0 since " "parameter with array type %1 is treated as pointer type %2">; diff --git a/lib/Sema/SemaStmt.cpp b/lib/Sema/SemaStmt.cpp index 61c6b37b4c..ed2fe4e3af 100644 --- a/lib/Sema/SemaStmt.cpp +++ b/lib/Sema/SemaStmt.cpp @@ -2149,6 +2149,56 @@ BuildNonArrayForRange(Sema &SemaRef, Expr *BeginRange, Expr *EndRange, Sema::LookupMemberName); LookupResult EndMemberLookup(SemaRef, EndNameInfo, Sema::LookupMemberName); + auto BuildBegin = [&] { + *BEF = BEF_begin; + Sema::ForRangeStatus RangeStatus = + SemaRef.BuildForRangeBeginEndCall(ColonLoc, ColonLoc, BeginNameInfo, + BeginMemberLookup, CandidateSet, + BeginRange, BeginExpr); + + if (RangeStatus != Sema::FRS_Success) { + if (RangeStatus == Sema::FRS_DiagnosticIssued) + SemaRef.Diag(BeginRange->getBeginLoc(), diag::note_in_for_range) + << ColonLoc << BEF_begin << BeginRange->getType(); + return RangeStatus; + } + if (!CoawaitLoc.isInvalid()) { + // FIXME: getCurScope() should not be used during template instantiation. + // We should pick up the set of unqualified lookup results for operator + // co_await during the initial parse. + *BeginExpr = SemaRef.ActOnCoawaitExpr(SemaRef.getCurScope(), ColonLoc, + BeginExpr->get()); + if (BeginExpr->isInvalid()) + return Sema::FRS_DiagnosticIssued; + } + if (FinishForRangeVarDecl(SemaRef, BeginVar, BeginExpr->get(), ColonLoc, + diag::err_for_range_iter_deduction_failure)) { + NoteForRangeBeginEndFunction(SemaRef, BeginExpr->get(), *BEF); + return Sema::FRS_DiagnosticIssued; + } + return Sema::FRS_Success; + }; + + auto BuildEnd = [&] { + *BEF = BEF_end; + Sema::ForRangeStatus RangeStatus = + SemaRef.BuildForRangeBeginEndCall(ColonLoc, ColonLoc, EndNameInfo, + EndMemberLookup, CandidateSet, + EndRange, EndExpr); + if (RangeStatus != Sema::FRS_Success) { + if (RangeStatus == Sema::FRS_DiagnosticIssued) + SemaRef.Diag(EndRange->getBeginLoc(), diag::note_in_for_range) + << ColonLoc << BEF_end << EndRange->getType(); + return RangeStatus; + } + if (FinishForRangeVarDecl(SemaRef, EndVar, EndExpr->get(), ColonLoc, + diag::err_for_range_iter_deduction_failure)) { + NoteForRangeBeginEndFunction(SemaRef, EndExpr->get(), *BEF); + return Sema::FRS_DiagnosticIssued; + } + return Sema::FRS_Success; + }; + if (CXXRecordDecl *D = RangeType->getAsCXXRecordDecl()) { // - if _RangeT is a class type, the unqualified-ids begin and end are // looked up in the scope of class _RangeT as if by class member access @@ -2156,68 +2206,62 @@ BuildNonArrayForRange(Sema &SemaRef, Expr *BeginRange, Expr *EndRange, // declaration, begin-expr and end-expr are __range.begin() and // __range.end(), respectively; SemaRef.LookupQualifiedName(BeginMemberLookup, D); + if (BeginMemberLookup.isAmbiguous()) + return Sema::FRS_DiagnosticIssued; + SemaRef.LookupQualifiedName(EndMemberLookup, D); + if (EndMemberLookup.isAmbiguous()) + return Sema::FRS_DiagnosticIssued; if (BeginMemberLookup.empty() != EndMemberLookup.empty()) { - SourceLocation RangeLoc = BeginVar->getLocation(); - *BEF = BeginMemberLookup.empty() ? BEF_end : BEF_begin; - - SemaRef.Diag(RangeLoc, diag::err_for_range_member_begin_end_mismatch) - << RangeLoc << BeginRange->getType() << *BEF; - return Sema::FRS_DiagnosticIssued; + // Look up the non-member form of the member we didn't find, first. + // This way we prefer a "no viable 'end'" diagnostic over a "i found + // a 'begin' but ignored it because there was no member 'end'" + // diagnostic. + auto BuildNonmember = [&]( + BeginEndFunction BEFFound, LookupResult &Found, + llvm::function_ref BuildFound, + llvm::function_ref BuildNotFound) { + LookupResult OldFound = std::move(Found); + Found.clear(); + + if (Sema::ForRangeStatus Result = BuildNotFound()) + return Result; + + switch (BuildFound()) { + case Sema::FRS_Success: + return Sema::FRS_Success; + + case Sema::FRS_NoViableFunction: + SemaRef.Diag(BeginRange->getBeginLoc(), diag::err_for_range_invalid) + << BeginRange->getType() << BEFFound; + CandidateSet->NoteCandidates(SemaRef, OCD_AllCandidates, BeginRange); + LLVM_FALLTHROUGH; + + case Sema::FRS_DiagnosticIssued: + for (NamedDecl *D : OldFound) { + SemaRef.Diag(D->getLocation(), + diag::note_for_range_member_begin_end_ignored) + << BeginRange->getType() << BEFFound; + } + return Sema::FRS_DiagnosticIssued; + } + llvm_unreachable("unexpected ForRangeStatus"); + }; + if (BeginMemberLookup.empty()) + return BuildNonmember(BEF_end, EndMemberLookup, BuildEnd, BuildBegin); + return BuildNonmember(BEF_begin, BeginMemberLookup, BuildBegin, BuildEnd); } } else { // - otherwise, begin-expr and end-expr are begin(__range) and // end(__range), respectively, where begin and end are looked up with // argument-dependent lookup (3.4.2). For the purposes of this name // lookup, namespace std is an associated namespace. - } - *BEF = BEF_begin; - Sema::ForRangeStatus RangeStatus = - SemaRef.BuildForRangeBeginEndCall(ColonLoc, ColonLoc, BeginNameInfo, - BeginMemberLookup, CandidateSet, - BeginRange, BeginExpr); - - if (RangeStatus != Sema::FRS_Success) { - if (RangeStatus == Sema::FRS_DiagnosticIssued) - SemaRef.Diag(BeginRange->getBeginLoc(), diag::note_in_for_range) - << ColonLoc << BEF_begin << BeginRange->getType(); - return RangeStatus; - } - if (!CoawaitLoc.isInvalid()) { - // FIXME: getCurScope() should not be used during template instantiation. - // We should pick up the set of unqualified lookup results for operator - // co_await during the initial parse. - *BeginExpr = SemaRef.ActOnCoawaitExpr(SemaRef.getCurScope(), ColonLoc, - BeginExpr->get()); - if (BeginExpr->isInvalid()) - return Sema::FRS_DiagnosticIssued; - } - if (FinishForRangeVarDecl(SemaRef, BeginVar, BeginExpr->get(), ColonLoc, - diag::err_for_range_iter_deduction_failure)) { - NoteForRangeBeginEndFunction(SemaRef, BeginExpr->get(), *BEF); - return Sema::FRS_DiagnosticIssued; - } - - *BEF = BEF_end; - RangeStatus = - SemaRef.BuildForRangeBeginEndCall(ColonLoc, ColonLoc, EndNameInfo, - EndMemberLookup, CandidateSet, - EndRange, EndExpr); - if (RangeStatus != Sema::FRS_Success) { - if (RangeStatus == Sema::FRS_DiagnosticIssued) - SemaRef.Diag(EndRange->getBeginLoc(), diag::note_in_for_range) - << ColonLoc << BEF_end << EndRange->getType(); - return RangeStatus; - } - if (FinishForRangeVarDecl(SemaRef, EndVar, EndExpr->get(), ColonLoc, - diag::err_for_range_iter_deduction_failure)) { - NoteForRangeBeginEndFunction(SemaRef, EndExpr->get(), *BEF); - return Sema::FRS_DiagnosticIssued; - } - return Sema::FRS_Success; + if (Sema::ForRangeStatus Result = BuildBegin()) + return Result; + return BuildEnd(); } /// Speculatively attempt to dereference an invalid range expression. diff --git a/test/CXX/stmt.stmt/stmt.iter/stmt.ranged/p1.cpp b/test/CXX/stmt.stmt/stmt.iter/stmt.ranged/p1.cpp index 473c8b62ba..f42fc9b6e5 100644 --- a/test/CXX/stmt.stmt/stmt.iter/stmt.ranged/p1.cpp +++ b/test/CXX/stmt.stmt/stmt.iter/stmt.ranged/p1.cpp @@ -150,9 +150,9 @@ void g() { struct NoEnd { null_t begin(); }; - for (auto u : NoBegin()) { // expected-error {{range type 'NoBegin' has 'end' member but no 'begin' member}} + for (auto u : NoBegin()) { // expected-error {{no viable 'begin' function available}} } - for (auto u : NoEnd()) { // expected-error {{range type 'NoEnd' has 'begin' member but no 'end' member}} + for (auto u : NoEnd()) { // expected-error {{no viable 'end' function available}} } struct NoIncr { @@ -271,3 +271,58 @@ namespace rdar13712739 { template void foo(const int&); // expected-note{{in instantiation of function template specialization}} } + +namespace p0962r1 { + namespace NA { + struct A { + void begin(); + }; + int *begin(A); + int *end(A); + } + + namespace NB { + struct B { + void end(); + }; + int *begin(B); + int *end(B); + } + + namespace NC { + struct C { + void begin(); + }; + int *begin(C); + } + + namespace ND { + struct D { + void end(); + }; + int *end(D); + } + + namespace NE { + struct E { + void begin(); // expected-note {{member is not a candidate because range type 'p0962r1::NE::E' has no 'end' member}} + }; + int *end(E); + } + + namespace NF { + struct F { + void end(); // expected-note {{member is not a candidate because range type 'p0962r1::NF::F' has no 'begin' member}} + }; + int *begin(F); + } + + void use(NA::A a, NB::B b, NC::C c, ND::D d, NE::E e, NF::F f) { + for (auto x : a) {} + for (auto x : b) {} + for (auto x : c) {} // expected-error {{no viable 'end' function}} + for (auto x : d) {} // expected-error {{no viable 'begin' function}} + for (auto x : e) {} // expected-error {{no viable 'begin' function}} + for (auto x : f) {} // expected-error {{no viable 'end' function}} + } +} diff --git a/test/SemaCXX/for-range-dereference.cpp b/test/SemaCXX/for-range-dereference.cpp index 7377199024..de4958c31b 100644 --- a/test/SemaCXX/for-range-dereference.cpp +++ b/test/SemaCXX/for-range-dereference.cpp @@ -17,7 +17,7 @@ struct DeletedEnd : public T { struct DeletedADLBegin { }; int* begin(DeletedADLBegin) = delete; //expected-note {{candidate function has been explicitly deleted}} \ - expected-note 5 {{candidate function not viable: no known conversion}} + expected-note 6 {{candidate function not viable: no known conversion}} struct PrivateEnd { Data *begin(); @@ -27,7 +27,7 @@ struct PrivateEnd { }; struct ADLNoEnd { }; -Data * begin(ADLNoEnd); // expected-note 6 {{candidate function not viable: no known conversion}} +Data * begin(ADLNoEnd); // expected-note 7 {{candidate function not viable: no known conversion}} struct OverloadedStar { T operator*(); @@ -45,7 +45,7 @@ void f() { for (auto i : parr) { }// expected-error{{invalid range expression of type 'int (*)[10]'; did you mean to dereference it with '*'?}} NoBegin NB; - for (auto i : NB) { }// expected-error{{range type 'NoBegin' has 'end' member but no 'begin' member}} + for (auto i : NB) { }// expected-error{{invalid range expression of type 'NoBegin'; no viable 'begin' function available}} NoBegin *pNB; for (auto i : pNB) { }// expected-error{{invalid range expression of type 'NoBegin *'; no viable 'begin' function available}} NoBegin **ppNB; -- 2.40.0