Skip to content

Commit c504eb1

Browse files
beccadaxbeccadax
committed
Warn if magic identifiers don’t match
When wrapping a function which is supposed to capture the caller’s location, there’s always a risk that the wrapper won’t capture the information the wrapped function wants; for instance, you might pass `(…, line, column)` where the callee expected `(…, column, line)`. This commit emits a warning when a call passes an explicit argument to something that has a default argument, and that explicit argument is itself a parameter with a default argument, and both parameters use magic identifiers, but they use *different* magic identifiers. This is partially in support of concise #file, but applies to all magic identifiers. Fixes rdar://problem/58588633.
1 parent b43b1ec commit c504eb1

File tree

7 files changed

+390
-23
lines changed

7 files changed

+390
-23
lines changed

‎include/swift/AST/DiagnosticsSema.def

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4678,6 +4678,17 @@ WARNING(observe_keypath_property_not_objc_dynamic, none,
46784678
"passing reference to non-'@objc dynamic' property %0 to KVO method %1 "
46794679
"may lead to unexpected behavior or runtime trap", (DeclName, DeclName))
46804680

4681+
WARNING(default_magic_identifier_mismatch, none,
4682+
"parameter %0 with default argument '%1' passed to parameter %2, whose "
4683+
"default argument is '%3'",
4684+
(Identifier, StringRef, Identifier, StringRef))
4685+
NOTE(change_caller_default_to_match_callee, none,
4686+
"did you mean for parameter %0 to default to '%1'?",
4687+
(Identifier, StringRef))
4688+
NOTE(silence_default_magic_identifier_mismatch, none,
4689+
"add parentheses to silence this warning", ())
4690+
4691+
46814692
//------------------------------------------------------------------------------
46824693
// MARK: Debug diagnostics
46834694
//------------------------------------------------------------------------------

