Skip to content

Commit 4afe0c6

Browse files
DougGregortkremenek
DougGregor
authored and
tkremenek
committed
Don't emit @objc Fix-Its or diagnostics where @objc inference would do the right job (#2748)
* [Diagnostic verify] Treat "{{none}}" as "no unexpected Fix-Its". We were using "{{none}}" to mean "no Fix-Its" at all in the diagnostic verifier, which is useful but narrow. Tweak it's meaning to mean "no Fix-Its other than the ones we've explicitly checked for", which lets us verify the exact set of Fix-Its produced as part of a diagnostic rather than only matching a subset. I'll be landing some tests that rely on this shortly. (cherry picked from commit 70f3290) * [QoI] Don't emit @objc Fix-Its for cases where @objc will be inferred from a requirement. This is a follow-up to the change that allowed one to omit @objc (or the name in an @objc) when it can be inferred by matching a requirement. There is no point in suggesting that one add @objc if it will be inferred anyway, since it's just syntactic noise. (cherry picked from commit 9b24c83) * QoI: Improve diagnostics for failed @objc inference. When we cannot infer an @objc name due to an ambiguity, provide a specific error with Fix-Its for various courses of action (pick a specific name, make it @nonobjc). Also, be sure to suppress redundant diagnostics for Objective-C selector checking when checking protocol conformances: a match-with-renaming will emit the appropriate '@objc' when it's needed, so we don't need a follow-up diagnostic here. Finishes rdar://problem/26518216. (cherry picked from commit a318acb)
1 parent f725655 commit 4afe0c6

File tree

11 files changed

+122
-27
lines changed

11 files changed

+122
-27
lines changed

