-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Fix: optional_data_string_conversion should detect String.init and leading-dot .init usages #6372
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Fix: optional_data_string_conversion should detect String.init and leading-dot .init usages #6372
Conversation
Generated by 🚫 Danger |
SimplyDanny
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for preparing the PR! Please consider my comments.
| expr.declName.baseName.description == "self" { | ||
| violations.append(node.positionAfterSkippingLeadingTrivia) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should rather implement visitPost(_ node: FunctionCallExprSyntax) instead. It can check all cases at the same time. If the called name is either String, String.init or .init and the arguments match, it might trigger.
The var x: String = .init(...) case is very special and I don't expect it to appear often. While String.init is safe, .init could appear everywhere (think of f(.init(...)). We might add an option for that (as I've already pointed out in #6359.
Tests/BuiltInRulesTests/OptionalDataStringConversionRuleTests.swift
Outdated
Show resolved
Hide resolved
| Example("String(bytes: data, encoding: .utf8)"), | ||
| Example("String(UTF8.self)"), | ||
| Example("String(a, b, c, UTF8.self)"), | ||
| Example("String(decoding: data, encoding: UTF8.self)"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add more cases that shouldn't trigger, e.g. let text: Int = .init(decoding: data, as: UTF8.self). 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm missing this example. Please add it.
804ec4c to
4cfbd90
Compare
|
|
||
| ### Bug Fixes | ||
|
|
||
| * optional_data_string_conversion: Detect `String.init(...)` and leading-dot `.init(...)` usages when converting `Data` to `String`. This fixes cases such as `String.init(decoding: data, as: UTF8.self)` and `let text: String = .init(decoding: data, as: UTF8.self)` which previously did not trigger the rule. (Fixes https://github.com/realm/SwiftLint/issues/6359) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add this entry to the current "Main" section at the top of the file.
| * optional_data_string_conversion: Detect `String.init(...)` and leading-dot `.init(...)` usages when converting `Data` to `String`. This fixes cases such as `String.init(decoding: data, as: UTF8.self)` and `let text: String = .init(decoding: data, as: UTF8.self)` which previously did not trigger the rule. (Fixes https://github.com/realm/SwiftLint/issues/6359) | ||
| [Nadeem Ali](https://github.com/nadeemnali) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * optional_data_string_conversion: Detect `String.init(...)` and leading-dot `.init(...)` usages when converting `Data` to `String`. This fixes cases such as `String.init(decoding: data, as: UTF8.self)` and `let text: String = .init(decoding: data, as: UTF8.self)` which previously did not trigger the rule. (Fixes https://github.com/realm/SwiftLint/issues/6359) | |
| [Nadeem Ali](https://github.com/nadeemnali) | |
| * Add detection of cases such as `String.init(decoding: data, as: UTF8.self)` and | |
| `let text: String = .init(decoding: data, as: UTF8.self)` to `optional_data_string_conversion` rule. | |
| [Nadeem Ali](https://github.com/nadeemnali) | |
| [#6359](https://github.com/realm/SwiftLint/issues/6359) |
| Example("String(bytes: data, encoding: .utf8)"), | ||
| Example("String(UTF8.self)"), | ||
| Example("String(a, b, c, UTF8.self)"), | ||
| Example("String(decoding: data, encoding: UTF8.self)"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm missing this example. Please add it.
| guard node.arguments.map(\.label?.text) == ["decoding", "as"] else { return } | ||
|
|
||
| // Check that the `as:` argument is `UTF8.self` | ||
| func isUTF8Self(on call: FunctionCallExprSyntax) -> Bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this function? It's only called once, isn't it?
| } | ||
|
|
||
| // Case 2 and 3: `.init` or `String.init` | ||
| if let member = called.as(MemberAccessExprSyntax.self), member.declName.baseName.text == "init" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be a guard.
| if member.base == nil { | ||
| // Walk ancestors to find a VariableDecl or PatternBinding with a type annotation of `String`. | ||
| var parent: Syntax? = node.parent | ||
| while let ancestor = parent { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to not walk up the tree arbitrarily high. node.parent.parent gives you the binding to check for the String type.
Summary
The optional_data_string_conversion SwiftSyntax rule only detected direct calls like:
but it missed two common variants:
String.init(decoding: data, as: UTF8.self)let text: String = .init(decoding: data, as: UTF8.self)This change updates the rule to detect both
String.init(...)and leading-dot.init(...)when the contextual type isString. It also adds examples to the rule description and a unit test that verifies the rule's description examples.Changes
String(...)calls,String.init(...)(MemberAccessExpr with baseStringand memberinit),.init(...)when assigned to a variable with an explicitStringtype annotation.Why
This fixes the bug reported where
let text: String = .init(decoding: data, as: UTF8.self)did not produce the expected lint violation.Test plan