Skip to content
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

False positive in UnnecessarySafeCall (1.11.0-RC2) #2961

Closed
vladimirfx opened this issue Aug 11, 2020 · 8 comments
Closed

False positive in UnnecessarySafeCall (1.11.0-RC2) #2961

vladimirfx opened this issue Aug 11, 2020 · 8 comments

Comments

@vladimirfx
Copy link

Observed Behavior

Issue reported for necessary safe call:

val linkedDevices: Map<Device, DeviceLink>
...
linkedDevices[ownerDevice]?.isOwned = true

Your Environment

  • Version of detekt used: 1.11.0-RC2
  • Version of Gradle used (if applicable): 6.6
  • Operating System and version: MacOS 10.15.5
@Julesssss
Copy link

Julesssss commented Aug 12, 2020

Possibly seeing a similar false positive UnnecessarySafeCall results in older versions too.

A)

class ExampleViewHolder(itemView: View) : RecyclerView.ViewHolder(itemView) {
    val textExample: TextView? = itemView.findViewById(R.id.textExample)
}
// incorrectly flagged as unnecessary
viewHolder.textExample?.typeface = getTypeface()

B)

class ListItem(val data: Map<String, Any>): BaseListItem {
    
    val optionalValue: String?
        get() = (data["optionalValue"] as? String)

}
// incorrectly flagged as unnecessary
listItem.optionalValue?.let {
 ...
}

In this case, there is a workaround. This passes:

val optionalValue: String? = listItem.optionalValue
optionalValue?.let {
    ...
}

Your environment

  • Version of detekt used: 1.11.0
  • Version of Gradle used (if applicable): 5.4.1
  • Operating System and version: MacOS 10.15.5

@schalkms
Copy link
Member

Thanks for listing and tracking further cases here.

@cortinico
Copy link
Member

Hey @vladimirfx @Julesssss
I've tested all of your examples removing the types that are not known (or that are Android specifics) but trying to maintain the patterns:

OP Example: https://pl.kotl.in/-VLlrSCsk
Example A: https://pl.kotl.in/ydew3b-Fc
Example B: https://pl.kotl.in/OGo8X_wPe

For all of them, I was not able to reproduce the issue.
Moreover, this specific rule is relying only on an inspection of the Kotlin compiler: UNNECESSARY_SAFE_CALL. There is no extra logic added.

Are you also getting warnings from the Kotlin compiler for those examples?
Can I ask you to try with the snippets I linked if the issue is still reported?

@Julesssss
Copy link

Julesssss commented Aug 15, 2020 via email

@vladimirfx
Copy link
Author

  1. Bug present in type resolution mode (detektMain)

  2. Detekt task in the beginning outputs:

    warning: classpath entry points to a non-existent location: /Users/user/.gradle/caches/modules-2/files-2.1/com.github.spotbugs/spotbugs-annotations/4.1.1/f2c06572ee999f374c0849d696305d203bf9c3e/spotbugs-annotations-4.1.1.jar;/Users/user/.gradle/caches/modules-2/files-2.1/com.google.guava/guava/26.0-jre/6a806eff209f36f635f943e16d97491f00f6bfab/guava-26.0-jre.jar;...

all classpath delimeted by ';' (shouldn't it be ':' in Unix system?).

Afterward 10k lines with errors such:

//Users/vladimirfx/dev/projects/fcb/mfc/mfc-backend/src/main/kotlin/internal/user/webservice/UserProfile.kt:9:18: error: cannot access built-in declaration 'kotlin.String'. Ensure that you have a dependency on the Kotlin standard library
val surName: String,
             ^

Detekt 1.10 works correctly in same project. Gradle downgrade is not helps.

@vladimirfx
Copy link
Author

vladimirfx commented Aug 17, 2020

Looks like bug is there:

fileCollection.joinToString(";") { it.absolutePath }) else emptyList()

IMO it should be:

java.io.File.pathSeparatorChar

instead of hardcoded ';'

@arturbosch
Copy link
Member

arturbosch commented Aug 17, 2020

Looks like bug is there:

fileCollection.joinToString(";") { it.absolutePath }) else emptyList()

IMO it should be:

java.io.File.pathSeparatorChar

instead of hardcoded ';'

Thanks for investigating this!
This line was not the problem but a good hint for the handling of the separators in core. I've created a PR to fix this regression for the next bugfix release.

arturbosch added a commit that referenced this issue Aug 18, 2020
* Fix regression separating classpath entries - #2961

* Use File.pathSeparaterChar to join classpath

* Swap separator chars in comment
@arturbosch
Copy link
Member

We had a regression in 1.11.0, please update to 1.11.1, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants