Skip to content

Commit 801b268

Browse files
beccadaxbeccadax
committed
Emit access note remarks only for valid attributes
This requires deferring emission until the end of typechecking. A future change will emit access-note-related notes when an attribute is invalidated.
1 parent 769ea4a commit 801b268

File tree

8 files changed

+309
-201
lines changed

8 files changed

+309
-201
lines changed

‎include/swift/AST/SourceFile.h

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -246,6 +246,12 @@ class SourceFile final : public FileUnit {
246246
/// List of Objective-C member conflicts we have found during type checking.
247247
std::vector<ObjCMethodConflict> ObjCMethodConflicts;
248248

249+
using DeclAndAttr = std::tuple<ValueDecl *, DeclAttribute *>;
250+
251+
/// List of attributes added by access notes, used to emit remarks for valid
252+
/// ones.
253+
std::vector<DeclAndAttr> AttrsAddedByAccessNotes;
254+
249255
/// Describes what kind of file this is, which can affect some type checking
250256
/// and other behavior.
251257
const SourceFileKind Kind;

‎lib/Sema/TypeCheckDecl.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,8 @@ void validatePrecedenceGroup(PrecedenceGroupDecl *PGD);
5959
bool checkDesignatedTypes(OperatorDecl *OD,
6060
ArrayRef<Located<Identifier>> identifiers);
6161

62+
void diagnoseAttrsAddedByAccessNote(SourceFile &SF);
63+
6264
}
6365

6466
#endif

‎lib/Sema/TypeCheckDeclObjC.cpp

Lines changed: 68 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,11 @@ static void describeObjCReason(const ValueDecl *VD, ObjCReason Reason) {
124124
}
125125
}
126126

