unused_closure_parameter autocorrect is too eager

[allen-zeng]

When autocorrecting unused_closure_parameter, swiftlint also removes the type annotation. This isn't going to work in a situation where the type annotation is necessary, for example, when generics needs to use it for type inference. That is, the autocorrection will make the code fail to compile.

func genericsFunc<T>(closure: (T) -> Void) {
    print("\(T.self)")
}

genericsFunc(closure: { (int: Int) -> Void in
    // this will print "Int"
})

Autocorrect replaces the entire parameter list with an underscore, which isn't valid syntax. It should instead retain the type, and replace the name with an underscore:

genericsFunc(closure: { (_) -> Void in
    // autocorrected version, it's wrong!
})

genericsFunc(closure: { (_: Int) -> Void in
    // this will compile and still print "Int"
})

[marcelofabri]

This is kind of hard, because if we always retain the type, I'd say in at least 95% of the cases it could be removed. On the other hand, autocorrect shouldn't break any code 🙃

[allen-zeng]

Does swiftlint actually work out whether the closure parameter types can be removed? If it doesn't, the correct autocorrection behaviour is to have leave the type annotation. If it's not even supplied then that's a different story.

func plainFunction(closure: (Int) -> Void) {
    // stuff
}

plainFunction { (param: Int) -> Void in // this will be corrected to { (_: Int) -> Void in }
    // stuff
}

plainFunction { param -> Void in // this will be corrected to { _ -> Void in }
    // stuff
}

[marcelofabri]

@allen-zeng Do you want to try to submit a PR?

[allen-zeng]

Sure! I'll see what I can do :)

[marcelofabri]

I took a look at this today and the problem is that key.nameoffset and key.namelength are 0 in this case:

{
	"key.nameoffset": 0,
	"key.typename": "Int",
	"key.length": 8,
	"key.name": "int",
	"key.kind": "source.lang.swift.decl.var.parameter",
	"key.namelength": 0,
	"key.offset": 25
}

We could however check if there's a key.typename and avoid correcting it in this case.

[allen-zeng]

Yeah I noticed that and thought it was weird. I'm guessing this is a bug in SourceKitten?

I've just got around making the changes, while all the tests pass in Xcode, they don't when I run script/cibuild. I'll investigate why

[jpsim]

If there's a bug here, it's likely in SourceKit rather than SourceKitten, which acts mostly just as a proxy in this situation.

I look forward to seeing your patch @allen-zeng!

[allen-zeng]

Pull request created :)

I do have a problem when running make docker_test, the output:

docker run -v `pwd`:/SwiftLint norionomura/sourcekit:302 bash -c "cd /SwiftLint && swift test"
Compile CYaml src/api.c
Compile CYaml src/dumper.c
Compile CYaml src/emitter.c
Compile CYaml src/loader.c
Compile CYaml src/parser.c
Compile CYaml src/reader.c
Compile CYaml src/scanner.c
Compile CYaml src/writer.c
Compile Swift Module 'SwiftyTextTable' (1 sources)
Compile Swift Module 'Yaml' (6 sources)
Compile Swift Module 'SWXMLHash' (2 sources)
Compile Swift Module 'Result' (2 sources)
Linking CYaml
Compile Swift Module 'Commandant' (9 sources)
Compile Swift Module 'Yams' (2 sources)
Compile Swift Module 'SourceKittenFramework' (33 sources)
Compile Swift Module 'sourcekitten' (10 sources)
Compile Swift Module 'SwiftLintFramework' (144 sources)
Linking ./.build/debug/sourcekitten
Compile Swift Module 'swiftlint' (10 sources)
Compile Swift Module 'SwiftLintFrameworkTests' (27 sources)
Linking ./.build/debug/swiftlint
Linking ./.build/debug/SwiftLintPackageTests.xctest
/usr/bin/ld.gold: fatal error: /SwiftLint/.build/debug/SwiftLintPackageTests.xctest: open: Is a directory
clang: error: linker command failed with exit code 1 (use -v to see invocation)
<unknown>:0: error: link command failed with exit code 1 (use -v to see invocation)
<unknown>:0: error: build had 1 command failures
swift-test: error: exit(1): /usr/bin/swift-build-tool -f /SwiftLint/.build/debug.yaml test
make: *** [docker_test] Error 1

I'm not entirely sure what's going on there, do you have the same problem @marcelofabri ?

[marcelofabri]

Try deleting .build

[allen-zeng]

Yep that worked!

In that case it's probably better to update Makefile so that docker_test deletes the .build folder before it runs the docker command. Does that sound reasonable?

[marcelofabri]

Probably make clean should delete it.

[allen-zeng]

I'll just add clean to docker_test then

[marcelofabri]

Actually there's a spm_clean that may remove this folder

[allen-zeng]

@marcelofabri I've created this pull request to run clean before docker test, can you take a look as well?

[marcelofabri]

Closed in #1247