Fix prefer_key_path to catch closures nested in macro expansions - #6906
Open
jadhavgaurav wants to merge 1 commit into
Open
Fix prefer_key_path to catch closures nested in macro expansions#6906jadhavgaurav wants to merge 1 commit into
jadhavgaurav wants to merge 1 commit into
Conversation
isInvalid(restrictToStandardFunctions:) only checked a closure's
immediate parent for MacroExpansionExprSyntax, so a closure nested one
level deeper, such as the where: argument of items.first(where:) inside
#require(...), was still rewritten to a key path. The key path form can
fail to compile inside #require/#expect, which decompose the call and
lose its non-throwing guarantee.
Replace the immediate-parent check with a full ancestor walk that looks
for a macro expansion at any depth, matching the style already used for
ancestor walks elsewhere in the rule set (see
AccessibilityLabelForImageRule.isInsideLabelIconClosure()). This also
subsumes the existing direct-argument case, so #Predicate { $0.a } keeps
working the same way.
Fixes realm#6657
Generated by 🚫 Danger |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
prefer_key_path'sisInvalid(restrictToStandardFunctions:)only checked aclosure's immediate parent for
MacroExpansionExprSyntax. That correctlysilences a closure that is a direct macro argument, like
#Predicate { $0.a },but a closure nested one or more levels deeper inside a macro expansion's
arguments was still flagged and rewritten:
swiftlint --fixrewrote this toitems.first(where: \.flag), which fails tocompile inside
#require/#expect: those macros decompose the call via__checkFunctionCall, and the key path form loses the non-throwing guaranteethe original closure had.
#5765already silenced the direct-argument case (#Predicate { $0.a }), butthe check only inspected
parent.kind, one level up from the closure.Fix
Replace the single-level check with a full ancestor walk that looks for a
MacroExpansionExprSyntaxorMacroExpansionDeclSyntaxat any depth, in thesame style already used elsewhere in the rule set for ancestor walks (see
AccessibilityLabelForImageRule.isInsideLabelIconClosure()). This subsumes theexisting direct-argument case, so
#Predicate { $0.a }keeps working exactlyas before, and additionally catches the deeper nesting from the issue.
Testing
Added
closureNestedInMacroExpansionIsNotRewritten()toPreferKeyPathRuleTests, covering:#require(f.first(where: { $0.a }))case from the issue,#expect(...),I could not run
swift teston this machine (only Xcode Command Line Toolsare installed here, no full Xcode, so the
swift-testingruntime the testtargets link against is unavailable, and
SourceKittenFrameworkcannot dlopensourcekitdInProc). To still prove the regression test actually exercises thefix, I added a small local executable target that links
SwiftLintCoreandSwiftLintFrameworkdirectly (bypassing the sourcekitd-dependent CLI path,since
prefer_key_pathis aSourceKitFreeRule) and ran the exact cases fromthe new test through
Linter/RuleStorageby hand:existing non-macro case (
f.first(where: { $0.a })) still reports 1, andthe direct-argument case (
#Predicate { $0.a }) still reports 0.Source/.../PreferKeyPathRule.swiftreverted (via a saved patch,not
git stash) and the test file left in place: the three nested caseseach report 1 violation, i.e. the regression reproduces and the new test
would fail. Reapplying the patch restores the passing state.
I also ran
swift build(full product) andswift build --target BuiltInRulesTestssuccessfully, and self-linted the two changed source fileswith the locally built
swiftlintbinary (0 violations, a few SourceKit-onlyrules skipped because of the same missing-sourcekitd limitation).
Fixes #6657