127+
void ObjCReason::setAttrInvalid() const {
128+
if (requiresAttr(kind))
129+
getAttr()->setInvalid();
130+
}
131+
127132
static void diagnoseTypeNotRepresentableInObjC(const DeclContext *DC,
128133
Type T,
129134
SourceRange TypeRange,
@@ -588,49 +593,61 @@ bool swift::isRepresentableInObjC(
588593
// Accessors can only be @objc if the storage declaration is.
589594
// Global computed properties may however @_cdecl their accessors.
590595
auto storage = accessor->getStorage();
591-
if (!storage->isObjC() && Reason != ObjCReason::ExplicitlyCDecl &&
592-
Reason != ObjCReason::WitnessToObjC &&
593-
Reason != ObjCReason::MemberOfObjCProtocol) {
594-
if (accessor->isGetter()) {
596+
bool storageIsObjC = storage->isObjC()
597+
|| Reason == ObjCReason::ExplicitlyCDecl
598+
|| Reason == ObjCReason::WitnessToObjC
599+
|| Reason == ObjCReason::MemberOfObjCProtocol;
600+
601+
switch (accessor->getAccessorKind()) {
602+
case AccessorKind::DidSet:
603+
case AccessorKind::WillSet: {
604+
// willSet/didSet implementations are never exposed to objc, they are
605+
// always directly dispatched from the synthesized setter.
606+
diagnoseAndRemoveAttr(accessor, Reason.getAttr(),
607+
diag::objc_observing_accessor)
608+
.limitBehavior(behavior);
609+
describeObjCReason(accessor, Reason);
610+
return false;
611+
}
612+
613+
case AccessorKind::Get:
614+
if (!storageIsObjC) {
595615
auto error = isa<VarDecl>(storage)
596616
? diag::objc_getter_for_nonobjc_property
597617
: diag::objc_getter_for_nonobjc_subscript;
598618

599-
accessor->diagnose(error).limitBehavior(behavior);
619+
diagnoseAndRemoveAttr(accessor, Reason.getAttr(), error)
620+
.limitBehavior(behavior);
600621
describeObjCReason(accessor, Reason);
601-
} else if (accessor->isSetter()) {
622+
return false;
623+
}
624+
return true;
625+
626+
case AccessorKind::Set:
627+
if (!storageIsObjC) {
602628
auto error = isa<VarDecl>(storage)
603629
? diag::objc_setter_for_nonobjc_property
604630
: diag::objc_setter_for_nonobjc_subscript;
605631

606-
accessor->diagnose(error).limitBehavior(behavior);
632+
diagnoseAndRemoveAttr(accessor, Reason.getAttr(), error)
633+
.limitBehavior(behavior);
607634
describeObjCReason(accessor, Reason);
635+
return false;
608636
}
609-
return false;
610-
}
611-
612-
switch (accessor->getAccessorKind()) {
613-
case AccessorKind::DidSet:
614-
case AccessorKind::WillSet:
615-
// willSet/didSet implementations are never exposed to objc, they are
616-
// always directly dispatched from the synthesized setter.
617-
accessor->diagnose(diag::objc_observing_accessor).limitBehavior(behavior);
618-
describeObjCReason(accessor, Reason);
619-
return false;
620-
621-
case AccessorKind::Get:
622-
case AccessorKind::Set:
623637
return true;
624638

625639
case AccessorKind::Address:
626640
case AccessorKind::MutableAddress:
627-
accessor->diagnose(diag::objc_addressor).limitBehavior(behavior);
641+
diagnoseAndRemoveAttr(accessor, Reason.getAttr(), diag::objc_addressor)
642+
.limitBehavior(behavior);
628643
describeObjCReason(accessor, Reason);
629644
return false;
630645

631646
case AccessorKind::Read:
632647
case AccessorKind::Modify:
633-
accessor->diagnose(diag::objc_coroutine_accessor).limitBehavior(behavior);
648+
diagnoseAndRemoveAttr(accessor, Reason.getAttr(),
649+
diag::objc_coroutine_accessor)
650+
.limitBehavior(behavior);
634651
describeObjCReason(accessor, Reason);
635652
return false;
636653
}
@@ -1145,9 +1162,9 @@ static bool isMemberOfObjCMembersClass(const ValueDecl *VD) {
11451162

11461163
ObjCReason swift::objCReasonForObjCAttr(const ObjCAttr *attr) {
11471164
if (attr->getAddedByAccessNote())
1148-
return ObjCReason::ExplicitlyObjCByAccessNote;
1165+
return ObjCReason(ObjCReason::ExplicitlyObjCByAccessNote, attr);
11491166

1150-
return ObjCReason::ExplicitlyObjC;
1167+
return ObjCReason(ObjCReason::ExplicitlyObjC, attr);
11511168
}
11521169

11531170
// A class is @objc if it does not have generic ancestry, and it either has
@@ -1325,18 +1342,22 @@ Optional<ObjCReason> shouldMarkAsObjC(const ValueDecl *VD, bool allowImplicit) {
13251342
//
13261343
// @IBInspectable and @GKInspectable imply @objc quietly in Swift 3
13271344
// (where they warn on failure) and loudly in Swift 4 (error on failure).
1328-
if (VD->getAttrs().hasAttribute<IBOutletAttr>())
1329-
return ObjCReason(ObjCReason::ExplicitlyIBOutlet);
1330-
if (VD->getAttrs().hasAttribute<IBActionAttr>())
1331-
return ObjCReason(ObjCReason::ExplicitlyIBAction);
1332-
if (VD->getAttrs().hasAttribute<IBSegueActionAttr>())
1333-
return ObjCReason(ObjCReason::ExplicitlyIBSegueAction);
1334-
if (VD->getAttrs().hasAttribute<IBInspectableAttr>())
1335-
return ObjCReason(ObjCReason::ExplicitlyIBInspectable);
1336-
if (VD->getAttrs().hasAttribute<GKInspectableAttr>())
1337-
return ObjCReason(ObjCReason::ExplicitlyGKInspectable);
1338-
if (VD->getAttrs().hasAttribute<NSManagedAttr>())
1339-
return ObjCReason(ObjCReason::ExplicitlyNSManaged);
1345+
if (auto attr = VD->getAttrs().getAttribute<IBOutletAttr>())
1346+
return ObjCReason(ObjCReason::ExplicitlyIBOutlet, attr);
1347+
if (auto attr = VD->getAttrs().getAttribute<IBActionAttr>())
1348+
return ObjCReason(ObjCReason::ExplicitlyIBAction, attr);
1349+
if (auto attr = VD->getAttrs().getAttribute<IBSegueActionAttr>())
1350+
return ObjCReason(ObjCReason::ExplicitlyIBSegueAction, attr);
1351+
if (auto attr = VD->getAttrs().getAttribute<IBInspectableAttr>())
1352+
return ObjCReason(ObjCReason::ExplicitlyIBInspectable, attr);
1353+
if (auto attr = VD->getAttrs().getAttribute<GKInspectableAttr>())
1354+
return ObjCReason(ObjCReason::ExplicitlyGKInspectable, attr);
1355+
if (auto attr = VD->getAttrs().getAttribute<NSManagedAttr>()) {
1356+
// Make sure the implicit DynamicAttr gets added before we potentially
1357+
// disable this attribute.
1358+
(void) VD->isDynamic();
1359+
return ObjCReason(ObjCReason::ExplicitlyNSManaged, attr);
1360+
}
13401361
// A member of an @objc protocol is implicitly @objc.
13411362
if (isMemberOfObjCProtocol) {
13421363
if (!VD->isProtocolRequirement())
@@ -1389,7 +1410,7 @@ Optional<ObjCReason> shouldMarkAsObjC(const ValueDecl *VD, bool allowImplicit) {
13891410
"@objc ");
13901411
}
13911412

1392-
return ObjCReason(ObjCReason::ExplicitlyDynamic);
1413+
return ObjCReason(ObjCReason::ExplicitlyDynamic, attr);
13931414
}
13941415
}
13951416

@@ -1584,17 +1605,23 @@ bool IsObjCRequest::evaluate(Evaluator &evaluator, ValueDecl *VD) const {
15841605
Optional<ForeignAsyncConvention> asyncConvention;
15851606
Optional<ForeignErrorConvention> errorConvention;
15861607
if (auto var = dyn_cast<VarDecl>(VD)) {
1587-
if (!isRepresentableInObjC(var, *isObjC))
1608+
if (!isRepresentableInObjC(var, *isObjC)) {
1609+
isObjC->setAttrInvalid();
15881610
return false;
1611+
}
15891612
} else if (auto subscript = dyn_cast<SubscriptDecl>(VD)) {
1590-
if (!isRepresentableInObjC(subscript, *isObjC))
1613+
if (!isRepresentableInObjC(subscript, *isObjC)) {
1614+
isObjC->setAttrInvalid();
15911615
return false;
1616+
}
15921617
} else if (isa<DestructorDecl>(VD)) {
15931618
// Destructors need no additional checking.
15941619
} else if (auto func = dyn_cast<AbstractFunctionDecl>(VD)) {
15951620
if (!isRepresentableInObjC(
1596-
func, *isObjC, asyncConvention, errorConvention))
1621+
func, *isObjC, asyncConvention, errorConvention)) {
1622+
isObjC->setAttrInvalid();
15971623
return false;
1624+
}
15981625
}
15991626

16001627
// Note that this declaration is exposed to Objective-C.

‎lib/Sema/TypeCheckDeclPrimary.cpp

Lines changed: 31 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1450,14 +1450,10 @@ static void addOrRemoveAttr(ValueDecl *VD, const AccessNotesFile &notes,
14501450
attr->setAddedByAccessNote();
14511451
VD->getAttrs().add(attr);
14521452

1453-
SmallString<64> attrString;
1454-
llvm::raw_svector_ostream os(attrString);
1455-
attr->print(os, VD);
1456-
1457-
diagnoseChangeByAccessNote(diag::attr_added_by_access_note,
1458-
diag::fixit_attr_added_by_access_note)
1459-
.fixItInsert(VD->getAttributeInsertionLoc(attr->isDeclModifier()),
1460-
attrString);
1453+
// Arrange for us to emit a remark about this attribute after type checking
1454+
// has ensured it's valid.
1455+
if (auto SF = VD->getDeclContext()->getParentSourceFile())
1456+
SF->AttrsAddedByAccessNotes.emplace_back(VD, attr);
14611457
} else {
14621458
VD->getAttrs().removeAttribute(attr);
14631459
diagnoseChangeByAccessNote(diag::attr_removed_by_access_note,
@@ -1518,6 +1514,29 @@ void TypeChecker::applyAccessNote(ValueDecl *VD) {
15181514
ApplyAccessNoteRequest{VD}, {});
15191515
}
15201516

1517+
void swift::diagnoseAttrsAddedByAccessNote(SourceFile &SF) {
1518+
for (auto declAndAttr : SF.AttrsAddedByAccessNotes) {
1519+
auto VD = std::get<0>(declAndAttr);
1520+
auto attr = std::get<1>(declAndAttr);
1521+
1522+
if (attr->isInvalid()) continue;
1523+
assert(attr->getAddedByAccessNote());
1524+
1525+
bool isModifier = attr->isDeclModifier();
1526+
1527+
auto reason = VD->getModuleContext()->getAccessNotes().Reason;
1528+
VD->diagnose(diag::attr_added_by_access_note, reason, isModifier,
1529+
attr->getAttrName(), VD->getDescriptiveKind());
1530+
// VD->getModuleContext()->getAccessNotes().noteReason(VD);
1531+
1532+
SmallString<64> attrString;
1533+
llvm::raw_svector_ostream os(attrString);
1534+
attr->print(os, VD);
1535+
VD->diagnose(diag::fixit_attr_added_by_access_note, isModifier)
1536+
.fixItInsert(VD->getAttributeInsertionLoc(isModifier), attrString);
1537+
}
1538+
}
1539+
15211540
evaluator::SideEffect
15221541
ApplyAccessNoteRequest::evaluate(Evaluator &evaluator, ValueDecl *VD) const {
15231542
AccessNotesFile &notes = VD->getModuleContext()->getAccessNotes();
@@ -2680,8 +2699,8 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
26802699
if (auto CDeclAttr = FD->getAttrs().getAttribute<swift::CDeclAttr>()) {
26812700
Optional<ForeignAsyncConvention> asyncConvention;
26822701
Optional<ForeignErrorConvention> errorConvention;
2683-
if (isRepresentableInObjC(FD, ObjCReason::ExplicitlyCDecl,
2684-
asyncConvention, errorConvention)) {
2702+
ObjCReason reason(ObjCReason::ExplicitlyCDecl, CDeclAttr);
2703+
if (isRepresentableInObjC(FD, reason, asyncConvention, errorConvention)) {
26852704
if (FD->hasAsync()) {
26862705
FD->setForeignAsyncConvention(*asyncConvention);
26872706
getASTContext().Diags.diagnose(CDeclAttr->getLocation(),
@@ -2691,6 +2710,8 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
26912710
getASTContext().Diags.diagnose(CDeclAttr->getLocation(),
26922711
diag::cdecl_throws);
26932712
}
2713+
} else {
2714+
reason.setAttrInvalid();
26942715
}
26952716
}
26962717
}

‎lib/Sema/TypeCheckObjC.h

Lines changed: 48 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
#include "swift/AST/ForeignAsyncConvention.h"
2222
#include "swift/AST/ForeignErrorConvention.h"
2323
#include "llvm/ADT/Optional.h"
24+
#include "llvm/ADT/PointerUnion.h"
2425

2526
namespace swift {
2627

@@ -90,14 +91,49 @@ class ObjCReason {
9091
Kind kind;
9192

9293
/// When the kind is \c WitnessToObjC, the requirement being witnessed.
93-
ValueDecl * decl = nullptr;
94-
95-
ObjCReason(Kind kind, ValueDecl *decl) : kind(kind), decl(decl) { }
94+
llvm::PointerUnion<ValueDecl *, DeclAttribute *> declOrAttr =
95+
static_cast<DeclAttribute *>(nullptr);
96+
97+
ObjCReason(Kind kind, ValueDecl *decl) : kind(kind), declOrAttr(decl) { }
98+
99+
static bool requiresAttr(Kind kind) {
100+
switch (kind) {
101+
case ExplicitlyCDecl:
102+
case ExplicitlyDynamic:
103+
case ExplicitlyObjC:
104+
case ExplicitlyIBOutlet:
105+
case ExplicitlyIBAction:
106+
case ExplicitlyIBSegueAction:
107+
case ExplicitlyNSManaged:
108+
case ExplicitlyIBInspectable:
109+
case ExplicitlyGKInspectable:
110+
case ExplicitlyObjCByAccessNote:
111+
return true;
112+
113+
case MemberOfObjCProtocol:
114+
case ImplicitlyObjC:
115+
case OverridesObjC:
116+
case WitnessToObjC:
117+
case MemberOfObjCExtension:
118+
case MemberOfObjCMembersClass:
119+
case MemberOfObjCSubclass:
120+
case ElementOfObjCEnum:
121+
case Accessor:
122+
return false;
123+
}
124+
}
96125

97126
public:
98127
/// Implicit conversion from the trivial reason kinds.
99128
ObjCReason(Kind kind) : kind(kind) {
100129
assert(kind != WitnessToObjC && "Use ObjCReason::witnessToObjC()");
130+
assert(!requiresAttr(kind) && "Use ObjCReason(Kind, DeclAttribute*)");
131+
}
132+
133+
ObjCReason(Kind kind, const DeclAttribute *attr)
134+
: kind(kind), declOrAttr(const_cast<DeclAttribute *>(attr)) {
135+
// const_cast above because it's difficult to get a non-const DeclAttribute.
136+
assert(requiresAttr(kind) && "Use ObjCReason(Kind)");
101137
}
102138

103139
/// Retrieve the kind of requirement.
@@ -113,8 +149,16 @@ class ObjCReason {
113149
/// requirement, retrieve the requirement.
114150
ValueDecl *getObjCRequirement() const {
115151
assert(kind == WitnessToObjC);
116-
return decl;
152+
return declOrAttr.get<ValueDecl *>();
117153
}
154+
155+
DeclAttribute *getAttr() const {
156+
if (!requiresAttr(kind))
157+
return nullptr;
158+
return declOrAttr.get<DeclAttribute *>();
159+
}
160+
161+
void setAttrInvalid() const;
118162
};
119163

120164
/// Determine how to diagnose conflicts due to inferring @objc with this

‎lib/Sema/TypeChecker.cpp

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717

1818
#include "swift/Subsystems.h"
1919
#include "TypeChecker.h"
20+
#include "TypeCheckDecl.h"
2021
#include "TypeCheckObjC.h"
2122
#include "TypeCheckType.h"
2223
#include "CodeSynthesis.h"
@@ -335,6 +336,7 @@ void swift::performWholeModuleTypeChecking(SourceFile &SF) {
335336
diagnoseObjCMethodConflicts(SF);
336337
diagnoseObjCUnsatisfiedOptReqConflicts(SF);
337338
diagnoseUnintendedObjCMethodOverrides(SF);
339+
diagnoseAttrsAddedByAccessNote(SF);
338340
return;
339341
case SourceFileKind::SIL:
340342
case SourceFileKind::Interface:

0 commit comments

Comments
 (0)