From ddd61920945d7f2b7d1b6e5fc99561ceb5d45793 Mon Sep 17 00:00:00 2001 From: Chris Lattner Date: Mon, 11 May 2015 06:26:05 +0000 Subject: [PATCH] Implement 3 prominent feature requests: warning that you can use 'let' not 'var' Compiler should warn me if I set a parameter as 'var' but never modify it QoI: warn about unused variables This uses a simple pass in MiscDiagnostics that walks the body of an AbstractFunctionDecl. This means that it doesn't warn about unused properties (etc), but it captures a vast majority of the cases. It also does not warn about unused parameters (as a policy decision) because it is too noisy, there are a variety of other refinements that could be done as well, thoughts welcome. Swift SVN r28412 --- include/swift/AST/DiagnosticsSema.def | 22 ++ lib/Sema/MiscDiagnostics.cpp | 359 ++++++++++++++++++ lib/Sema/MiscDiagnostics.h | 4 + lib/Sema/TypeCheckStmt.cpp | 6 +- test/Parse/switch.swift | 2 +- .../SILPasses/definite_init_diagnostics.swift | 57 ++- test/attr/attr_autoclosure.swift | 2 +- test/decl/class/override.swift | 11 +- test/decl/func/functions.swift | 2 +- test/decl/func/trailing_closures.swift | 2 +- test/decl/var/properties.swift | 11 +- test/decl/var/usage.swift | 135 +++++++ test/expr/closure/closures.swift | 27 +- ...raints-solution-computesubstitutions.swift | 2 +- 14 files changed, 599 insertions(+), 43 deletions(-) create mode 100644 test/decl/var/usage.swift diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def index 0482792c43e..3d2763bb6b4 100644 --- a/include/swift/AST/DiagnosticsSema.def +++ b/include/swift/AST/DiagnosticsSema.def @@ -2265,6 +2265,28 @@ ERROR(availability_declaration_less_available_than_protocol, sema_avail, NOTE(availability_protocol_requirement_here, sema_avail, none, "protocol requirement here", ()) + +//------------------------------------------------------------------------------ +// Variable usage diagnostics +//------------------------------------------------------------------------------ + +WARNING(pbd_never_used, sema_varusage, none, + "initialization of %select{variable|immutable value}1 %0 was never used" + "; consider replacing with assignment to '_' or removing it", + (Identifier, unsigned)) + +WARNING(variable_never_used, sema_varusage, none, + "%select{variable|immutable value}1 %0 was never used; " + "consider replacing with '_' or removing it", + (Identifier, unsigned)) +WARNING(variable_never_mutated, sema_varusage, none, + "%select{variable|parameter}1 %0 was never mutated; " + "consider changing to 'let' constant", + (Identifier, unsigned)) +WARNING(variable_never_read, sema_varusage, none, + "%select{variable|parameter}1 %0 was written to, but never read", + (Identifier, unsigned)) + #ifndef DIAG_NO_UNDEF # if defined(DIAG) # undef DIAG diff --git a/lib/Sema/MiscDiagnostics.cpp b/lib/Sema/MiscDiagnostics.cpp index 00c51e0e9ef..9a8c3b0dcbb 100644 --- a/lib/Sema/MiscDiagnostics.cpp +++ b/lib/Sema/MiscDiagnostics.cpp @@ -19,6 +19,7 @@ #include "swift/Basic/SourceManager.h" #include "swift/AST/ASTWalker.h" #include "swift/Parse/Lexer.h" +#include "llvm/ADT/MapVector.h" using namespace swift; //===--------------------------------------------------------------------===// @@ -774,6 +775,364 @@ void swift::performStmtDiagnostics(TypeChecker &TC, const Stmt *S) { return diagUnreachableCode(TC, S); } +//===--------------------------------------------------------------------===// +// Per func/init diagnostics +//===--------------------------------------------------------------------===// + +namespace { +class VarDeclUsageChecker : public ASTWalker { + TypeChecker &TC; + + // Keep track of some information about a variable. + enum { + RK_Read = 1, ///< Whether it was ever read. + RK_Written = 2, ///< Whether it was ever written or passed inout. + }; + + /// These are all of the variables that we are tracking. VarDecls get added + /// to this when the declaration is seen. We use a MapVector to keep the + /// diagnostics emission in deterministic order. + llvm::SmallMapVector VarDecls; + + bool sawError = false; + +public: + VarDeclUsageChecker(TypeChecker &TC, AbstractFunctionDecl *AFD) : TC(TC) { + // Track the parameters of the function. + for (auto P : AFD->getBodyParamPatterns()) + P->forEachVariable([&](VarDecl *VD) { + if (shouldTrackVarDecl(VD)) + VarDecls[VD] = 0; + }); + } + + // After we have scanned the entire region, diagnose variables that could be + // declared with a narrower usage kind. + ~VarDeclUsageChecker(); + + bool shouldTrackVarDecl(VarDecl *VD) { + // If the variable is implicit, ignore it. + if (VD->isImplicit() || VD->getLoc().isInvalid()) + return false; + + // If the variable was invalid, ignore it and notice that the code is + // malformed. + if (VD->isInvalid() || !VD->hasType()) { + sawError = true; + return false; + } + + // If the variable is already unnamed, ignore it. + if (!VD->hasName() || VD->getName().str() == "_") + return false; + + return true; + } + + void addMark(Decl *D, unsigned Flag) { + auto *vd = dyn_cast(D); + if (!vd) return; + + auto vdi = VarDecls.find(vd); + if (vdi != VarDecls.end()) + vdi->second |= Flag; + } + + void markBaseOfAbstractStorageDeclStore(Expr *E, ConcreteDeclRef decl); + + void markStoredOrInOutExpr(Expr *E, unsigned Flags); + + // We generally walk into declarations, other than types and nested functions. + // FIXME: peek into capture lists of nested functions. + bool walkToDeclPre(Decl *D) override { + if (isa(D)) + return false; + + // If this is a VarDecl, then add it to our list of things to track. + if (auto *vd = dyn_cast(D)) + if (shouldTrackVarDecl(vd)) + VarDecls[vd] = 0; + + if (auto *afd = dyn_cast(D)) { + // If this is a nested function with a capture list, mark any captured + // variables. + if (afd->isBodyTypeChecked()) { + for (const auto &capture : afd->getCaptureInfo().getCaptures()) + addMark(capture.getDecl(), RK_Read|RK_Written); + } else { + // If the body hasn't been type checked yet, be super-conservative and + // mark all variables as used. This can be improved later, e.g. by + // walking the untype-checked body to look for things that could + // possibly be used. + VarDecls.clear(); + } + + // Don't walk into it though, it may not even be type checked yet. + return false; + } + + + // 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. + return true; + } + + /// The heavy lifting happens when visiting expressions. + std::pair walkToExprPre(Expr *E) override; +}; +} + + +// After we have scanned the entire region, diagnose variables that could be +// declared with a narrower usage kind. +VarDeclUsageChecker::~VarDeclUsageChecker() { + // If we saw an ErrorExpr somewhere in the body, then we have a malformed AST + // and we know stuff got dropped. Instead of producing these diagnostics, + // lets let the bigger issues get resolved first. + if (sawError) + return; + + for (auto elt : VarDecls) { + auto *var = elt.first; + unsigned access = elt.second; + + // If this is a 'let' value, any stores to it are actually initializations, + // not mutations. + if (var->isLet()) + access &= ~RK_Written; + + // If this variable has WeakStorageType, then it can be mutated in ways we + // don't know. + if (var->getType()->is()) + access |= RK_Written; + + // If this is a vardecl with 'inout' type, then it is an inout argument to a + // function, never diagnose anything related to it. + if (var->getType()->is()) + 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(var)) + access |= RK_Read; + + // Diagnose variables that were never used (other than their + // initialization). + // + if (access == 0) { + // If the source of the VarDecl is a trivial PatternBinding with only a + // single binding, rewrite the whole thing into an assignment. + // let x = foo() + // -> + // _ = foo() + if (auto *pbd = var->getParentPatternBinding()) + if (pbd->getSingleVar() == var && pbd->getInit(0) != nullptr) { + unsigned varKind = var->isLet(); + TC.diagnose(var->getLoc(), diag::pbd_never_used, + var->getName(), varKind) + .fixItReplace(SourceRange(pbd->getLoc(), var->getLoc()), "_"); + continue; + } + + // Otherwise, this is something more complex, perhaps + // let (a,b) = foo() + // Just rewrite the one variable with a _. + unsigned varKind = var->isLet(); + TC.diagnose(var->getLoc(), diag::variable_never_used, + var->getName(), varKind) + .fixItReplace(var->getLoc(), "_"); + continue; + } + + // If this is a mutable 'var', and it was never written to, suggest + // upgrading to 'let'. We do this even for a parameter. + if (!var->isLet() && (access & RK_Written) == 0) { + SourceLoc FixItLoc; + + // Try to find the location of the 'var' so we can produce a fixit. If + // this is a simple PatternBinding, use its location. + if (auto *PBD = var->getParentPatternBinding()) + if (PBD->getSingleVar() == var) + FixItLoc = PBD->getLoc(); + + // If this is a parameter explicitly marked 'var', remove it. + if (auto *param = dyn_cast(var)) + if (auto *pattern = param->getParamParentPattern()) + if (auto *vp = dyn_cast(pattern)) { + TC.diagnose(var->getLoc(), diag::variable_never_mutated, + var->getName(), /*param*/1) + .fixItRemove(vp->getLoc()); + continue; + } + + unsigned varKind = isa(var); + // FIXME: fixit when we can find a pattern binding. + if (FixItLoc.isInvalid()) + TC.diagnose(var->getLoc(), diag::variable_never_mutated, + var->getName(), varKind); + else + TC.diagnose(var->getLoc(), diag::variable_never_mutated, + var->getName(), varKind) + .fixItReplace(FixItLoc, "let"); + continue; + } + + // If this is a variable that was only written to, emit a warning. + if ((access & RK_Read) == 0) { + TC.diagnose(var->getLoc(), diag::variable_never_read, var->getName(), + isa(var)); + continue; + } + } +} + +/// Handle a store to "x.y" where 'base' is the expression for x and 'decl' is +/// the decl for 'y'. +void VarDeclUsageChecker:: +markBaseOfAbstractStorageDeclStore(Expr *base, ConcreteDeclRef decl) { + // If the base is a class or an rvalue, then this store just loads the base. + if (base->getType()->isAnyClassReferenceType() || + !(base->getType()->isLValueType() || base->getType()->is())) { + base->walk(*this); + return; + } + + // If the store is to a non-mutating member, then this is just a load, even + // if the base is an inout expr. + auto *ASD = cast(decl.getDecl()); + if ((ASD->hasAccessorFunctions() && ASD->getSetter() && + !ASD->getSetter()->isMutating()) || + (ASD->hasAddressors() && ASD->getMutableAddressor() && + !ASD->getMutableAddressor()->isMutating())) { + // Sema conservatively converts the base to inout expr when it is an lvalue. + // Look through it because we know it isn't actually doing a load/store. + if (auto *ioe = dyn_cast(base)) + base = ioe->getSubExpr(); + base->walk(*this); + return; + } + + // Otherwise this is a read and write of the base. + return markStoredOrInOutExpr(base, RK_Written|RK_Read); +} + + + void VarDeclUsageChecker::markStoredOrInOutExpr(Expr *E, unsigned Flags) { + // Sema leaves some subexpressions null, which seems really unfortunate. It + // should replace them with ErrorExpr. + if (E == nullptr || !E->getType() || E->getType()->is()) { + sawError = true; + return; + } + + // Ignore parens and other easy cases. + E = E->getSemanticsProvidingExpr(); + + // If we found a decl that is being assigned to, then mark it. + if (auto *DRE = dyn_cast(E)) { + addMark(DRE->getDecl(), Flags); + return; + } + + if (auto *TE = dyn_cast(E)) { + for (auto &elt : TE->getElements()) + markStoredOrInOutExpr(elt, Flags); + return; + } + + // If this is an assignment into a mutating subscript lvalue expr, then we + // are mutating the base expression. We also need to visit the index + // expressions as loads though. + if (auto *SE = dyn_cast(E)) { + // The index of the subscript is evaluted as an rvalue. + SE->getIndex()->walk(*this); + if (SE->hasDecl()) + markBaseOfAbstractStorageDeclStore(SE->getBase(), SE->getDecl()); + else // FIXME: Should not be needed! + markStoredOrInOutExpr(SE->getBase(), RK_Written|RK_Read); + + return; + } + + if (auto *ioe = dyn_cast(E)) + return markStoredOrInOutExpr(ioe->getSubExpr(), RK_Written|RK_Read); + + if (auto *MRE = dyn_cast(E)) { + markBaseOfAbstractStorageDeclStore(MRE->getBase(), MRE->getMember()); + return; + } + + if (auto *TEE = dyn_cast(E)) + return markStoredOrInOutExpr(TEE->getBase(), Flags); + + + // If we don't know what kind of expression this is, assume it's a reference + // and mark it as a read. + E->walk(*this); +} + + + +/// The heavy lifting happens when visiting expressions. +std::pair VarDeclUsageChecker::walkToExprPre(Expr *E) { + // Sema leaves some subexpressions null, which seems really unfortunate. It + // should replace them with ErrorExpr. + if (E == nullptr || !E->getType() || E->getType()->is()) { + sawError = true; + return { false, E }; + } + + // If this is a DeclRefExpr found in a random place, it is a load of the + // vardecl. + if (auto *DRE = dyn_cast(E)) + addMark(DRE->getDecl(), RK_Read); + + // If this is an AssignExpr, see if we're mutating something that we know + // about. + if (auto *assign = dyn_cast(E)) { + markStoredOrInOutExpr(assign->getDest(), RK_Written); + + // Don't walk into the LHS of the assignment, only the RHS. + assign->getSrc()->walk(*this); + return { false, E }; + } + + // '&x' is a read and write of 'x'. + if (auto *io = dyn_cast(E)) { + markStoredOrInOutExpr(io->getSubExpr(), RK_Read|RK_Written); + // Don't bother walking into this. + return { false, E }; + } + + // If we saw an ErrorExpr, take note of this. + if (isa(E)) + sawError = true; + + return { true, E }; +} + + + +/// Perform diagnostics for func/init/deinit declarations. +void swift::performAbstractFuncDeclDiagnostics(TypeChecker &TC, + AbstractFunctionDecl *AFD) { + assert(AFD->getBody() && "Need a body to check"); + + // Don't produce these diagnostics for implicitly generated code. + if (AFD->getLoc().isInvalid() || AFD->isImplicit() || AFD->isInvalid()) + return; + + // Check for unused variables, as well as variables that are could be + // declared as constants. + AFD->getBody()->walk(VarDeclUsageChecker(TC, AFD)); +} + + + + //===--------------------------------------------------------------------===// // Utility functions //===--------------------------------------------------------------------===// diff --git a/lib/Sema/MiscDiagnostics.h b/lib/Sema/MiscDiagnostics.h index 6081bc5a1a0..74156bfc980 100644 --- a/lib/Sema/MiscDiagnostics.h +++ b/lib/Sema/MiscDiagnostics.h @@ -16,6 +16,7 @@ #include "swift/AST/Attr.h" namespace swift { + class AbstractFunctionDecl; class DeclContext; class Expr; class InFlightDiagnostic; @@ -29,6 +30,9 @@ void performExprDiagnostics(TypeChecker &TC, const Expr *E, /// \brief Emit diagnostics for a given statement. void performStmtDiagnostics(TypeChecker &TC, const Stmt *S); +void performAbstractFuncDeclDiagnostics(TypeChecker &TC, + AbstractFunctionDecl *AFD); + /// Emit a fix-it to set the accessibility of \p VD to \p desiredAccess. /// /// This actually updates \p VD as well. diff --git a/lib/Sema/TypeCheckStmt.cpp b/lib/Sema/TypeCheckStmt.cpp index e8f671091ea..588a6441b2c 100644 --- a/lib/Sema/TypeCheckStmt.cpp +++ b/lib/Sema/TypeCheckStmt.cpp @@ -1120,7 +1120,11 @@ bool TypeChecker::typeCheckAbstractFunctionBody(AbstractFunctionDecl *AFD) { if (DebugTimeFunctionBodies) timer.emplace(AFD); - return typeCheckAbstractFunctionBodyUntil(AFD, SourceLoc()); + if (typeCheckAbstractFunctionBodyUntil(AFD, SourceLoc())) + return true; + + performAbstractFuncDeclDiagnostics(*this, AFD); + return false; } // Type check a function body (defined with the func keyword) that is either a diff --git a/test/Parse/switch.swift b/test/Parse/switch.swift index ad4e23b7282..209dc444a19 100644 --- a/test/Parse/switch.swift +++ b/test/Parse/switch.swift @@ -28,7 +28,7 @@ func parseError4(x: Int) { func parseError5(x: Int) { switch x { - case var z // expected-error {{expected ':' after 'case'}} + case let z // expected-error {{expected ':' after 'case'}} expected-warning {{immutable value 'z' was never used}} } } diff --git a/test/SILPasses/definite_init_diagnostics.swift b/test/SILPasses/definite_init_diagnostics.swift index 66808fab5d3..611eafd9db4 100644 --- a/test/SILPasses/definite_init_diagnostics.swift +++ b/test/SILPasses/definite_init_diagnostics.swift @@ -12,6 +12,7 @@ func markUsed(t: T) {} // memory promotion pass. func test1() -> Int { + // expected-warning @+1 {{variable 'a' was never mutated; consider changing to 'let' constant}} var a : Int // expected-note {{variable defined here}} return a // expected-error {{variable 'a' used before being initialized}} } @@ -54,6 +55,7 @@ func test2() { // Closures. + // expected-warning @+1 {{variable 'b1' was never mutated}} var b1 : Int // expected-note {{variable defined here}} takes_closure { // expected-error {{variable 'b1' used before being initialized}} markUsed(b1) @@ -63,6 +65,7 @@ func test2() { takes_closure { // ok. markUsed(b2) } + b2 = 1 var b3 : Int b3 = 4 @@ -73,17 +76,19 @@ func test2() { // Structs var s1 : SomeStruct s1 = SomeStruct() // ok + _ = s1 var s2 : SomeStruct // expected-note {{variable defined here}} s2.x = 1 // expected-error {{struct 's2' must be completely initialized before a member is stored to}} // Classes + // expected-warning @+1 {{variable 'c1' was never mutated}} var c1 : SomeClass // expected-note {{variable defined here}} markUsed(c1.x) // expected-error {{variable 'c1' used before being initialized}} - var c2 = SomeClass() + let c2 = SomeClass() markUsed(c2.x) // ok @@ -97,10 +102,11 @@ func test2() { // Unowned. This is immediately crashing code (it causes a retain of a // released object) so it should be diagnosed with a warning someday. + // expected-warning @+1 {{variable 'u1' was never mutated; consider changing to 'let' constant}} unowned var u1 : SomeClass // expected-note {{variable defined here}} var _ = u1 // expected-error {{variable 'u1' used before being initialized}} - unowned var u2 = SomeClass() + unowned let u2 = SomeClass() var _ = u2 // ok } @@ -108,10 +114,12 @@ func test2() { // Tuple field sensitivity. func test4() { + // expected-warning @+1 {{variable 't1' was never mutated; consider changing to 'let' constant}} var t1 = (1, 2, 3) markUsed(t1.0 + t1.1 + t1.2) // ok + // expected-warning @+1 {{variable 't2' was never mutated; consider changing to 'let' constant}} var t2 : (Int, Int, Int) // expected-note 3 {{variable defined here}} markUsed(t2.0) // expected-error {{variable 't2.0' used before being initialized}} markUsed(t2.1) // expected-error {{variable 't2.1' used before being initialized}} @@ -156,7 +164,7 @@ func test5(x: T, y: T) { var a : ((T, T), T) // expected-note {{variable defined here}} a.0 = (x, y) - var b = a // expected-error {{variable 'a.1' used before being initialized}} + _ = a // expected-error {{variable 'a.1' used before being initialized}} } @@ -190,24 +198,32 @@ protocol SomeProtocol { protocol DerivedProtocol : SomeProtocol {} func existentials(i: Int, dp: DerivedProtocol) { + // expected-warning @+1 {{variable 'a' was written to, but never read}} var a : Any = () a = i + // expected-warning @+1 {{variable 'b' was written to, but never read}} var b : Any b = () + // expected-warning @+1 {{variable 'c' was never used}} var c : Any // no uses. + // expected-warning @+1 {{variable 'd1' was never mutated}} var d1 : Any // expected-note {{variable defined here}} - var d2 = d1 // expected-error {{variable 'd1' used before being initialized}} + _ = d1 // expected-error {{variable 'd1' used before being initialized}} + + // expected-warning @+1 {{variable 'e' was never mutated}} var e : SomeProtocol // expected-note {{variable defined here}} e.protoMe() // expected-error {{variable 'e' used before being initialized}} var f : SomeProtocol = dp // ok, init'd by existential upcast. + // expected-warning @+1 {{variable 'g' was never mutated}} var g : DerivedProtocol // expected-note {{variable defined here}} f = g // expected-error {{variable 'g' used before being initialized}} + _ = f } @@ -235,9 +251,11 @@ struct EmptyStruct {} func useEmptyStruct(a: EmptyStruct) {} func emptyStructTest() { - var a : EmptyStruct // expected-note {{variable defined here}} + let a : EmptyStruct // expected-note {{variable defined here}} useEmptyStruct(a) // expected-error {{variable 'a' used before being initialized}} + // expected-warning @+2 {{variable 'b' was never mutated}} + // expected-warning @+1 {{variable 'd' was never mutated}} var (b,c,d) : (EmptyStruct,EmptyStruct,EmptyStruct) // expected-note 2 {{variable defined here}} c = EmptyStruct() @@ -271,6 +289,7 @@ func partialInit() { var x : Int if (n > 2) { continue } x = 1 + _ = x } } @@ -278,6 +297,7 @@ func partialInit() { func trivial_tuple() { var a : (Int, Int) a.1 = 1 + _ = a.1 } @@ -288,6 +308,7 @@ func partialInit() { x.0 = SomeClass() } else { x.1 = SomeClass() + _ = x.1 } } } @@ -305,6 +326,7 @@ func conditionalInitOrAssign(c : Bool, x : Int) { t = x } t = 2 + _ = t // Nontrivial type var sc : SomeClass @@ -312,6 +334,7 @@ func conditionalInitOrAssign(c : Bool, x : Int) { sc = SomeClass() } sc = SomeClass() + _ = sc // Tuple element types var tt : (SomeClass, SomeClass) @@ -354,12 +377,12 @@ func tuple_test() -> Int { t.1 = 4 - for i in 0..<45 { + for _ in 0..<45 { } t.0 = 1 - for i in 0..<45 { + for _ in 0..<45 { } return t.1+t.0 // No diagnostic, everything is fully initialized. @@ -443,7 +466,7 @@ class DelegatingCtorClass { convenience init(x: EmptyStruct) { self.init() - var tmp = ivar // okay: ivar has been initialized by the delegation above + _ = ivar // okay: ivar has been initialized by the delegation above } convenience init(x: EmptyStruct, y: EmptyStruct) { @@ -480,11 +503,11 @@ struct DelegatingCtorStruct { init(a : Double) { self.init() - var tmp = ivar // okay: ivar has been initialized by the delegation above + _ = ivar // okay: ivar has been initialized by the delegation above } init(a : Int) { - var tmp = ivar // expected-error {{use of 'self' in delegating initializer before self.init is called}} + _ = ivar // expected-error {{use of 'self' in delegating initializer before self.init is called}} self.init() } @@ -511,12 +534,12 @@ enum DelegatingCtorEnum { init(a : Double) { self.init() - var tmp = self // okay: self has been initialized by the delegation above + _ = self // okay: self has been initialized by the delegation above self = .Dinosaur } init(a : Int) { - var tmp = self // expected-error {{use of 'self' in delegating initializer before self.init is called}} + _ = self // expected-error {{use of 'self' in delegating initializer before self.init is called}} self.init() } @@ -577,7 +600,7 @@ class rdar16119509_Derived : rdar16119509_Base { // Bogus error: self.init called multiple times in initializer extension Foo { convenience init() { - for i in 0..<42 { + for _ in 0..<42 { } self.init(a: 4) } @@ -928,7 +951,7 @@ struct MyMutabilityImplementation : TestMutabilityProtocol { func testLocalProperties(b : Int) -> Int { // expected-note @+1 {{change 'let' to 'var' to make it mutable}} let x : Int - let y : Int // never assigned is ok + let y : Int // never assigned is ok expected-warning {{immutable value 'y' was never used}} x = b x = b // expected-error {{immutable value 'x' may only be initialized once}} @@ -941,6 +964,7 @@ func testLocalProperties(b : Int) -> Int { z = b } + _ = z return x } @@ -949,7 +973,7 @@ func testAddressOnlyProperty(b : T) -> T { // expected-note @+1 {{change 'let' to 'var' to make it mutable}} let x : T let y : T - let z : T // never assigned is ok. + let z : T // never assigned is ok. expected-warning {{immutable value 'z' was never used}} x = b y = b x = b // expected-error {{immutable value 'x' may only be initialized once}} @@ -1023,7 +1047,7 @@ class MyClassTestExample { init(){ clientFormat = MyClassWithAnInt() - let channels = clientFormat.channelCount + let _ = clientFormat.channelCount } } @@ -1101,6 +1125,7 @@ func testReassignment() { let c : Int // expected-note {{change 'let' to 'var' to make it mutable}} c = 12 c = 32 // expected-error {{immutable value 'c' may only be initialized once}} + _ = c } diff --git a/test/attr/attr_autoclosure.swift b/test/attr/attr_autoclosure.swift index 0a606377edc..85c30b29076 100644 --- a/test/attr/attr_autoclosure.swift +++ b/test/attr/attr_autoclosure.swift @@ -12,7 +12,7 @@ func func2(@autoclosure fp : () -> Int) { func2(4)} func func3(@autoclosure fp fpx : () -> Int) {func3(fp: 0)} func func4(@autoclosure fp fp : () -> Int) {func4(fp: 0)} -func func5(@autoclosure var fp fp : () -> Int) {func5(fp: 0)} +func func5(@autoclosure var fp fp : () -> Int) {func5(fp: 0)} // expected-warning {{parameter 'fp' was never mutated}} func func6(@autoclosure () -> Int) {func6(0)} // declattr and typeattr on the argument. diff --git a/test/decl/class/override.swift b/test/decl/class/override.swift index bc43efd8c63..5f28bd8f537 100644 --- a/test/decl/class/override.swift +++ b/test/decl/class/override.swift @@ -144,12 +144,13 @@ class H : G { func oneA(_: AnyObject) {} func oneB(x x: AnyObject) {} - func oneC(var x x: AnyObject) {} + func oneC(var x x: AnyObject) {} // expected-warning {{parameter 'x' was never mutated}} func oneD(x: AnyObject) {} func manyA(_: AnyObject, _: AnyObject) {} func manyB(a: AnyObject, b: AnyObject) {} - func manyC(var a: AnyObject, var b: AnyObject) {} + func manyC(var a: AnyObject, // expected-warning {{parameter 'a' was never mutated}} + var b: AnyObject) {} // expected-warning {{parameter 'b' was never mutated}} func result() -> AnyObject? { return nil } func both(x: AnyObject) -> AnyObject? { return x } @@ -166,7 +167,7 @@ class IUOTestSubclass : IUOTestBaseClass { override func oneB(x x: AnyObject!) {} // expected-warning {{overriding instance method parameter of type 'AnyObject' with implicitly unwrapped optional type 'AnyObject!'}} // expected-note@-1 {{remove '!' to make the parameter required}} {{36-37=}} // expected-note@-2 {{add parentheses to silence this warning}} {{27-27=(}} {{37-37=)}} - override func oneC(var x x: AnyObject!) {} // expected-warning {{overriding instance method parameter of type 'AnyObject' with implicitly unwrapped optional type 'AnyObject!'}} + override func oneC(var x x: AnyObject!) {} // expected-warning {{overriding instance method parameter of type 'AnyObject' with implicitly unwrapped optional type 'AnyObject!'}} expected-warning {{parameter 'x' was never mutated}} // expected-note@-1 {{remove '!' to make the parameter required}} {{40-41=}} // expected-note@-2 {{add parentheses to silence this warning}} {{31-31=(}} {{41-41=)}} override func oneD(x: AnyObject!) {} // expected-warning {{overriding instance method parameter of type 'AnyObject' with implicitly unwrapped optional type 'AnyObject!'}} @@ -179,7 +180,7 @@ class IUOTestSubclass : IUOTestBaseClass { override func manyB(a: AnyObject!, b: AnyObject!) {} // expected-warning 2 {{overriding instance method parameter of type 'AnyObject' with implicitly unwrapped optional type 'AnyObject!'}} // expected-note@-1 2 {{remove '!' to make the parameter required}} // expected-note@-2 2 {{add parentheses to silence this warning}} - override func manyC(var a: AnyObject!, var b: AnyObject!) {} // expected-warning 2 {{overriding instance method parameter of type 'AnyObject' with implicitly unwrapped optional type 'AnyObject!'}} + override func manyC(var a: AnyObject!, var b: AnyObject!) {} // expected-warning 2 {{overriding instance method parameter of type 'AnyObject' with implicitly unwrapped optional type 'AnyObject!'}} expected-warning {{parameter 'a' was never mutated}} expected-warning {{parameter 'b' was never mutated}} // expected-note@-1 2 {{remove '!' to make the parameter required}} // expected-note@-2 2 {{add parentheses to silence this warning}} @@ -205,7 +206,7 @@ class IUOTestSubclass2 : IUOTestBaseClass { override func oneA(x: AnyObject!) {} // expected-warning {{overriding instance method parameter of type 'AnyObject' with implicitly unwrapped optional type 'AnyObject!'}} // expected-note@-1 {{remove '!' to make the parameter required}} {{34-35=}} // expected-note@-2 {{add parentheses to silence this warning}} {{25-25=(}} {{35-35=)}} - override func oneB(var x x: AnyObject!) {} // expected-warning {{overriding instance method parameter of type 'AnyObject' with implicitly unwrapped optional type 'AnyObject!'}} + override func oneB(var x x: AnyObject!) {} // expected-warning {{overriding instance method parameter of type 'AnyObject' with implicitly unwrapped optional type 'AnyObject!'}} expected-warning {{parameter 'x' was never mutated}} // expected-note@-1 {{remove '!' to make the parameter required}} {{40-41=}} // expected-note@-2 {{add parentheses to silence this warning}} {{31-31=(}} {{41-41=)}} override func oneD(_: AnyObject!) {} // expected-warning {{overriding instance method parameter of type 'AnyObject' with implicitly unwrapped optional type 'AnyObject!'}} diff --git a/test/decl/func/functions.swift b/test/decl/func/functions.swift index 054fce12026..cf6ebda4d1a 100644 --- a/test/decl/func/functions.swift +++ b/test/decl/func/functions.swift @@ -134,7 +134,7 @@ func !!!(lhs: UnsafePointer, rhs: UnsafePointer) -> Bool { return false // Functions currently permit 'var inout' parameters func var_inout_error(inout var x : Int) {} // expected-error {{parameter may not have multiple 'inout', 'var', or 'let' specifiers}} -func var_inout_error(var inout x : Int) {} // expected-error {{parameter may not have multiple 'inout', 'var', or 'let' specifiers}} +func var_inout_error(var inout x : Int) {} // expected-error {{parameter may not have multiple 'inout', 'var', or 'let' specifiers}} expected-warning {{parameter 'x' was never mutated}} diff --git a/test/decl/func/trailing_closures.swift b/test/decl/func/trailing_closures.swift index 8b07b73d12e..86ab6e07db1 100644 --- a/test/decl/func/trailing_closures.swift +++ b/test/decl/func/trailing_closures.swift @@ -5,7 +5,7 @@ func f1(f: () -> (), bar: Int = 10) { } // expected-warning{{closure parameter prior to parameters with default arguments will not be treated as a trailing closure}} func f2(f: (() -> ())!, bar: Int = 10, wibble: Int = 17) { } // expected-warning{{closure parameter prior to parameters with default arguments will not be treated as a trailing closure}} func f3(f: (() -> ())?, bar: Int = 10) { } // expected-warning{{closure parameter prior to parameters with default arguments will not be treated as a trailing closure}} -func f4(var f: (() -> ())?, bar: Int = 10) { } // expected-warning{{closure parameter prior to parameters with default arguments will not be treated as a trailing closure}} +func f4(var f: (() -> ())?, bar: Int = 10) { } // expected-warning{{closure parameter prior to parameters with default arguments will not be treated as a trailing closure}} expected-warning {{parameter 'f' was never mutated}} // An extra set of parentheses suppresses the warning. func g1(f: (() -> ()), bar: Int = 10) { } // no-warning diff --git a/test/decl/var/properties.swift b/test/decl/var/properties.swift index a31e64ae826..1af4084a837 100644 --- a/test/decl/var/properties.swift +++ b/test/decl/var/properties.swift @@ -113,7 +113,7 @@ var x15: Int { // For the purpose of this test we need to use an attribute that can not be // applied to the getter. weak - var foo: SomeClass? = SomeClass() + var foo: SomeClass? = SomeClass() // expected-warning {{variable 'foo' was written to, but never read}} return 0 } @@ -179,7 +179,7 @@ func disambiguateGetSet3Attr() { // Disambiguated as stored property with a trailing closure in the initializer. func disambiguateGetSet4() { func set(x: Int, fn: () -> ()) {} - var newValue: Int = 0 + let newValue: Int = 0 var a: Int = takeTrailingClosure { set(newValue) {} } @@ -425,7 +425,7 @@ extension ProtocolWithExtension1 { } func getS() -> S { - var s: S + let s: S return s } @@ -491,7 +491,8 @@ func accept_int(c: Int) { } func test_settable_of_nonsettable(a: Aleph) { a.b.c = 1 // expected-error{{cannot assign}} - var x:Int = a.b.c + let x:Int = a.b.c + _ = x accept_int(a.b.c) accept_int_inout(&a.b.c) // expected-error {{cannot pass immutable value of type 'Int' as inout argument}} @@ -597,7 +598,7 @@ class SelfRefProperties { } set { markUsed(setter) // no-warning - var unused = setter + setter + var unused = setter + setter // expected-warning {{initialization of variable 'unused' was never used; consider replacing with assignment to '_' or removing it}} setter = newValue // expected-warning {{attempting to modify 'setter' within its own setter}} // expected-note@-1 {{access 'self' explicitly to silence this warning}} {{7-7=self.}} } diff --git a/test/decl/var/usage.swift b/test/decl/var/usage.swift new file mode 100644 index 00000000000..bfb2f7e8ed9 --- /dev/null +++ b/test/decl/var/usage.swift @@ -0,0 +1,135 @@ +// RUN: %target-parse-verify-swift + +// QoI: warn about unused variables +// warning that you can use 'let' not 'var' +// Compiler should warn me if I set a parameter as 'var' but never modify it + +func basicTests() -> Int { + let x = 42 // expected-warning {{immutable value 'x' was never used; consider replacing with assignment to '_' or removing it}} + var y = 12 // expected-warning {{variable 'y' was never mutated; consider changing to 'let' constant}} + let _ = 42 // ok + _ = 42 // ok + return y +} + +func mutableParameter(a : Int, h : Int, var i : Int, var j: Int, + var g : Int) -> Int { // expected-warning {{parameter 'g' was never mutated; consider changing to 'let' constant}} + swap(&i, &j) + return i+g +} + +struct X { + func f() {} + mutating func g() {} +} + + +func testStruct() { + let a = X() + a.f() + + var b = X() + b.g() + + var c = X() // expected-warning {{variable 'c' was never mutated; consider changing to 'let' constant}} + c.f() +} + +func takeClosure(fn : () -> ()) {} + +class TestClass { + + func f() { + + takeClosure { [weak self] in // self is mutable but never mutated. Ok because it is weak + self?.f() + } + } +} + + +func nestedFunction() -> Int { + var x = 42 // No warning about being never-set. + + func g() { + x = 97 + var q = 27 // expected-warning {{variable 'q' was never used}} + } + g() + + return x +} + +func neverRead() { + var x = 42 // expected-warning {{variable 'x' was written to, but never read}} + + x = 97 + x = 123 +} + +func property() -> Int { + var p : Int { // everything ok + return 42 + } + return p +} + + +func testInOut(inout x : Int) { // Ok. +} + +struct TestStruct { + var property = 42 +} + + +func testStructMember() -> TestStruct { + var x = TestStruct() // ok + x.property = 17 + return x +} + + +func testSubscript() -> [Int] { + var x = [1,2,3] // ok + x[1] = 27 + return x +} + + +func testTuple(var x : Int) -> Int { + var y : Int // Ok, stored by a tuple + + (x, y) = (1,2) + return y +} + + + + struct TestComputedPropertyStruct { + + var x : Int { + get {} + nonmutating set {} + } +} + +func test() { + let v = TestComputedPropertyStruct() + v.x = 42 + + var v2 = TestComputedPropertyStruct() // expected-warning {{variable 'v2' was never mutated; consider changing to 'let' constant}} + v2.x = 42 +} + +func test4() { + // expected-warning @+1 {{variable 'dest' was never mutated; consider changing to 'let' constant}} + var dest = UnsafeMutablePointer(bitPattern: 0) + + dest[0] = 0 +} + +func testTuple() { + var tup : (x:Int, y:Int) // expected-warning {{variable 'tup' was written to, but never read}} + tup.x = 1 +} diff --git a/test/expr/closure/closures.swift b/test/expr/closure/closures.swift index b695f7cce5c..4b4ce316761 100644 --- a/test/expr/closure/closures.swift +++ b/test/expr/closure/closures.swift @@ -92,8 +92,8 @@ func closure_no_body(p: () -> ()) { // rdar://12019415 func t() { - var u8 : UInt8 - var x : Bool + let u8 : UInt8 = 1 + let x : Bool = true if 0xA0..<0xBF ~= Int(u8) && x { } @@ -139,9 +139,11 @@ class ExplicitSelfRequiredTest { // "self." shouldn't be required in the initializer expression in a capture list // This should not produce an error, "x" isn't being captured by the closure. + // expected-warning @+1 {{initialization of immutable value 'myX' was never used}} doStuff({ [myX = x] in 4 }) // This should produce an error, since x is used within the inner closure. + // expected-warning @+1 {{initialization of immutable value 'myX' was never used}} doStuff({ [myX = {x}] in 4 }) // expected-error {{reference to property 'x' in closure requires explicit 'self.' to make capture semantics explicit}} return 42 @@ -161,9 +163,9 @@ class SomeClass { func testCaptureBehavior(ptr : SomeClass) { // Test normal captures. weak var wv : SomeClass? = ptr - unowned var uv : SomeClass = ptr - unowned(unsafe) var uv1 : SomeClass = ptr - unowned(safe) var uv2 : SomeClass = ptr + unowned let uv : SomeClass = ptr + unowned(unsafe) let uv1 : SomeClass = ptr + unowned(safe) let uv2 : SomeClass = ptr doStuff { wv!.foo() } doStuff { uv.foo() } doStuff { uv1.foo() } @@ -171,10 +173,11 @@ func testCaptureBehavior(ptr : SomeClass) { // Capture list tests - var v1 : SomeClass? = ptr - var v2 : SomeClass = ptr + let v1 : SomeClass? = ptr + let v2 : SomeClass = ptr doStuff { [weak v1] in v1!.foo() } + // expected-warning @+2 {{variable 'v1' was written to, but never read}} doStuff { [weak v1, // expected-note {{previous}} weak v1] in v1!.foo() } // expected-error {{definition conflicts with previous value}} doStuff { [unowned v2] in v2.foo() } @@ -182,7 +185,8 @@ func testCaptureBehavior(ptr : SomeClass) { doStuff { [unowned(safe) v2] in v2.foo() } doStuff { [weak v1, weak v2] in v1!.foo() + v2!.foo() } - var i = 42 + let i = 42 + // expected-warning @+1 {{variable 'i' was never mutated}} doStuff { [weak i] in i! } // expected-error {{'weak' cannot be applied to non-class type 'Int'}} } @@ -194,13 +198,14 @@ extension SomeClass { // rdar://16889886 - Assert when trying to weak capture a property of self in a lazy closure doStuff { [weak self.field] in field!.foo() } // expected-error {{fields may only be captured by assigning to a specific name}} expected-error {{reference to property 'field' in closure requires explicit 'self.' to make capture semantics explicit}} + // expected-warning @+1 {{variable 'self' was written to, but never read}} doStuff { [weak self&field] in 42 } // expected-error {{expected ']' at end of capture list}} } func strong_in_capture_list() { // QOI: "[strong self]" in capture list generates unhelpful error message - var fn = {[strong self] () -> () in return } // expected-error {{expected 'weak', 'unowned', or no specifier in capture list}} + var _ = {[strong self] () -> () in return } // expected-error {{expected 'weak', 'unowned', or no specifier in capture list}} } } @@ -209,10 +214,10 @@ extension SomeClass { var closureWithObservedProperty: () -> () = { var a: Int = 42 { willSet { - let message = "Will set a to \(newValue)" + let _ = "Will set a to \(newValue)" } didSet { - let message = "Did set a with old value of \(oldValue)" + let _ = "Did set a with old value of \(oldValue)" } } } diff --git a/validation-test/compiler_crashers_fixed/0040-std-function-func-swift-constraints-solution-computesubstitutions.swift b/validation-test/compiler_crashers_fixed/0040-std-function-func-swift-constraints-solution-computesubstitutions.swift index 2486326ac64..8f31b12f05a 100644 --- a/validation-test/compiler_crashers_fixed/0040-std-function-func-swift-constraints-solution-computesubstitutions.swift +++ b/validation-test/compiler_crashers_fixed/0040-std-function-func-swift-constraints-solution-computesubstitutions.swift @@ -5,6 +5,6 @@ // rdar://18175202 func d == b.Generator.Element>(c : b) -> e? { - for mx : e? in c { + for mx : e? in c { // expected-warning {{immutable value 'mx' was never used}} } }