diff --git a/.swiftlint.yml b/.swiftlint.yml index 3f27461534..cba38f4e75 100644 --- a/.swiftlint.yml +++ b/.swiftlint.yml @@ -51,6 +51,7 @@ disabled_rules: - trailing_closure - type_contents_order - vertical_whitespace_between_cases + - variable_shadowing # Configurations attributes: diff --git a/CHANGELOG.md b/CHANGELOG.md index f6455ddce4..33c047b76d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -52,6 +52,11 @@ [kapitoshka438](https://github.com/kapitoshka438) [#6045](https://github.com/realm/SwiftLint/issues/6045) +* Add `variable_shadowing` rule that flags when a variable declaration shadows + an identifier from an outer scope. + [nadeemnali](https://github.com/nadeemnali) + [#6228](https://github.com/realm/SwiftLint/issues/6228) + ### Bug Fixes * Add an `ignore_attributes` option to `implicit_optional_initialization` so diff --git a/Source/SwiftLintBuiltInRules/Models/BuiltInRules.swift b/Source/SwiftLintBuiltInRules/Models/BuiltInRules.swift index b6341fe12e..b30f1fbf89 100644 --- a/Source/SwiftLintBuiltInRules/Models/BuiltInRules.swift +++ b/Source/SwiftLintBuiltInRules/Models/BuiltInRules.swift @@ -240,6 +240,7 @@ public let builtInRules: [any Rule.Type] = [ UnusedParameterRule.self, UnusedSetterValueRule.self, ValidIBInspectableRule.self, + VariableShadowingRule.self, VerticalParameterAlignmentOnCallRule.self, VerticalParameterAlignmentRule.self, VerticalWhitespaceBetweenCasesRule.self, diff --git a/Source/SwiftLintBuiltInRules/Rules/Lint/VariableShadowingRule.swift b/Source/SwiftLintBuiltInRules/Rules/Lint/VariableShadowingRule.swift new file mode 100644 index 0000000000..e6e047db6a --- /dev/null +++ b/Source/SwiftLintBuiltInRules/Rules/Lint/VariableShadowingRule.swift @@ -0,0 +1,313 @@ +import SwiftLintCore +import SwiftSyntax + +@SwiftSyntaxRule +struct VariableShadowingRule: Rule { + var configuration = VariableShadowingConfiguration() + + static let description = RuleDescription( + identifier: "variable_shadowing", + name: "Variable Shadowing", + description: "Do not shadow variables declared in outer scopes", + kind: .lint, + nonTriggeringExamples: [ + Example(""" + var a: String? + func test(a: String?) { + print(a) + } + """, configuration: ["ignore_parameters": true]), + Example(""" + var a: String = "hello" + if let b = a { + print(b) + } + """), + Example(""" + var a: String? + func test() { + if let b = a { + print(b) + } + } + """), + Example(""" + for i in 1...10 { + print(i) + } + for j in 1...10 { + print(j) + } + """), + Example(""" + func test() { + var a: String = "hello" + func nested() { + var b: String = "world" + print(a, b) + } + } + """), + Example(""" + class Test { + var a: String? + func test(a: String?) { + print(a) + } + } + """), + Example(""" + var outer: String = "hello" + if let inner = Optional(outer) { + print(inner) + } + """), + Example(""" + var a: String = "outer" + let (b, c) = ("first", "second") + print(a, b, c) + """), + Example(""" + class Test { + var property: String = "class property" + func test() { + var localVar = "local" + print(property, localVar) + } + } + """), + Example(""" + func outer() { + func inner() { + print("no shadowing") + } + } + """), + Example(""" + var result: String? + if let unwrappedResult = result { + print(unwrappedResult) + } + """), + Example(""" + var value: Int? = 10 + guard let safeValue = value else { + return + } + print(safeValue) + """), + Example(""" + var data: [Int?] = [1, nil, 3] + for case let item? in data { + print(item) + } + """), + ], + triggeringExamples: [ + Example(""" + var outer: String = "hello" + func test() { + let ↓outer = "world" + print(outer) + } + """), + Example(""" + var x = 1 + do { + let ↓x = 2 + print(x) + } + """), + Example(""" + var counter = 0 + func incrementCounter() { + var ↓counter = 1 + counter += 1 + } + """), + Example(""" + func outer() { + var value = 10 + do { + let ↓value = 20 + print(value) + } + } + """), + Example(""" + var globalName = "global" + func test() { + for item in [1, 2, 3] { + var ↓globalName = "local" + print(globalName) + } + } + """), + Example(""" + var foo = 1 + do { + let ↓foo = 2 + } + """), + Example(""" + var bar = 1 + func test() { + let ↓bar = 2 + } + """), + Example(""" + var a = 1 + if let ↓a = Optional(2) { + let ↓a = 3 + print(a) + } + """), + Example(""" + var i = 1 + for ↓i in 1...3 { + let ↓i = 2 + print(i) + } + """), + Example(""" + func test() { + var a = 1 + do { + var ↓a = 2 + print(a) + } + } + """), + Example(""" + func test() { + var a = 1 + if true { + var ↓a = 2 + print(a) + } + } + """), + Example(""" + func test() { + var a = 1 + for _ in 0..<1 { + var ↓a = 2 + print(a) + } + } + """), + Example(""" + func test() { + var a = 1 + while true { + var ↓a = 2 + break + } + } + """), + Example(""" + var a = 1 + if let ↓a = Optional(2) {} + """), + Example(""" + var i = 1 + for ↓i in 1...3 {} + """), + Example(""" + var a: String? + func test(↓a: String?) { + print(a) + } + """, configuration: ["ignore_parameters": false]), + ] + ) +} + +private extension VariableShadowingRule { + final class Visitor: DeclaredIdentifiersTrackingVisitor { + override func visit(_ node: VariableDeclSyntax) -> SyntaxVisitorContinueKind { + if node.parent?.is(MemberBlockItemSyntax.self) == false { + node.bindings.forEach { binding in + checkForShadowing(in: binding.pattern) + } + } + return super.visit(node) + } + + override func visit(_ node: FunctionDeclSyntax) -> SyntaxVisitorContinueKind { + if !configuration.ignoreParameters { + for param in node.signature.parameterClause.parameters { + let nameToken = param.secondName ?? param.firstName + if nameToken.text != "_", isShadowingAnyScope(nameToken.text) { + violations.append(nameToken.positionAfterSkippingLeadingTrivia) + } + } + } + return super.visit(node) + } + + override func visit(_ node: ForStmtSyntax) -> SyntaxVisitorContinueKind { + checkForBindingShadowing(in: node.pattern) + return super.visit(node) + } + + override func visit(_ node: IfExprSyntax) -> SyntaxVisitorContinueKind { + for condition in node.conditions { + if let optBinding = condition.condition.as(OptionalBindingConditionSyntax.self) { + checkForBindingShadowing(in: optBinding.pattern) + } + } + return super.visit(node) + } + + // Used for VariableDecl: the new identifier is added to the *current* scope, + // so we only check ancestor scopes (dropLast). + private func checkForShadowing(in pattern: PatternSyntax) { + if let identifier = pattern.as(IdentifierPatternSyntax.self) { + let identifierText = identifier.identifier.text + if isShadowingOuterScope(identifierText) { + violations.append(identifier.identifier.positionAfterSkippingLeadingTrivia) + } + } else if let tuple = pattern.as(TuplePatternSyntax.self) { + tuple.elements.forEach { element in + checkForShadowing(in: element.pattern) + } + } else if let valueBinding = pattern.as(ValueBindingPatternSyntax.self) { + checkForShadowing(in: valueBinding.pattern) + } + } + + // Used for if-let / for-loop bindings: the new identifier is added to a *child* scope, + // so we check all current scopes. + private func checkForBindingShadowing(in pattern: PatternSyntax) { + if let identifier = pattern.as(IdentifierPatternSyntax.self) { + let identifierText = identifier.identifier.text + if isShadowingAnyScope(identifierText) { + violations.append(identifier.identifier.positionAfterSkippingLeadingTrivia) + } + } else if let tuple = pattern.as(TuplePatternSyntax.self) { + tuple.elements.forEach { element in + checkForBindingShadowing(in: element.pattern) + } + } else if let valueBinding = pattern.as(ValueBindingPatternSyntax.self) { + checkForBindingShadowing(in: valueBinding.pattern) + } + } + + private func isShadowingOuterScope(_ identifier: String) -> Bool { + guard scope.count > 1 else { return false } + + for scopeDeclarations in scope.dropLast() where + scopeDeclarations.contains(where: { $0.declares(id: identifier) }) { + return true + } + return false + } + + /// Checks all scope levels including the current one. Used for parameter checking + /// since parameters are declared into a child scope, not the current one. + private func isShadowingAnyScope(_ identifier: String) -> Bool { + scope.contains { $0.contains { $0.declares(id: identifier) } } + } + } +} diff --git a/Source/SwiftLintBuiltInRules/Rules/RuleConfigurations/VariableShadowingConfiguration.swift b/Source/SwiftLintBuiltInRules/Rules/RuleConfigurations/VariableShadowingConfiguration.swift new file mode 100644 index 0000000000..d77c73628f --- /dev/null +++ b/Source/SwiftLintBuiltInRules/Rules/RuleConfigurations/VariableShadowingConfiguration.swift @@ -0,0 +1,9 @@ +import SwiftLintCore + +@AutoConfigParser +struct VariableShadowingConfiguration: SeverityBasedRuleConfiguration { + @ConfigurationElement(key: "severity") + private(set) var severityConfiguration = SeverityConfiguration(.warning) + @ConfigurationElement(key: "ignore_parameters") + private(set) var ignoreParameters = true +} diff --git a/Tests/BuiltInRulesTests/VariableShadowingRuleTests.swift b/Tests/BuiltInRulesTests/VariableShadowingRuleTests.swift new file mode 100644 index 0000000000..4b01bb5ce1 --- /dev/null +++ b/Tests/BuiltInRulesTests/VariableShadowingRuleTests.swift @@ -0,0 +1,15 @@ +@testable import SwiftLintBuiltInRules +import TestHelpers +import XCTest + +final class VariableShadowingRuleTests: SwiftLintTestCase { + func testWithIgnoreParametersTrue() { + let configuration = ["ignore_parameters": true] + verifyRule(VariableShadowingRule.description, ruleConfiguration: configuration) + } + + func testWithIgnoreParametersFalse() { + let configuration = ["ignore_parameters": false] + verifyRule(VariableShadowingRule.description, ruleConfiguration: configuration) + } +} diff --git a/Tests/GeneratedTests/GeneratedTests_10.swift b/Tests/GeneratedTests/GeneratedTests_10.swift index 46bb104908..3794c0e5d5 100644 --- a/Tests/GeneratedTests/GeneratedTests_10.swift +++ b/Tests/GeneratedTests/GeneratedTests_10.swift @@ -85,6 +85,12 @@ final class ValidIBInspectableRuleGeneratedTests: SwiftLintTestCase { } } +final class VariableShadowingRuleGeneratedTests: SwiftLintTestCase { + func testWithDefaultConfiguration() { + verifyRule(VariableShadowingRule.description) + } +} + final class VerticalParameterAlignmentOnCallRuleGeneratedTests: SwiftLintTestCase { func testWithDefaultConfiguration() { verifyRule(VerticalParameterAlignmentOnCallRule.description) @@ -150,9 +156,3 @@ final class XCTSpecificMatcherRuleGeneratedTests: SwiftLintTestCase { verifyRule(XCTSpecificMatcherRule.description) } } - -final class YodaConditionRuleGeneratedTests: SwiftLintTestCase { - func testWithDefaultConfiguration() { - verifyRule(YodaConditionRule.description) - } -} diff --git a/Tests/GeneratedTests/GeneratedTests_11.swift b/Tests/GeneratedTests/GeneratedTests_11.swift new file mode 100644 index 0000000000..758f00504c --- /dev/null +++ b/Tests/GeneratedTests/GeneratedTests_11.swift @@ -0,0 +1,14 @@ +// GENERATED FILE. DO NOT EDIT! + +// swiftlint:disable:next blanket_disable_command superfluous_disable_command +// swiftlint:disable single_test_class type_name + +@testable import SwiftLintBuiltInRules +@testable import SwiftLintCore +import TestHelpers + +final class YodaConditionRuleGeneratedTests: SwiftLintTestCase { + func testWithDefaultConfiguration() { + verifyRule(YodaConditionRule.description) + } +} diff --git a/Tests/IntegrationTests/Resources/default_rule_configurations.yml b/Tests/IntegrationTests/Resources/default_rule_configurations.yml index fb30c62bd2..f6d2689c85 100644 --- a/Tests/IntegrationTests/Resources/default_rule_configurations.yml +++ b/Tests/IntegrationTests/Resources/default_rule_configurations.yml @@ -1383,6 +1383,12 @@ valid_ibinspectable: meta: opt-in: false correctable: false +variable_shadowing: + severity: warning + ignore_parameters: true + meta: + opt-in: false + correctable: false vertical_parameter_alignment: severity: warning meta: diff --git a/Tests/generated_tests.bzl b/Tests/generated_tests.bzl index 689f1a5310..e82cc1832d 100644 --- a/Tests/generated_tests.bzl +++ b/Tests/generated_tests.bzl @@ -14,7 +14,8 @@ GENERATED_TEST_TARGETS = [ "//Tests:GeneratedTests_07", "//Tests:GeneratedTests_08", "//Tests:GeneratedTests_09", - "//Tests:GeneratedTests_10" + "//Tests:GeneratedTests_10", + "//Tests:GeneratedTests_11" ] def generated_tests(): @@ -29,6 +30,7 @@ def generated_tests(): generated_test_shard("08") generated_test_shard("09") generated_test_shard("10") + generated_test_shard("11") native.test_suite( name = "GeneratedTests",