Fix explicit_type_interface when used in statements
#2244
Conversation
dirtydanee
changed the title
explicit_type_interface when used in statements or applying wea…explicit_type_interface when used in statements
dirtydanee
force-pushed
the
dm-bugfix-case-let-in-explicit_type_interface
branch
3 times, most recently
from
41a1d81
to
09abd6c
Compare
Codecov Report
@@ Coverage Diff @@
## master #2244 +/- ##
==========================================
+ Coverage 91.89% 91.96% +0.07%
==========================================
Files 304 304
Lines 15254 15419 +165
==========================================
+ Hits 14017 14180 +163
- Misses 1237 1239 +2
Continue to review full report at Codecov.
|
dirtydanee
force-pushed
the
dm-bugfix-case-let-in-explicit_type_interface
branch
2 times, most recently
from
f37a6cd
to
0244e6b
Compare
| } | ||
|
|
||
| var containsWeakSelf: Bool { | ||
| guard let name = name, name == "self" else { |
There was a problem hiding this comment.
you could replace this to guard name == "self" else
| @@ -95,6 +95,10 @@ | |||
|
|
|||
| #### Bug Fixes | |||
|
|
|||
| * Fix `explicit_type_interface` when used in statements. | |||
There was a problem hiding this comment.
this is in the wrong section, can you update this before merging?
| !containsType(dictionary: dictionary), | ||
| let offset = dictionary.offset else { | ||
| !dictionary.containsType, | ||
| !dictionary.containsWeakSelf, |
There was a problem hiding this comment.
shouldn't we try to check if there's a capture group, no matter if it's weak or if it's capturing self?
There was a problem hiding this comment.
Yeah, that would make sense. Thanks for pointing it out!
This case is still failing:
"class Foo {\n" +
" func bar() {\n" +
" var k = 0\n" +
" _ = { [weak self, weak k] in\n" +
" guard let strongSelf = self else { return }\n" +
" }\n" +
" }\n" +
"}",
I am not sure how could I find the capture group, my first guess would be using a regex, something like this:
=\s+{\s*\[((\s)*\w+\s*\w+(,)*)+]\s+in
Do you have maybe an other idea how to approach finding the capture group?
There was a problem hiding this comment.
Added a solution for searching capture groups with regex.
dirtydanee
force-pushed
the
dm-bugfix-case-let-in-explicit_type_interface
branch
5 times, most recently
from
57070ed
to
068259f
Compare
| verifyRule(description) | ||
| } | ||
|
|
||
| func testWeakSelf() { |
There was a problem hiding this comment.
this is not just testing weak self anymore
| let offset = dictionary.offset else { | ||
| !dictionary.containsType, | ||
| let offset = dictionary.offset, | ||
| !file.captureGroupByteRanges.contains(where: { $0.contains(offset) }), |
There was a problem hiding this comment.
This will perform the search for the whole file every time 😱
This is an opt-in rule, so having a worse performance is acceptable, but I'd be awesome if we could think of a way to make it faster.
Performing the regex match just on the parent type would be an option, but it'd have to change a few things. Other option would be performing the whole search first and the navigate the AST manually instead of using ASTRule.
There was a problem hiding this comment.
I choose the later one, thanks for the suggestion. Here is a screenshot of the listing speed from before the change:
dirtydanee
force-pushed
the
dm-bugfix-case-let-in-explicit_type_interface
branch
from
c402e8c
to
6d5e2e4
Compare
dirtydanee
force-pushed
the
dm-bugfix-case-let-in-explicit_type_interface
branch
from
6d5e2e4
to
f7900b3
Compare
|
@marcelofabri I have pushed changes fixing the false negatives as you pointed out on #2321 and rebased to current master.
EDIT: I still see issues with declarations in |
dirtydanee
force-pushed
the
dm-bugfix-case-let-in-explicit_type_interface
branch
2 times, most recently
from
df340da
to
15e8cab
Compare
dirtydanee
force-pushed
the
dm-bugfix-case-let-in-explicit_type_interface
branch
from
15e8cab
to
ccc8fab
Compare
|
In my last commit 31ec32b I added a Swift version runtime guard for the Given this simple example: func foo() {
switch foo {
case var (x, y): ↓let fooBar = 1
}
}The issue seems for the simple example below I think the rule could be helpful for earlier Swift versions, so I would keep its |
|
Is this PR still considered? |
|
@dirtydanee would you mind opening a new PR with this change? I'd be happy to start a review on it again, but I'd rather start fresh. |
|
Fantastic, thank you @dirtydanee. Let's close this and continue there. |
Fix #2154