Skip to content

Commit a0842b5

Browse files
beccadaxbeccadax
committed
Fix crasher in objcImpl diagnostic
`fixDeclarationObjCName()` accepted optional selectors, but crashed if they were None. Make these parameters non-optional, treat an invalid `targetName` selector as “no name”, and adjust callers to pass non-optional values. Re-enables decl/ext/objc_implementation.swift, which was disabled due to this bug. Fixes rdar://128683206.
1 parent 915b531 commit a0842b5

File tree

6 files changed

+45
-40
lines changed

6 files changed

+45
-40
lines changed

‎lib/Sema/TypeCheckAttr.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1607,7 +1607,8 @@ visitObjCImplementationAttr(ObjCImplementationAttr *attr) {
16071607
auto argListRange = getArgListRange(Ctx, attr);
16081608
if (argListRange.isValid()) {
16091609
diag.fixItRemove(argListRange);
1610-
fixDeclarationObjCName(diag, ED, objcLangAttr->getName(),
1610+
fixDeclarationObjCName(diag, ED,
1611+
objcLangAttr->getName().value_or(ObjCSelector()),
16111612
correctSelector);
16121613
}
16131614
objcLangAttr->setName(correctSelector, /*implicit=*/false);

‎lib/Sema/TypeCheckDeclObjC.cpp

Lines changed: 19 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1748,7 +1748,7 @@ static ObjCSelector inferObjCName(ValueDecl *decl) {
17481748
diag::objc_override_method_selector_mismatch,
17491749
*attr->getName(), overriddenSelector)
17501750
.limitBehavior(behavior));
1751-
fixDeclarationObjCName(diag, decl, attr->getName(),
1751+
fixDeclarationObjCName(diag, decl, attr->getName().value(),
17521752
overriddenSelector);
17531753
}
17541754

@@ -1838,8 +1838,9 @@ static ObjCSelector inferObjCName(ValueDecl *decl) {
18381838
auto diag = decl->diagnose(diag::objc_ambiguous_inference_candidate,
18391839
req, proto, *req->getObjCRuntimeName());
18401840
fixDeclarationObjCName(diag, decl,
1841-
decl->getObjCRuntimeName(/*skipIsObjC=*/true),
1842-
req->getObjCRuntimeName());
1841+
decl->getObjCRuntimeName(/*skipIsObjC=*/true)
1842+
.value_or(ObjCSelector()),
1843+
req->getObjCRuntimeName().value());
18431844
};
18441845
diagnoseCandidate(firstReq);
18451846
diagnoseCandidate(req);
@@ -2150,8 +2151,8 @@ bool swift::fixDeclarationName(InFlightDiagnostic &diag, const ValueDecl *decl,
21502151

21512152
bool swift::fixDeclarationObjCName(InFlightDiagnostic &diag,
21522153
const Decl *decl,
2153-
std::optional<ObjCSelector> nameOpt,
2154-
std::optional<ObjCSelector> targetNameOpt,
2154+
ObjCSelector name,
2155+
ObjCSelector targetName,
21552156
bool ignoreImpliedName) {
21562157
if (decl->isImplicit())
21572158
return false;
@@ -2163,9 +2164,6 @@ bool swift::fixDeclarationObjCName(InFlightDiagnostic &diag,
21632164
return false;
21642165
}
21652166

2166-
auto name = *nameOpt;
2167-
auto targetName = *targetNameOpt;
2168-
21692167
// Dig out the existing '@objc' attribute on the witness. We don't care
21702168
// about implicit ones because they don't have useful source location
21712169
// information.
@@ -2212,7 +2210,7 @@ bool swift::fixDeclarationObjCName(InFlightDiagnostic &diag,
22122210

22132211
// If the names of the witness and requirement differ, we need to
22142212
// specify the name.
2215-
if (name != targetName || ignoreImpliedName) {
2213+
if (targetName && (name != targetName || ignoreImpliedName)) {
22162214
out << "(";
22172215
out << targetName;
22182216
out << ")";
@@ -2907,8 +2905,10 @@ bool swift::diagnoseObjCUnsatisfiedOptReqConflicts(SourceFile &sf) {
29072905
// Fix the '@objc' attribute, if needed.
29082906
if (!bestConflict->canInferObjCFromRequirement(req))
29092907
fixDeclarationObjCName(diag, bestConflict,
2910-
bestConflict->getObjCRuntimeName(),
2911-
req->getObjCRuntimeName(),
2908+
bestConflict->getObjCRuntimeName()
2909+
.value_or(ObjCSelector()),
2910+
req->getObjCRuntimeName()
2911+
.value_or(ObjCSelector()),
29122912
/*ignoreImpliedName=*/true);
29132913
}
29142914

@@ -3277,12 +3277,12 @@ class ObjCImplementationChecker {
32773277
return ObjCSelector();
32783278
}
32793279

3280-
static std::optional<ObjCSelector> getObjCName(ValueDecl *VD) {
3280+
static ObjCSelector getObjCName(ValueDecl *VD) {
32813281
if (!VD->getCDeclName().empty()) {
32823282
auto ident = VD->getASTContext().getIdentifier(VD->getCDeclName());
32833283
return ObjCSelector(VD->getASTContext(), 0, { ident });
32843284
}
3285-
return VD->getObjCRuntimeName();
3285+
return VD->getObjCRuntimeName().value_or(ObjCSelector());
32863286
}
32873287

32883288
public:
@@ -3427,8 +3427,8 @@ class ObjCImplementationChecker {
34273427
for (auto req : matchedRequirements.matches) {
34283428
auto diag =
34293429
diagnose(cand, diag::objc_implementation_one_matched_requirement,
3430-
req, *getObjCName(req), shouldOfferFix,
3431-
getObjCName(req)->getString(scratch));
3430+
req, getObjCName(req), shouldOfferFix,
3431+
getObjCName(req).getString(scratch));
34323432
if (shouldOfferFix) {
34333433
fixDeclarationObjCName(diag, cand, getObjCName(cand),
34343434
getObjCName(req), /*ignoreImpliedName=*/true);
@@ -3468,14 +3468,14 @@ class ObjCImplementationChecker {
34683468
auto ext =
34693469
cast<ExtensionDecl>(reqIDC->getImplementationContext());
34703470
diagnose(ext, diag::objc_implementation_multiple_matching_candidates,
3471-
req, *getObjCName(req));
3471+
req, getObjCName(req));
34723472

34733473
for (auto cand : cands.matches) {
34743474
bool shouldOfferFix = !unmatchedCandidates[cand];
34753475
auto diag =
34763476
diagnose(cand, diag::objc_implementation_candidate_impl_here,
34773477
cand, shouldOfferFix,
3478-
getObjCName(req)->getString(scratch));
3478+
getObjCName(req).getString(scratch));
34793479

34803480
if (shouldOfferFix) {
34813481
fixDeclarationObjCName(diag, cand, getObjCName(cand),
@@ -3650,7 +3650,7 @@ class ObjCImplementationChecker {
36503650

36513651
void diagnoseOutcome(MatchOutcome outcome, ValueDecl *req, ValueDecl *cand,
36523652
ObjCSelector explicitObjCName) {
3653-
auto reqObjCName = *getObjCName(req);
3653+
auto reqObjCName = getObjCName(req);
36543654

36553655
switch (outcome) {
36563656
case MatchOutcome::NoRelationship:
@@ -3667,7 +3667,7 @@ class ObjCImplementationChecker {
36673667
case MatchOutcome::WrongImplicitObjCName:
36683668
case MatchOutcome::WrongExplicitObjCName: {
36693669
auto diag = diagnose(cand, diag::objc_implementation_wrong_objc_name,
3670-
*getObjCName(cand), cand, reqObjCName);
3670+
getObjCName(cand), cand, reqObjCName);
36713671
fixDeclarationObjCName(diag, cand, explicitObjCName, reqObjCName);
36723672
return;
36733673
}

‎lib/Sema/TypeCheckDeclPrimary.cpp

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2045,29 +2045,33 @@ static void applyAccessNote(ValueDecl *VD, const AccessNote &note,
20452045
});
20462046

20472047
if (note.ObjCName) {
2048+
auto newName = note.ObjCName.value();
2049+
2050+
// addOrRemoveAttr above guarantees there's an ObjCAttr on this decl.
20482051
auto attr = VD->getAttrs().getAttribute<ObjCAttr>();
20492052
assert(attr && "ObjCName set, but ObjCAttr not true or did not apply???");
20502053

20512054
if (!attr->hasName()) {
2052-
auto oldName = attr->getName();
2053-
attr->setName(*note.ObjCName, true);
2055+
// There was already an @objc attribute with no selector. Set it.
2056+
attr->setName(newName, true);
20542057

20552058
if (!ctx.LangOpts.shouldRemarkOnAccessNoteSuccess())
20562059
return;
20572060

20582061
VD->diagnose(diag::attr_objc_name_changed_by_access_note,
2059-
notes.Reason, VD->getDescriptiveKind(), *note.ObjCName);
2062+
notes.Reason, VD->getDescriptiveKind(), newName);
20602063

20612064
auto fixIt =
20622065
VD->diagnose(diag::fixit_attr_objc_name_changed_by_access_note);
2063-
fixDeclarationObjCName(fixIt, VD, oldName, *note.ObjCName);
2066+
fixDeclarationObjCName(fixIt, VD, ObjCSelector(), newName);
20642067
}
2065-
else if (attr->getName() != *note.ObjCName) {
2068+
else if (attr->getName() != newName) {
2069+
// There was already an @objc
20662070
auto behavior = ctx.LangOpts.getAccessNoteFailureLimit();
20672071

20682072
VD->diagnose(diag::attr_objc_name_conflicts_with_access_note,
2069-
notes.Reason, VD->getDescriptiveKind(), *attr->getName(),
2070-
*note.ObjCName)
2073+
notes.Reason, VD->getDescriptiveKind(),
2074+
attr->getName().value(), newName)
20712075
.highlight(attr->getRangeWithAt())
20722076
.limitBehavior(behavior);
20732077
}

‎lib/Sema/TypeCheckObjC.h

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -213,8 +213,7 @@ bool fixDeclarationName(InFlightDiagnostic &diag, const ValueDecl *decl,
213213
/// For properties, the selector should be a zero-parameter selector of the
214214
/// given property's name.
215215
bool fixDeclarationObjCName(InFlightDiagnostic &diag, const Decl *decl,
216-
std::optional<ObjCSelector> nameOpt,
217-
std::optional<ObjCSelector> targetNameOpt,
216+
ObjCSelector name, ObjCSelector targetName,
218217
bool ignoreImpliedName = false);
219218

220219
} // end namespace swift

‎lib/Sema/TypeCheckProtocol.cpp

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2850,8 +2850,10 @@ diagnoseMatch(ModuleDecl *module, NormalProtocolConformance *conformance,
28502850
// Also fix the Objective-C name, if needed.
28512851
if (!match.Witness->canInferObjCFromRequirement(req))
28522852
fixDeclarationObjCName(diag, match.Witness,
2853-
match.Witness->getObjCRuntimeName(),
2854-
req->getObjCRuntimeName());
2853+
match.Witness->getObjCRuntimeName()
2854+
.value_or(ObjCSelector()),
2855+
req->getObjCRuntimeName()
2856+
.value_or(ObjCSelector()));
28552857
break;
28562858
}
28572859

@@ -5065,8 +5067,8 @@ void ConformanceChecker::resolveValueWitnesses() {
50655067
if (!witness->canInferObjCFromRequirement(requirement)) {
50665068
fixDeclarationObjCName(
50675069
fixItDiag.value(), witness,
5068-
witness->getObjCRuntimeName(),
5069-
requirement->getObjCRuntimeName());
5070+
witness->getObjCRuntimeName().value_or(ObjCSelector()),
5071+
requirement->getObjCRuntimeName().value_or(ObjCSelector()));
50705072
}
50715073
} else if (isa<VarDecl>(witness)) {
50725074
std::optional<InFlightDiagnostic> fixItDiag = C.Diags.diagnose(
@@ -5085,8 +5087,8 @@ void ConformanceChecker::resolveValueWitnesses() {
50855087
if (!witness->canInferObjCFromRequirement(requirement)) {
50865088
fixDeclarationObjCName(
50875089
fixItDiag.value(), witness,
5088-
witness->getObjCRuntimeName(),
5089-
requirement->getObjCRuntimeName());
5090+
witness->getObjCRuntimeName().value_or(ObjCSelector()),
5091+
requirement->getObjCRuntimeName().value_or(ObjCSelector()));
50905092
}
50915093
} else if (isa<SubscriptDecl>(witness)) {
50925094
std::optional<InFlightDiagnostic> fixItDiag = C.Diags.diagnose(
@@ -5784,8 +5786,10 @@ static void diagnosePotentialWitness(NormalProtocolConformance *conformance,
57845786
witness->diagnose(diag::optional_req_nonobjc_near_match_add_objc);
57855787
if (!witness->canInferObjCFromRequirement(req))
57865788
fixDeclarationObjCName(diag, witness,
5787-
witness->getObjCRuntimeName(),
5788-
req->getObjCRuntimeName());
5789+
witness->getObjCRuntimeName()
5790+
.value_or(ObjCSelector()),
5791+
req->getObjCRuntimeName()
5792+
.value_or(ObjCSelector()));
57895793
} else {
57905794
diagnoseMatch(conformance->getDeclContext()->getParentModule(),
57915795
conformance, req, match);

‎test/decl/ext/objc_implementation.swift

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,6 @@
11
// RUN: %target-typecheck-verify-swift -import-objc-header %S/Inputs/objc_implementation.h -enable-experimental-feature ObjCImplementation -enable-experimental-feature CImplementation -target %target-stable-abi-triple
22
// REQUIRES: objc_interop
33

4-
// Temporarily disabled rdar://128683206
5-
// REQUIRES: rdar128683206
6-
74
protocol EmptySwiftProto {}
85

96
@objc @implementation extension ObjCClass: EmptySwiftProto, EmptyObjCProto {

0 commit comments

Comments
 (0)