‎include/swift/AST/Expr.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -503,7 +503,7 @@ class alignas(8) Expr {
503503

504504
/// Retrieves the declaration that is being referenced by this
505505
/// expression, if any.
506-
ConcreteDeclRef getReferencedDecl() const;
506+
ConcreteDeclRef getReferencedDecl(bool stopAtParenExpr = false) const;
507507

508508
/// Determine whether this expression is 'super', possibly converted to
509509
/// a base class.

‎lib/AST/Expr.cpp

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -228,7 +228,7 @@ DeclRefExpr *Expr::getMemberOperatorRef() {
228228
return operatorRef;
229229
}
230230

231-
ConcreteDeclRef Expr::getReferencedDecl() const {
231+
ConcreteDeclRef Expr::getReferencedDecl(bool stopAtParenExpr) const {
232232
switch (getKind()) {
233233
// No declaration reference.
234234
#define NO_REFERENCE(Id) case ExprKind::Id: return ConcreteDeclRef()
@@ -237,7 +237,8 @@ ConcreteDeclRef Expr::getReferencedDecl() const {
237237
return cast<Id##Expr>(this)->Getter()
238238
#define PASS_THROUGH_REFERENCE(Id, GetSubExpr) \
239239
case ExprKind::Id: \
240-
return cast<Id##Expr>(this)->GetSubExpr()->getReferencedDecl()
240+
return cast<Id##Expr>(this) \
241+
->GetSubExpr()->getReferencedDecl(stopAtParenExpr)
241242

242243
NO_REFERENCE(Error);
243244
NO_REFERENCE(NilLiteral);
@@ -275,7 +276,12 @@ ConcreteDeclRef Expr::getReferencedDecl() const {
275276
NO_REFERENCE(UnresolvedMember);
276277
NO_REFERENCE(UnresolvedDot);
277278
NO_REFERENCE(Sequence);
278-
PASS_THROUGH_REFERENCE(Paren, getSubExpr);
279+
280+
case ExprKind::Paren:
281+
if (stopAtParenExpr) return ConcreteDeclRef();
282+
return cast<ParenExpr>(this)
283+
->getSubExpr()->getReferencedDecl(stopAtParenExpr);
284+
279285
PASS_THROUGH_REFERENCE(DotSelf, getSubExpr);
280286
PASS_THROUGH_REFERENCE(Try, getSubExpr);
281287
PASS_THROUGH_REFERENCE(ForceTry, getSubExpr);

‎lib/Sema/MiscDiagnostics.cpp

Lines changed: 106 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -194,28 +194,30 @@ static void diagSyntacticUseRestrictions(const Expr *E, const DeclContext *DC,
194194
callee = dynamicMRE->getMember();
195195
}
196196

197-
visitArguments(Call, [&](unsigned argIndex, Expr *arg) {
198-
// InOutExprs can be wrapped in some implicit casts.
199-
Expr *unwrapped = arg;
200-
if (auto *IIO = dyn_cast<InjectIntoOptionalExpr>(arg))
201-
unwrapped = IIO->getSubExpr();
202-
203-
if (isa<InOutToPointerExpr>(unwrapped) ||
204-
isa<ArrayToPointerExpr>(unwrapped) ||
205-
isa<ErasureExpr>(unwrapped)) {
206-
auto operand =
207-
cast<ImplicitConversionExpr>(unwrapped)->getSubExpr();
208-
if (auto *IOE = dyn_cast<InOutExpr>(operand))
209-
operand = IOE->getSubExpr();
210-
211-
// Also do some additional work based on how the function uses
212-
// the argument.
213-
if (callee) {
197+
if (callee) {
198+
visitArguments(Call, [&](unsigned argIndex, Expr *arg) {
199+
checkMagicIdentifierMismatch(callee, uncurryLevel, argIndex, arg);
200+
201+
// InOutExprs can be wrapped in some implicit casts.
202+
Expr *unwrapped = arg;
203+
if (auto *IIO = dyn_cast<InjectIntoOptionalExpr>(arg))
204+
unwrapped = IIO->getSubExpr();
205+
206+
if (isa<InOutToPointerExpr>(unwrapped) ||
207+
isa<ArrayToPointerExpr>(unwrapped) ||
208+
isa<ErasureExpr>(unwrapped)) {
209+
auto operand =
210+
cast<ImplicitConversionExpr>(unwrapped)->getSubExpr();
211+
if (auto *IOE = dyn_cast<InOutExpr>(operand))
212+
operand = IOE->getSubExpr();
213+
214+
// Also do some additional work based on how the function uses
215+
// the argument.
214216
checkConvertedPointerArgument(callee, uncurryLevel, argIndex,
215217
unwrapped, operand);
216218
}
217-
}
218-
});
219+
});
220+
}
219221
}
220222

221223
// If we have an assignment expression, scout ahead for acceptable _'s.
@@ -421,6 +423,91 @@ static void diagSyntacticUseRestrictions(const Expr *E, const DeclContext *DC,
421423
// Otherwise, we can't support this.
422424
}
423425

426+
void checkMagicIdentifierMismatch(ConcreteDeclRef callee,
427+
unsigned uncurryLevel,
428+
unsigned argIndex,
429+
Expr *arg) {
430+
// We only care about args in the arg list.
431+
if (uncurryLevel != (callee.getDecl()->hasCurriedSelf() ? 1 : 0))
432+
return;
433+
434+
// Get underlying params for both callee and caller, if declared.
435+
auto *calleeParam = getParameterAt(callee.getDecl(), argIndex);
436+
auto *callerParam = dyn_cast_or_null<ParamDecl>(
437+
arg->getReferencedDecl(/*stopAtParenExpr=*/true).getDecl()
438+
);
439+
440+
// (Otherwise, we don't need to do anything.)
441+
if (!calleeParam || !callerParam)
442+
return;
443+
444+
auto calleeDefaultArg = getMagicIdentifierDefaultArgKind(calleeParam);
445+
auto callerDefaultArg = getMagicIdentifierDefaultArgKind(callerParam);
446+
447+
// If one of the parameters doesn't have a default arg, or they both have
448+
// the same one, everything's fine.
449+
if (!calleeDefaultArg || !callerDefaultArg ||
450+
*calleeDefaultArg == *callerDefaultArg)
451+
return;
452+
453+
StringRef calleeDefaultArgString =
454+
MagicIdentifierLiteralExpr::getKindString(*calleeDefaultArg);
455+
StringRef callerDefaultArgString =
456+
MagicIdentifierLiteralExpr::getKindString(*callerDefaultArg);
457+
458+
// Emit main warning
459+
Ctx.Diags.diagnose(arg->getLoc(), diag::default_magic_identifier_mismatch,
460+
callerParam->getName(), callerDefaultArgString,
461+
calleeParam->getName(), calleeDefaultArgString);
462+
463+
// Add "change caller default arg" fixit
464+
SourceLoc callerDefaultArgLoc =
465+
callerParam->getStructuralDefaultExpr()->getLoc();
466+
Ctx.Diags.diagnose(callerDefaultArgLoc,
467+
diag::change_caller_default_to_match_callee,
468+
callerParam->getName(), calleeDefaultArgString)
469+
.fixItReplace(callerDefaultArgLoc, calleeDefaultArgString);
470+
471+
// Add "silence with parens" fixit
472+
Ctx.Diags.diagnose(arg->getLoc(),
473+
diag::silence_default_magic_identifier_mismatch)
474+
.fixItInsert(arg->getStartLoc(), "(")
475+
.fixItInsertAfter(arg->getEndLoc(), ")");
476+
477+
// Point to callee parameter
478+
Ctx.Diags.diagnose(calleeParam, diag::decl_declared_here,
479+
calleeParam->getFullName());
480+
}
481+
482+
Optional<MagicIdentifierLiteralExpr::Kind>
483+
getMagicIdentifierDefaultArgKind(const ParamDecl *param) {
484+
switch (param->getDefaultArgumentKind()) {
485+
case DefaultArgumentKind::Column:
486+
return MagicIdentifierLiteralExpr::Kind::Column;
487+
case DefaultArgumentKind::DSOHandle:
488+
return MagicIdentifierLiteralExpr::Kind::DSOHandle;
489+
case DefaultArgumentKind::File:
490+
return MagicIdentifierLiteralExpr::Kind::File;
491+
case DefaultArgumentKind::FilePath:
492+
return MagicIdentifierLiteralExpr::Kind::FilePath;
493+
case DefaultArgumentKind::Function:
494+
return MagicIdentifierLiteralExpr::Kind::Function;
495+
case DefaultArgumentKind::Line:
496+
return MagicIdentifierLiteralExpr::Kind::Line;
497+
498+
case DefaultArgumentKind::None:
499+
case DefaultArgumentKind::Normal:
500+
case DefaultArgumentKind::Inherited:
501+
case DefaultArgumentKind::NilLiteral:
502+
case DefaultArgumentKind::EmptyArray:
503+
case DefaultArgumentKind::EmptyDictionary:
504+
case DefaultArgumentKind::StoredProperty:
505+
return None;
506+
}
507+
508+
llvm_unreachable("Unhandled DefaultArgumentKind in "
509+
"getMagicIdentifierDefaultArgKind");
510+
}
424511

425512
void checkUseOfModule(DeclRefExpr *E) {
426513
// Allow module values as a part of:

‎test/Sema/diag_mismatched_magic_literals.swift

Lines changed: 186 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,186 @@
1+
// RUN: %target-typecheck-verify-swift
2+
3+
func callee(file: String = #file) {} // expected-note {{'file' declared here}}
4+
func callee(optFile: String? = #file) {} // expected-note {{'optFile' declared here}}
5+
func callee(arbitrary: String) {}
6+
7+
class SomeClass {
8+
static func callee(file: String = #file) {} // expected-note 2{{'file' declared here}}
9+
static func callee(optFile: String? = #file) {} // expected-note {{'optFile' declared here}}
10+
static func callee(arbitrary: String) {}
11+
12+
func callee(file: String = #file) {} // expected-note 2{{'file' declared here}}
13+
func callee(optFile: String? = #file) {} // expected-note {{'optFile' declared here}}
14+
func callee(arbitrary: String) {}
15+
}
16+
17+
//
18+
// Basic functionality
19+
//
20+
21+
// We should warn when we we pass a `#function`-defaulted argument to a
22+
// `#file`-defaulted argument.
23+
func bad(function: String = #function) {
24+
// expected-note@-1 3{{did you mean for parameter 'function' to default to '#file'?}} {{29-38=#file}}
25+
callee(file: function)
26+
// expected-warning@-1 {{parameter 'function' with default argument '#function' passed to parameter 'file', whose default argument is '#file'}}
27+
// expected-note@-2 {{add parentheses to silence this warning}} {{16-16=(}} {{24-24=)}}
28+
29+
SomeClass.callee(file: function)
30+
// expected-warning@-1 {{parameter 'function' with default argument '#function' passed to parameter 'file', whose default argument is '#file'}}
31+
// expected-note@-2 {{add parentheses to silence this warning}} {{26-26=(}} {{34-34=)}}
32+
33+
SomeClass().callee(file: function)
34+
// expected-warning@-1 {{parameter 'function' with default argument '#function' passed to parameter 'file', whose default argument is '#file'}}
35+
// expected-note@-2 {{add parentheses to silence this warning}} {{28-28=(}} {{36-36=)}}
36+
}
37+
38+
// We should not warn when we pass a `#file`-defaulted argument to a
39+
// `#file`-defaulted argument.
40+
func good(file: String = #file) {
41+
callee(file: file)
42+
43+
SomeClass.callee(file: file)
44+
45+
SomeClass().callee(file: file)
46+
}
47+
48+
// We should not warn when we surround the `#function`-defaulted argument
49+
// with parentheses, which explicitly silences the warning.
50+
func disabled(function: String = #function) {
51+
callee(file: (function))
52+
53+
SomeClass.callee(file: (function))
54+
55+
SomeClass().callee(file: (function))
56+
}
57+
58+
//
59+
// With implicit instance `self`
60+
//
61+
62+
extension SomeClass {
63+
// We should warn when we we pass a `#function`-defaulted argument to a
64+
// `#file`-defaulted argument.
65+
func bad(function: String = #function) {
66+
// expected-note@-1 1{{did you mean for parameter 'function' to default to '#file'?}} {{31-40=#file}}
67+
callee(file: function)
68+
// expected-warning@-1 {{parameter 'function' with default argument '#function' passed to parameter 'file', whose default argument is '#file'}}
69+
// expected-note@-2 {{add parentheses to silence this warning}} {{18-18=(}} {{26-26=)}}
70+
}
71+
72+
// We should not warn when we pass a `#file`-defaulted argument to a
73+
// `#file`-defaulted argument.
74+
func good(file: String = #file) {
75+
callee(file: file)
76+
}
77+
78+
// We should not warn when we surround the `#function`-defaulted argument
79+
// with parentheses, which explicitly silences the warning.
80+
func disabled(function: String = #function) {
81+
callee(file: (function))
82+
}
83+
}
84+
85+
//
86+
// With implicit type `self`
87+
//
88+
89+
extension SomeClass {
90+
// We should warn when we we pass a `#function`-defaulted argument to a
91+
// `#file`-defaulted argument.
92+
static func bad(function: String = #function) {
93+
// expected-note@-1 1{{did you mean for parameter 'function' to default to '#file'?}} {{38-47=#file}}
94+
callee(file: function)
95+
// expected-warning@-1 {{parameter 'function' with default argument '#function' passed to parameter 'file', whose default argument is '#file'}}
96+
// expected-note@-2 {{add parentheses to silence this warning}} {{18-18=(}} {{26-26=)}}
97+
}
98+
99+
// We should not warn when we pass a `#file`-defaulted argument to a
100+
// `#file`-defaulted argument.
101+
static func good(file: String = #file) {
102+
callee(file: file)
103+
}
104+
105+
// We should not warn when we surround the `#function`-defaulted argument
106+
// with parentheses, which explicitly silences the warning.
107+
static func disabled(function: String = #function) {
108+
callee(file: (function))
109+
}
110+
}
111+
112+
//
113+
// Looking through implicit conversions
114+
//
115+
// Same as above, but these lift the argument from `String` to `String?`, so
116+
// the compiler has to look through the implicit conversion.
117+
//
118+
119+
// We should warn when we we pass a `#function`-defaulted argument to a
120+
// `#file`-defaulted argument.
121+
func optionalBad(function: String = #function) {
122+
// expected-note@-1 3{{did you mean for parameter 'function' to default to '#file'?}} {{37-46=#file}}
123+
callee(optFile: function)
124+
// expected-warning@-1 {{parameter 'function' with default argument '#function' passed to parameter 'optFile', whose default argument is '#file'}}
125+
// expected-note@-2 {{add parentheses to silence this warning}} {{19-19=(}} {{27-27=)}}
126+
127+
SomeClass.callee(optFile: function)
128+
// expected-warning@-1 {{parameter 'function' with default argument '#function' passed to parameter 'optFile', whose default argument is '#file'}}
129+
// expected-note@-2 {{add parentheses to silence this warning}} {{29-29=(}} {{37-37=)}}
130+
131+
SomeClass().callee(optFile: function)
132+
// expected-warning@-1 {{parameter 'function' with default argument '#function' passed to parameter 'optFile', whose default argument is '#file'}}
133+
// expected-note@-2 {{add parentheses to silence this warning}} {{31-31=(}} {{39-39=)}}
134+
}
135+
136+
// We should not warn when we pass a `#file`-defaulted argument to a
137+
// `#file`-defaulted argument.
138+
func optionalGood(file: String = #file) {
139+
callee(optFile: file)
140+
141+
SomeClass.callee(optFile: file)
142+
143+
SomeClass().callee(optFile: file)
144+
}
145+
146+
// We should not warn when we surround the `#function`-defaulted argument
147+
// with parentheses, which explicitly silences the warning.
148+
func optionalDisabled(function: String = #function) {
149+
callee(optFile: (function))
150+
151+
SomeClass.callee(optFile: (function))
152+
153+
SomeClass().callee(optFile: (function))
154+
}
155+
156+
//
157+
// More negative cases
158+
//
159+
160+
// We should not warn if the caller's parameter has no default.
161+
func explicit(arbitrary: String) {
162+
callee(file: arbitrary)
163+
164+
SomeClass.callee(file: arbitrary)
165+
166+
SomeClass().callee(file: arbitrary)
167+
}
168+
169+
// We should not warn if the caller's parameter has a non-magic-identifier
170+
// default.
171+
func empty(arbitrary: String = "") {
172+
callee(file: arbitrary)
173+
174+
SomeClass.callee(file: arbitrary)
175+
176+
SomeClass().callee(file: arbitrary)
177+
}
178+
179+
// We should not warn if the callee's parameter has no default.
180+
func ineligible(function: String = #function) {
181+
callee(arbitrary: function)
182+
183+
SomeClass.callee(arbitrary: function)
184+
185+
SomeClass().callee(arbitrary: function)
186+
}

‎test/multifile/default-arguments/one-module/Inputs/mismatched-magic-literals-other.swift

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
func callee(file: String = #file) {} // expected-note {{'file' declared here}}
2+
func callee(optFile: String? = #file) {} // expected-note {{'optFile' declared here}}
3+
func callee(arbitrary: String) {}

0 commit comments

Comments
 (0)