‎include/swift/AST/Decl.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2141,6 +2141,10 @@ class ValueDecl : public Decl {
21412141
/// first slot.
21422142
Optional<ObjCSelector> getObjCRuntimeName() const;
21432143

2144+
/// Determine whether the given declaration can infer @objc, or the
2145+
/// Objective-C name, if used to satisfy the given requirement.
2146+
bool canInferObjCFromRequirement(ValueDecl *requirement);
2147+
21442148
SourceLoc getNameLoc() const { return NameLoc; }
21452149
SourceLoc getLoc() const { return NameLoc; }
21462150

‎include/swift/AST/DiagnosticsSema.def

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2699,6 +2699,13 @@ ERROR(objc_override_property_name_mismatch,none,
26992699
"Objective-C property has a different name from the "
27002700
"property it overrides (%0 vs. %1)", (Identifier, Identifier))
27012701

2702+
ERROR(objc_ambiguous_inference,none,
2703+
"ambiguous inference of Objective-C name for %0 %1 (%2 vs %3)",
2704+
(DescriptiveDeclKind, DeclName, ObjCSelector, ObjCSelector))
2705+
NOTE(objc_ambiguous_inference_candidate,none,
2706+
"%0 (in protocol %1) provides Objective-C name %2",
2707+
(DeclName, DeclName, ObjCSelector))
2708+
27022709
ERROR(nonlocal_bridged_to_objc,none,
27032710
"conformance of %0 to %1 can only be written in module %2",
27042711
(Identifier, Identifier, Identifier))

‎lib/AST/ASTContext.cpp

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2478,8 +2478,9 @@ bool ASTContext::diagnoseObjCUnsatisfiedOptReqConflicts(SourceFile &sf) {
24782478
fixDeclarationName(diag, conflicts[0], req->getFullName());
24792479

24802480
// Fix the '@objc' attribute, if needed.
2481-
fixDeclarationObjCName(diag, conflicts[0], req->getObjCRuntimeName(),
2482-
/*ignoreImpliedName=*/true);
2481+
if (!conflicts[0]->canInferObjCFromRequirement(req))
2482+
fixDeclarationObjCName(diag, conflicts[0], req->getObjCRuntimeName(),
2483+
/*ignoreImpliedName=*/true);
24832484
}
24842485

24852486
// @nonobjc will silence this warning.

‎lib/AST/Decl.cpp

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1736,6 +1736,42 @@ Optional<ObjCSelector> ValueDecl::getObjCRuntimeName() const {
17361736
return None;
17371737
}
17381738

1739+
bool ValueDecl::canInferObjCFromRequirement(ValueDecl *requirement) {
1740+
// Only makes sense for a requirement of an @objc protocol.
1741+
auto proto = cast<ProtocolDecl>(requirement->getDeclContext());
1742+
if (!proto->isObjC()) return false;
1743+
1744+
// Only makes sense when this declaration is within a nominal type
1745+
// or extension thereof.
1746+
auto nominal =
1747+
getDeclContext()->getAsNominalTypeOrNominalTypeExtensionContext();
1748+
if (!nominal) return false;
1749+
1750+
// If there is already an @objc attribute with an explicit name, we
1751+
// can't infer a name (it's already there).
1752+
if (auto objcAttr = getAttrs().getAttribute<ObjCAttr>()) {
1753+
if (!objcAttr->isNameImplicit()) return false;
1754+
}
1755+
1756+
// If the nominal type doesn't conform to the protocol at all, we
1757+
// cannot infer @objc no matter what we do.
1758+
SmallVector<ProtocolConformance *, 1> conformances;
1759+
if (!nominal->lookupConformance(getModuleContext(), proto, conformances))
1760+
return false;
1761+
1762+
// If any of the conformances is attributed to the context in which
1763+
// this declaration resides, we can infer @objc or the Objective-C
1764+
// name.
1765+
auto dc = getDeclContext();
1766+
for (auto conformance : conformances) {
1767+
if (conformance->getDeclContext() == dc)
1768+
return true;
1769+
}
1770+
1771+
// Nothing to infer from.
1772+
return false;
1773+
}
1774+
17391775
SourceLoc ValueDecl::getAttributeInsertionLoc(bool forModifier) const {
17401776
if (auto var = dyn_cast<VarDecl>(this)) {
17411777
if (auto pbd = var->getParentPatternBinding()) {

‎lib/Frontend/DiagnosticVerifier.cpp

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -41,8 +41,8 @@ namespace {
4141
bool mayAppear = false;
4242

4343
// This is true if a '{{none}}' is present to mark that there should be no
44-
// fixits at all.
45-
bool noFixitsMayAppear = false;
44+
// extra fixits.
45+
bool noExtraFixitsMayAppear = false;
4646

4747
// This is the raw input buffer for the message text, the part in the
4848
// {{...}}
@@ -364,7 +364,7 @@ bool DiagnosticVerifier::verifyFile(unsigned BufferID,
364364

365365
// Special case for specifying no fixits should appear.
366366
if (FixItStr == "none") {
367-
Expected.noFixitsMayAppear = true;
367+
Expected.noExtraFixitsMayAppear = true;
368368
continue;
369369
}
370370

@@ -459,6 +459,9 @@ bool DiagnosticVerifier::verifyFile(unsigned BufferID,
459459
if (!checkForFixIt(fixit, FoundDiagnostic, InputFile))
460460
IncorrectFixit = fixit.StartLoc;
461461
}
462+
463+
bool matchedAllFixIts =
464+
expected.Fixits.size() == FoundDiagnostic.getFixIts().size();
462465

463466
// If we have any expected fixits that didn't get matched, then they are
464467
// wrong. Replace the failed fixit with what actually happened.
@@ -476,8 +479,8 @@ bool DiagnosticVerifier::verifyFile(unsigned BufferID,
476479
addError(IncorrectFixit,
477480
"expected fix-it not seen; actual fix-its: " + actual, fix);
478481
}
479-
} else if (expected.noFixitsMayAppear &&
480-
!FoundDiagnostic.getFixIts().empty() &&
482+
} else if (expected.noExtraFixitsMayAppear &&
483+
!matchedAllFixIts &&
481484
!expected.mayAppear) {
482485
// If there was no fixit specification, but some were produced, add a
483486
// fixit to add them in.

‎lib/Sema/TypeCheckDecl.cpp

Lines changed: 27 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2296,18 +2296,42 @@ static void inferObjCName(TypeChecker &tc, ValueDecl *decl) {
22962296
// When no override determined the Objective-C name, look for
22972297
// requirements for which this declaration is a witness.
22982298
Optional<ObjCSelector> requirementObjCName;
2299+
ValueDecl *firstReq = nullptr;
22992300
for (auto req : tc.findWitnessedObjCRequirements(decl,
23002301
/*onlyFirst=*/false)) {
23012302
// If this is the first requirement, take its name.
23022303
if (!requirementObjCName) {
23032304
requirementObjCName = req->getObjCRuntimeName();
2305+
firstReq = req;
23042306
continue;
23052307
}
23062308

23072309
// If this requirement has a different name from one we've seen,
2308-
// bail out and let protocol-conformance diagnostics handle this.
2309-
if (*requirementObjCName != *req->getObjCRuntimeName())
2310-
return;
2310+
// note the ambiguity.
2311+
if (*requirementObjCName != *req->getObjCRuntimeName()) {
2312+
tc.diagnose(decl, diag::objc_ambiguous_inference,
2313+
decl->getDescriptiveKind(), decl->getFullName(),
2314+
*requirementObjCName, *req->getObjCRuntimeName());
2315+
2316+
// Note the candidates and what Objective-C names they provide.
2317+
auto diagnoseCandidate = [&](ValueDecl *req) {
2318+
auto proto = cast<ProtocolDecl>(req->getDeclContext());
2319+
auto diag = tc.diagnose(decl,
2320+
diag::objc_ambiguous_inference_candidate,
2321+
req->getFullName(),
2322+
proto->getFullName(),
2323+
*req->getObjCRuntimeName());
2324+
fixDeclarationObjCName(diag, decl, req->getObjCRuntimeName());
2325+
};
2326+
diagnoseCandidate(firstReq);
2327+
diagnoseCandidate(req);
2328+
2329+
// Suggest '@nonobjc' to suppress this error, and not try to
2330+
// infer @objc for anything.
2331+
tc.diagnose(decl, diag::optional_req_near_match_nonobjc, true)
2332+
.fixItInsert(decl->getAttributeInsertionLoc(false), "@nonobjc ");
2333+
break;
2334+
}
23112335
}
23122336

23132337
// If we have a name, install it via an @objc attribute.

‎lib/Sema/TypeCheckProtocol.cpp

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1718,7 +1718,7 @@ diagnoseMatch(TypeChecker &tc, Module *module,
17181718
fixDeclarationName(diag, match.Witness, req->getFullName());
17191719

17201720
// Also fix the Objective-C name, if needed.
1721-
if (req->isObjC())
1721+
if (!match.Witness->canInferObjCFromRequirement(req))
17221722
fixDeclarationObjCName(diag, match.Witness, req->getObjCRuntimeName());
17231723
break;
17241724
}
@@ -3653,6 +3653,7 @@ void ConformanceChecker::checkConformance() {
36533653

36543654
// Objective-C checking for @objc requirements.
36553655
if (requirement->isObjC() &&
3656+
requirement->getFullName() == witness->getFullName() &&
36563657
!requirement->getAttrs().isUnavailable(TC.Context)) {
36573658
// The witness must also be @objc.
36583659
if (!witness->isObjC()) {
@@ -3665,9 +3666,11 @@ void ConformanceChecker::checkConformance() {
36653666
: diag::witness_non_objc,
36663667
diagInfo.first, diagInfo.second,
36673668
Proto->getFullName());
3668-
fixDeclarationObjCName(
3669-
diag, witness,
3670-
cast<AbstractFunctionDecl>(requirement)->getObjCSelector());
3669+
if (!witness->canInferObjCFromRequirement(requirement)) {
3670+
fixDeclarationObjCName(
3671+
diag, witness,
3672+
cast<AbstractFunctionDecl>(requirement)->getObjCSelector());
3673+
}
36713674
} else if (isa<VarDecl>(witness)) {
36723675
auto diag = TC.diagnose(witness,
36733676
isOptional
@@ -3676,10 +3679,13 @@ void ConformanceChecker::checkConformance() {
36763679
/*isSubscript=*/false,
36773680
witness->getFullName(),
36783681
Proto->getFullName());
3679-
fixDeclarationObjCName(
3680-
diag, witness,
3681-
ObjCSelector(requirement->getASTContext(), 0,
3682-
cast<VarDecl>(requirement)->getObjCPropertyName()));
3682+
if (!witness->canInferObjCFromRequirement(requirement)) {
3683+
fixDeclarationObjCName(
3684+
diag, witness,
3685+
ObjCSelector(requirement->getASTContext(), 0,
3686+
cast<VarDecl>(requirement)
3687+
->getObjCPropertyName()));
3688+
}
36833689
} else if (isa<SubscriptDecl>(witness)) {
36843690
TC.diagnose(witness,
36853691
isOptional
@@ -4412,7 +4418,8 @@ static void diagnosePotentialWitness(TypeChecker &tc,
44124418
// Special case: note to add @objc.
44134419
auto diag = tc.diagnose(witness,
44144420
diag::optional_req_nonobjc_near_match_add_objc);
4415-
fixDeclarationObjCName(diag, witness, req->getObjCRuntimeName());
4421+
if (!witness->canInferObjCFromRequirement(req))
4422+
fixDeclarationObjCName(diag, witness, req->getObjCRuntimeName());
44164423
} else {
44174424
diagnoseMatch(tc, conformance->getDeclContext()->getParentModule(),
44184425
conformance, req, match);

‎test/ClangModules/objc_parse.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -265,7 +265,7 @@ func classAnyObject(_ obj: NSObject) {
265265
class Wobbler : NSWobbling { // expected-note{{candidate has non-matching type '()'}}
266266
@objc func wobble() { }
267267

268-
func returnMyself() -> Self { return self } // expected-error{{non-'@objc' method 'returnMyself()' does not satisfy requirement of '@objc' protocol 'NSWobbling'}}{{3-3=@objc }}
268+
func returnMyself() -> Self { return self } // expected-error{{non-'@objc' method 'returnMyself()' does not satisfy requirement of '@objc' protocol 'NSWobbling'}}{{none}}
269269
// expected-error@-1{{method cannot be an implementation of an @objc requirement because its result type cannot be represented in Objective-C}}
270270
}
271271

‎test/decl/protocol/conforms/near_miss_objc.swift

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
class C1a : P1 {
1111
@objc func doSomething(a: Int, c: Double) { }
1212
// expected-warning@-1{{instance method 'doSomething(a:c:)' nearly matches optional requirement 'doSomething(a:b:)' of protocol 'P1'}}
13-
// expected-note@-2{{rename to 'doSomething(a:b:)' to satisfy this requirement}}{{34-34=b }}{{8-8=(doSomethingWithA:b:)}}
13+
// expected-note@-2{{rename to 'doSomething(a:b:)' to satisfy this requirement}}{{34-34=b }}{{none}}
1414
// expected-note@-3{{move 'doSomething(a:c:)' to an extension to silence this warning}}
1515
// expected-note@-4{{make 'doSomething(a:c:)' private to silence this warning}}{{9-9=private }}
1616
}
@@ -28,7 +28,7 @@ class C1c {
2828
extension C1c : P1 {
2929
func doSomething(a: Int, c: Double) { }
3030
// expected-warning@-1{{instance method 'doSomething(a:c:)' nearly matches optional requirement 'doSomething(a:b:)' of protocol 'P1'}}
31-
// expected-note@-2{{rename to 'doSomething(a:b:)' to satisfy this requirement}}{{28-28=b }}{{3-3=@objc(doSomethingWithA:b:) }}
31+
// expected-note@-2{{rename to 'doSomething(a:b:)' to satisfy this requirement}}{{28-28=b }}{{none}}
3232
// expected-note@-3{{move 'doSomething(a:c:)' to another extension to silence this warning}}
3333
// expected-note@-4{{make 'doSomething(a:c:)' private to silence this warning}}{{3-3=private }}
3434
}
@@ -87,7 +87,7 @@ class C4a : P4 {
8787
class C5a : P5 {
8888
func method(_: Int, for someClass: SomeClass, dividing double: Double) { }
8989
// expected-warning@-1{{instance method 'method(_:for:dividing:)' nearly matches optional requirement 'methodWithInt(_:forSomeClass:dividingDouble:)' of protocol 'P5'}}
90-
// expected-note@-2{{rename to 'methodWithInt(_:forSomeClass:dividingDouble:)' to satisfy this requirement}}{{3-3=@objc(methodWithInt:forSomeClass:dividingDouble:) }}{{8-14=methodWithInt}}{{23-26=forSomeClass}}{{49-57=dividingDouble}}
90+
// expected-note@-2{{rename to 'methodWithInt(_:forSomeClass:dividingDouble:)' to satisfy this requirement}}{{8-14=methodWithInt}}{{23-26=forSomeClass}}{{49-57=dividingDouble}}{{none}}
9191
// expected-note@-3{{move 'method(_:for:dividing:)' to an extension to silence this warning}}
9292
// expected-note@-4{{make 'method(_:for:dividing:)' private to silence this warning}}{{3-3=private }}
9393
}
@@ -100,7 +100,7 @@ class C5a : P5 {
100100
class C6a : P6 {
101101
func methodWithInt(_: Int, forSomeClass: SomeClass, dividingDouble: Double) { }
102102
// expected-warning@-1{{instance method 'methodWithInt(_:forSomeClass:dividingDouble:)' nearly matches optional requirement 'method(_:for:dividing:)' of protocol 'P6'}}
103-
// expected-note@-2{{rename to 'method(_:for:dividing:)' to satisfy this requirement}}{{3-3=@objc(method:for:dividing:) }}{{8-21=method}}{{30-30=for }}{{55-55=dividing }}
103+
// expected-note@-2{{rename to 'method(_:for:dividing:)' to satisfy this requirement}}{{8-21=method}}{{30-30=for }}{{55-55=dividing }}{{none}}
104104
// expected-note@-3{{move 'methodWithInt(_:forSomeClass:dividingDouble:)' to an extension to silence this warning}}
105105
// expected-note@-4{{make 'methodWithInt(_:forSomeClass:dividingDouble:)' private to silence this warning}}{{3-3=private }}
106106
}

‎test/decl/protocol/objc.swift

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,19 @@ class C4b : P4 {
130130
// Don't infer when there is an ambiguity.
131131
class C4_5a : P4, P5 {
132132
func method(x: Int, y: Int) { }
133-
// expected-error@-1{{Objective-C method 'methodWithX:y:' provided by method 'method(x:y:)' does not match the requirement's selector ('foo:bar:')}}
134-
// expected-error@-2{{Objective-C method 'methodWithX:y:' provided by method 'method(x:y:)' does not match the requirement's selector ('wibble:wobble:')}}
133+
// expected-error@-1{{ambiguous inference of Objective-C name for instance method 'method(x:y:)' ('foo:bar:' vs 'wibble:wobble:')}}
134+
// expected-note@-2{{'method(x:y:)' (in protocol 'P4') provides Objective-C name 'foo:bar:'}}{{3-3=@objc(foo:bar:) }}
135+
// expected-note@-3{{'method(x:y:)' (in protocol 'P5') provides Objective-C name 'wibble:wobble:'}}{{3-3=@objc(wibble:wobble:) }}
136+
// expected-note@-4{{add '@nonobjc' to silence this error}}{{3-3=@nonobjc }}
137+
// expected-error@-5{{Objective-C method 'foo:bar:' provided by method 'method(x:y:)' does not match the requirement's selector ('wibble:wobble:')}}
138+
}
139+
140+
// Don't complain about a selector mismatch in cases where the
141+
// selector will be inferred.
142+
@objc protocol P6 {
143+
func doSomething(_ sender: AnyObject?) // expected-note{{requirement 'doSomething' declared here}}
144+
}
145+
146+
class C6a : P6 {
147+
func doSomething(sender: AnyObject?) { } // expected-error{{method 'doSomething(sender:)' has different argument names from those required by protocol 'P6' ('doSomething')}}{{20-20=_ }}{{none}}
135148
}

0 commit comments

Comments
 (0)