Merge pull request #11465 from KingOfBrian/bugfix/SR-964
Warn if the getter is used, but the setter argument is not used
diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def
index 64e96e0..c8af8d0 100644
--- a/include/swift/AST/DiagnosticsSema.def
+++ b/include/swift/AST/DiagnosticsSema.def
@@ -3721,6 +3721,11 @@
"value %0 was defined but never used; consider replacing "
"with boolean test",
(Identifier))
+WARNING(unused_setter_parameter, none,
+ "setter argument %0 was never used, but the property was accessed",
+ (Identifier))
+NOTE(fixit_for_unused_setter_parameter, none,
+ "did you mean to use %0 instead of accessing the property's current value?", (Identifier))
WARNING(pbd_never_used, none,
"initialization of %select{variable|immutable value}1 %0 was never used"
diff --git a/lib/Sema/MiscDiagnostics.cpp b/lib/Sema/MiscDiagnostics.cpp
index 54fd144..d47a255 100644
--- a/lib/Sema/MiscDiagnostics.cpp
+++ b/lib/Sema/MiscDiagnostics.cpp
@@ -2093,7 +2093,13 @@
/// This is a mapping from an OpaqueValue to the expression that initialized
/// it.
llvm::SmallDenseMap<OpaqueValueExpr*, Expr*> OpaqueValueMap;
-
+
+ /// The getter associated with a setter function declaration.
+ const VarDecl *AssociatedGetter = nullptr;
+
+ /// The first reference to the associated getter.
+ const DeclRefExpr *AssociatedGetterDeclRef = nullptr;
+
/// This is a mapping from VarDecls to the if/while/guard statement that they
/// occur in, when they are in a pattern in a StmtCondition.
llvm::SmallDenseMap<VarDecl*, LabeledConditionalStmt*> StmtConditionForVD;
@@ -2105,12 +2111,18 @@
public:
VarDeclUsageChecker(TypeChecker &TC, AbstractFunctionDecl *AFD) : Diags(TC.Diags) {
- // Track the parameters of the function.
- for (auto PL : AFD->getParameterLists())
- for (auto param : *PL)
- if (shouldTrackVarDecl(param))
- VarDecls[param] = 0;
-
+ // If this AFD is a setter, track the parameter and the getter for
+ // the containing property so if newValue isn't used but the getter is used
+ // an error can be reported.
+ if (auto FD = dyn_cast<FuncDecl>(AFD)) {
+ if (FD->getAccessorKind() == AccessorKind::IsSetter) {
+ if (auto getter = dyn_cast<VarDecl>(FD->getAccessorStorageDecl())) {
+ auto arguments = FD->getParameterLists().back();
+ VarDecls[arguments->get(0)] = 0;
+ AssociatedGetter = getter;
+ }
+ }
+ }
}
VarDeclUsageChecker(DiagnosticEngine &Diags) : Diags(Diags) {}
@@ -2119,7 +2131,7 @@
// Track a specific VarDecl
VarDecls[VD] = 0;
}
-
+
void suppressDiagnostics() {
sawError = true; // set this flag so that no diagnostics will be emitted on delete.
}
@@ -2257,7 +2269,6 @@
}
}
-
// Note that we ignore the initialization behavior of PatternBindingDecls,
// but we do want to walk into them, because we want to see any uses or
// other things going on in the initializer expressions.
@@ -2321,12 +2332,27 @@
if (var->isInOut())
continue;
- // Consider parameters to always have been read. It is common to name a
- // parameter and not use it (e.g. because you are an override or want the
- // named keyword, etc). Warning to rewrite it to _ is more annoying than
- // it is useful.
- if (isa<ParamDecl>(var))
- access |= RK_Read;
+ // If the setter parameter is not used, but the property is read, report
+ // a warning. Otherwise, parameters should not generate usage warnings. It
+ // is common to name a parameter and not use it (e.g. because you are an
+ // override or want the named keyword, etc). Warning to rewrite it to _ is
+ // more annoying than it is useful.
+ if (auto param = dyn_cast<ParamDecl>(var)) {
+ auto FD = dyn_cast<FuncDecl>(param->getDeclContext());
+ if (FD && FD->getAccessorKind() == AccessorKind::IsSetter) {
+ auto getter = dyn_cast<VarDecl>(FD->getAccessorStorageDecl());
+ if ((access & RK_Read) == 0 && AssociatedGetter == getter) {
+ if (auto DRE = AssociatedGetterDeclRef) {
+ Diags.diagnose(DRE->getLoc(), diag::unused_setter_parameter,
+ var->getName());
+ Diags.diagnose(DRE->getLoc(), diag::fixit_for_unused_setter_parameter,
+ var->getName())
+ .fixItReplace(DRE->getSourceRange(), var->getName().str());
+ }
+ }
+ }
+ continue;
+ }
// Diagnose variables that were never used (other than their
// initialization).
@@ -2608,9 +2634,15 @@
// If this is a DeclRefExpr found in a random place, it is a load of the
// vardecl.
- if (auto *DRE = dyn_cast<DeclRefExpr>(E))
+ if (auto *DRE = dyn_cast<DeclRefExpr>(E)) {
addMark(DRE->getDecl(), RK_Read);
+ // If the Decl is a read of a getter, track the first DRE for diagnostics
+ if (auto VD = dyn_cast<VarDecl>(DRE->getDecl())) {
+ if (AssociatedGetter == VD && AssociatedGetterDeclRef == nullptr)
+ AssociatedGetterDeclRef = DRE;
+ }
+ }
// If this is an AssignExpr, see if we're mutating something that we know
// about.
if (auto *assign = dyn_cast<AssignExpr>(E)) {
diff --git a/test/NameBinding/scope_map_lookup.swift b/test/NameBinding/scope_map_lookup.swift
index 102ff1f..902aebd 100644
--- a/test/NameBinding/scope_map_lookup.swift
+++ b/test/NameBinding/scope_map_lookup.swift
@@ -93,6 +93,7 @@
return localProperty // expected-warning{{attempting to access 'localProperty' within its own getter}}
}
set {
+ _ = newValue
print(localProperty)
}
}
diff --git a/test/decl/var/usage.swift b/test/decl/var/usage.swift
index b6236f8..73273f3 100644
--- a/test/decl/var/usage.swift
+++ b/test/decl/var/usage.swift
@@ -274,3 +274,34 @@
let x: Int // expected-warning {{immutable value 'x' was never used; consider removing it}}
x = 42
}
+
+// Tests fix to SR-964
+func sr964() {
+ var noOpSetter: String {
+ get { return "" }
+ set { } // No warning
+ }
+ var suspiciousSetter: String {
+ get { return "" }
+ set {
+ print(suspiciousSetter) // expected-warning {{setter argument 'newValue' was never used, but the property was accessed}} expected-note {{did you mean to use 'newValue' instead of accessing the property's current value?}} {{13-29=newValue}}
+ }
+ }
+ var namedSuspiciousSetter: String {
+ get { return "" }
+ set(parameter) {
+ print(namedSuspiciousSetter) // expected-warning {{setter argument 'parameter' was never used, but the property was accessed}} expected-note {{did you mean to use 'parameter' instead of accessing the property's current value?}} {{13-34=parameter}}
+ }
+ }
+ var okSetter: String {
+ get { return "" }
+ set { print(newValue) } // No warning
+ }
+ var multiTriggerSetter: String {
+ get { return "" }
+ set {
+ print(multiTriggerSetter) // expected-warning {{setter argument 'newValue' was never used, but the property was accessed}} expected-note {{did you mean to use 'newValue' instead of accessing the property's current value?}} {{13-31=newValue}}
+ print(multiTriggerSetter)
+ }
+ }
+}
