Add DuplicateImportsRule
#2004
Add DuplicateImportsRule
#2004
Conversation
|
Hi @sammy-SC. I don't know useful my suggestion would be for the implementation. But an easy way to find dups is using a |
|
Hello @ornithocoder I was considering that approach however I couldn't find a way to deal with nested modules. and this should throw a warning on first line as that module has already been imported. Hope it makes sense and explains why I chose to go with comparing lines among themselves with two nested for loops. |
Codecov Report
@@ Coverage Diff @@
## master #2004 +/- ##
==========================================
+ Coverage 91.92% 91.97% +0.04%
==========================================
Files 307 309 +2
Lines 15333 15452 +119
==========================================
+ Hits 14095 14212 +117
- Misses 1238 1240 +2
Continue to review full report at Codecov.
|
|
@ornithocoder I don't know how to reliably detect this. @jpsim @marcelofabri how would you go about detecting Also to me it doesn't look like valid use case of Thank you |
| kind: .style, | ||
| nonTriggeringExamples: [ | ||
| "import A\nimport B\nimport C", | ||
| "import A.B\nimport import A.C" |
There was a problem hiding this comment.
There's a double import for A.C.
There was a problem hiding this comment.
@ornithocoder thank you, I've fixed that.
|
Hi @sammy-SC. Regarding the {
"key.diagnostic_stage" : "source.diagnostic.stage.swift.parse",
"key.substructure" : [
],
"key.offset" : 0,
"key.length" : 67
}But "shows" on the syntax map. [
{
"type" : "source.lang.swift.syntaxtype.buildconfig.keyword",
"offset" : 0,
"length" : 3
},
{
"type" : "source.lang.swift.syntaxtype.buildconfig.id",
"offset" : 4,
"length" : 5
},
{
"type" : "source.lang.swift.syntaxtype.attribute.builtin",
"offset" : 14,
"length" : 9
},
{
"type" : "source.lang.swift.syntaxtype.keyword",
"offset" : 24,
"length" : 6
},
{
"type" : "source.lang.swift.syntaxtype.identifier",
"offset" : 31,
"length" : 5
},
{
"type" : "source.lang.swift.syntaxtype.buildconfig.keyword",
"offset" : 37,
"length" : 5
},
{
"type" : "source.lang.swift.syntaxtype.keyword",
"offset" : 47,
"length" : 6
},
{
"type" : "source.lang.swift.syntaxtype.identifier",
"offset" : 54,
"length" : 5
},
{
"type" : "source.lang.swift.syntaxtype.buildconfig.keyword",
"offset" : 60,
"length" : 6
}
]But honestly, I don't know what to do in this situation. Probably ignore duplicates if they're wrapped by flags? Specially if the |
|
Hello @ornithocoder I've done what you suggested, ignoring imports in conditional compilation directives. |
|
I'm curious on why https://github.com/apple/swift/blob/d01c9fa136bb35b801acb948ab2ee4f1d6cb3e9c/stdlib/public/Platform/msvcrt.swift#L14:1 triggered the warning 🤔 |
sammy-SC
force-pushed
the
new-rule/duplicate-imports
branch
2 times, most recently
from
b1099b7
to
d864afa
Compare
|
@marcelofabri I'm sorry I must have missed that. |
|
Hmm, apparently this rule made linting way slower on oss-check:
I'm re-running it just to be sure it wasn't caused by a Circle CI instability. |
|
@marcelofabri are the new results sufficient or should I look into improving performance? |
|
@sammy-SC I think it's fine now. I'll try to review this in the following days. |
|
@marcelofabri could you please check this PR when you have time? I think it would be a useful addition to SwiftLint. |
|
@sammy-SC Sorry for the delay. I think we should make sure we're handling all the import forms, including the less-known import kind (https://developer.apple.com/library/content/documentation/Swift/Conceptual/Swift_Programming_Language/Declarations.html#//apple_ref/doc/uid/TP40014097-CH34-ID354). Or we can skip that for now and make the rule opt-in. |
| import Foundation | ||
| import SourceKittenFramework | ||
|
|
||
| extension Line { |
There was a problem hiding this comment.
this extension should be private (and I would move it to the end of this file)
| identifier: "duplicate_imports", | ||
| name: "Duplicate Imports", | ||
| description: "Imports should be unique.", | ||
| kind: .style, |
| guard let kind = SyntaxKind(rawValue: token.type) else { | ||
| return false | ||
| } | ||
| return kind == SyntaxKind.buildconfigKeyword |
There was a problem hiding this comment.
you can use just kind == .buildconfigKeyword
| @@ -5,6 +5,7 @@ | |||
| // Created by Scott Hoyt on 12/28/15. | |||
| // Copyright © 2015 Realm. All rights reserved. | |||
| // | |||
|
|
|||
There was a problem hiding this comment.
🤔
Did you manually add this line?
There was a problem hiding this comment.
I'm sorry, I removed the line
sammy-SC
force-pushed
the
new-rule/duplicate-imports
branch
2 times, most recently
from
c7e1c8c
to
f96068b
Compare
|
@marcelofabri Please when you have a time could you take another look at this? Thank you |
|
@marcelofabri could you please rerun the speed check? |
sammy-SC
force-pushed
the
new-rule/duplicate-imports
branch
2 times, most recently
from
afd2bbb
to
761f75e
Compare
|
@marcelofabri I resolved merge conflicts. |
|
@sammy-SC I ran oss-check locally and it seems it's not too bad: |
|
@marcelofabri please let me know if there is anything else you would like me to change. Any tips on improving the code are very welcome, even if it was to improve readability / clarify of the solution. If approved I would be more than happy to add autocorrection. |
|
OSSCheck highlighted a number of false positives. For example, import Realm
import Realm.Private |
|
@jpsim that's intentional, is that not correct? The idea was that if you have imports such as would get highlighted as duplicate import. Is that not desired? If not, I'm happy to change it. |
|
However, I think this is fairly rare in practice that the false positive may be worth the ability to find duplicate Apple module imports, which as you point out are usually re-exported by parents. |
jpsim
force-pushed
the
new-rule/duplicate-imports
branch
from
761f75e
to
6b4bb80
Compare
|
Thank you for being so patient on this one @sammy-SC! I finally had some time to update this, refactor it a little and came to a decision on keeping the rule more powerful but with a few rare false positives. |
|
@jpsim thanks for the help. I will check out the refactoring. It took a little over a year so not too bad :) |
implements #1881
Any recommendations for improvement are very welcome.
if you are happy with implementation, I will be happy to get started on autocorrection.