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

CastNullableToNonNullableType false positive on platform types #6146

Closed
TWiStErRob opened this issue May 25, 2023 · 2 comments · Fixed by #6149
Closed

CastNullableToNonNullableType false positive on platform types #6146

TWiStErRob opened this issue May 25, 2023 · 2 comments · Fixed by #6149
Labels
bug good first issue Issue that is easy to pickup for people that are new to the project help wanted

Comments

@TWiStErRob
Copy link
Member

TWiStErRob commented May 25, 2023

Observed Behavior

image
as GradleInternal is reported

Expected Behavior

No report, because Project.gradle is defined as:
image
Notice that there's no nullability annotation, only javadoc, but there's a package-level annotation:

@org.gradle.api.NonNullApi
package org.gradle.api;

Looks like this might be a type resolution issue, because the IDE infers non-null:
image

Anyway, the problem exists on all platform types, even when there's no annotation.

Steps to Reproduce

Cast any function or property result which has platform type. For example

class C {
    static CharSequence cs() {
        return "";
    }
}
fun f() {
    val cs = C.cs()
    val str = cs as String // CastNullableToNonNullableType
}

Use separate null assertion and type cast like ('(cs ?: error("null assertion message")) as String') instead of 'cs as String'. [CastNullableToNonNullableType]

Context

When the inferred type is platform type because there's no nullability defined in Java APIs, by definition we're free to interpret that as nullable or non-nullable. So these should be ignored, or at least ignorable by a flag (and I would vote for that flag to be turned on by default to avoid lots of false positives).

Your Environment

  • Version of detekt used: 1.23.0
  • Version of Gradle used (if applicable): 8.1.1
  • Gradle scan link (add --scan option when running the gradle task): N/A
  • Operating System and version: all
  • Link to your project (if it's a public repository): not needed, full repro present

cc @atulgpt as author

@TWiStErRob TWiStErRob added the bug label May 25, 2023
@TWiStErRob
Copy link
Member Author

TWiStErRob commented May 25, 2023

Another case: NumberFormat.getInstance() as DecimalFormat and if I add !!:

NumberFormat.getInstance()!! as DecimalFormat

I get this compiler warning:

[UNNECESSARY_NOT_NULL_ASSERTION] Unnecessary non-null assertion (!!) on a non-null receiver of type NumberFormat!

so Kotlin knows it's non-null somehow?


An interesting study: this might be also a false negative in some cases, if we agree that T! should always be treated as T?. Think about these functions:

fun NumberFormat.asDecimal(): DecimalFormat =
	this as DecimalFormat
fun asDecimal(nf: NumberFormat): DecimalFormat =
	nf as DecimalFormat

It's strictly non-nullable on all fronts and throws ClassCastException when it's the wrong type, however when used on a platform type (i.e. NumberFormat.getInstance().asDecimal() or asDecimal(NumberFormat.getInstance())), it creates an implicit non-null cast. The generated bytecode is literally asDecimal(NumberFormat.getInstance()!!) in both cases. For the equivalent NumberFormat? declarations the null check is of course not present.

So calling a function with clear nullability on a platform type is like opening Schrodinger's box, it manifests itself as nullable/non-nullable as per the receiver/parameter type.

@cortinico cortinico added help wanted good first issue Issue that is easy to pickup for people that are new to the project labels May 25, 2023
@atulgpt
Copy link
Contributor

atulgpt commented May 25, 2023

Somehow I have this TC

@Test
fun `reports casting of platform type to NonNullable type`() {
    val code = """
        class Foo {
            fun test() {
                // getSimpleName() is not annotated with nullability information in the JDK, so compiler treats
                // it as a platform type with unknown nullability.
                val y = javaClass.simpleName as String
            }
        }
    """.trimIndent()
    val findings = subject.compileAndLintWithContext(env, code)
    assertThat(findings).hasSize(1)
    assertThat(findings).hasStartSourceLocation(5, 38)
    assertThat(findings[0]).hasMessage(
        "Use separate `null` assertion and type cast like " +
            "('(javaClass.simpleName ?: error(\"null assertion message\")) as String') instead of " +
            "'javaClass.simpleName as String'."
    )
}

But I agree this rule should not assume the nullability of platform type as that handled by different rule

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug good first issue Issue that is easy to pickup for people that are new to the project help wanted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants