spotbugs / sonar-findbugs

SpotBugs plugin for SonarQube
351 stars 135 forks source link

[Kotlin] Bogus errors NP_NONNULL_FIELD_NOT_INITIALIZED_IN_CONSTRUCTOR #1036

Open rupert-jung-mw opened 2 weeks ago

rupert-jung-mw commented 2 weeks ago

Issue Description

When analyzing Kotlin dataclasses, I get one "critical" error for each non-nullable constructor parameter without a default value with the following description:

"The field is marked as non-null, but isn't written to by the constructor. The field might be initialized elsewhere during constructor, or might always be initialized before use."

This is just plain wrong as the property someValue is literally part of the constructor and has to be set there with a non-null value.

Environment

Sonar Qube 9.9 (build 65466)

Component Version
SonarQube 65466
Sonar-FindBugs 4.2.4
Maven 3.9
Kotlin 2.0.20
data class MyClass(var someValue: String);

Thank you!

gtoison commented 2 weeks ago

Thank you for reporting this issue, would you be able to share a sample compiled .class file and the corresponding source? SpotBugs works by analyzing the bytecode, not the source, so it might be that something gets lost during the Kotlin compilation.

There should be a log line starting with Findbugs plugin version: in your analyzer logs, this should tell you the plugin version you are using. Alternatively it should be visible in the SonarQube admin page for plugin, but you need admin rights to see it

rupert-jung-mw commented 2 weeks ago

Hello @gtoison,

thank you, I updated the version in the ticket.

Unfortunately I am not allowed to share files from the project publically. Here is a small reproduction:

Kotlin Source:

@Entity
@Table(name = "\"case\"") // 'case' is protected SQL keyword, so we have to put it in quotes here
data class CaseBE(

    @Id
    override val id: CASE_ID = UUID.randomUUID(),

    override var createdAt: OffsetDateTime,

    override var updatedAt: OffsetDateTime,

    var breakdownFormattedAddress: String, // <--- this is perfectly legal, value has to be specified when calling constructor

    ) : AbstractAuditedBaseBE(id, createdAt, updatedAt)

Decompiled class file

package com.xxx.cases.entity

@jakarta.persistence.Entity @jakarta.persistence.Table public final data class CaseBE public constructor(id: com.bmw.cc.csvbe.bm.cases.CASE_ID /* = java.util.UUID */ = COMPILED_CODE, createdAt: java.time.OffsetDateTime, updatedAt: java.time.OffsetDateTime, breakdownCity: kotlin.String) : com.bmw.cc.csvbe.persistence.AbstractAuditedBaseBE {
    @field:jakarta.persistence.Id public open val id: com.bmw.cc.csvbe.bm.cases.CASE_ID /* = java.util.UUID */ /* compiled code */

    public open var createdAt: java.time.OffsetDateTime /* compiled code */

    public open var updatedAt: java.time.OffsetDateTime /* compiled code */

    public final var breakdownFormattedAddress: kotlin.String /* compiled code */

    public final operator fun component1(): com.bmw.cc.csvbe.bm.cases.CASE_ID /* = java.util.UUID */ { /* compiled code */ }

    public final operator fun component2(): java.time.OffsetDateTime { /* compiled code */ }

    public final operator fun component3(): java.time.OffsetDateTime { /* compiled code */ }

    public final operator fun component4(): kotlin.String { /* compiled code */ }

    public open operator fun equals(other: kotlin.Any?): kotlin.Boolean { /* compiled code */ }

    public open fun hashCode(): kotlin.Int { /* compiled code */ }

    public open fun toString(): kotlin.String { /* compiled code */ }
}

Here's the class file: DummyBE.zip

The error looks in SonarQube like this, even the marker looks like it is on the wrong line (it says 'L1' for line number for each of the errors)

image

PS: Actually, there are more bogus errors when analyzing files compiled from Kotlin. There was another problem with 'duplicated lines' referencing to line numbers not even existing in the source (where in fact was not a single duplicated line).

I was under the impression that FindBugs is supporting Kotlin (not only Java)? If not, how can I deactivate it for Kotlin (currently this is breaking our deployment pipelines). I was trying something like

<sonar.findbugs.onlyAnalyze>**/*.java</sonar.findbugs.onlyAnalyze>

in pom.xml, but it didn't work.

PS2: The FindBugs errors are not reported in SonarLint connected to the same project, there are only visible in SonarQube for some reason)

gtoison commented 2 weeks ago

For Java the compilation process is relatively straightforward and each byte code instructions maps to a source line. Kotlin is bit different because (last time I checked) there's a transpilation step where the Kotlin source is turned into a java source and then the compiler adds things such as helper or extension methods that were not in the original Kotlin source, to support the constructs provided by the language. I don't quite recall the details but from memory there are cases when I'm not able to trace back the source line corresponding to a byte code instruction, sometimes the bytecode is from the Kotlin standard library. So when SpotBugs finds an issue it can happen that it is reported on the first line.

There's a sample Kotlin project here analysed during the integration tests. It would help immensely if you could provide me with a sample Kotlin source (with minimal dependencies) illustrating the problem. Sorry for the silly request but I don't know Kotlin!

Regarding Sonarlint, it does not apply the rules corresponding to plugins by design. I think it wouldn't be easy for Sonarsource to provide a plugin API allowing incremental analysis. This explains why you only see the issues in SonarQube.

In the end the problem is most likely with SpotBugs, since the plugin is essentially converting SpotBugs issues into SonarQube issues.

gtoison commented 2 weeks ago

Ah sorry, just saw you had linked a dummy sample .class file, let me look into that.

For the "duplicated lines" issue, I'm not sure what you might be referring to because I could not find that text in the plugin or in SpotBugs, could you please provide more details?

rupert-jung-mw commented 2 weeks ago

Here's a simple data class you could add to the sample project:

data class MyData(
    var mutableProp: String,
    val immutableProp:String,

    val immutableProp:String,
    var mutablePropWithDefault: String = "hello",

    val immutableNullableProp:String?,
    var mutableNullablePropWithDefault: String? = "hello",

    val immutableNullableProp: String?,
    var mutableNullablePropWithDefaultNull: String? = null,
)

I will also try to provoke the line duplication errors on another test branch.

rupert-jung-mw commented 2 weeks ago

Here's the error about line duplications:

image

This is how the file looks like:

package com.xxx.yyy.spring.rest.control

import io.swagger.v3.oas.models.OpenAPI
import io.swagger.v3.oas.models.Paths
import io.swagger.v3.oas.models.info.Contact
import io.swagger.v3.oas.models.info.Info
import io.swagger.v3.oas.models.security.SecurityScheme
import io.swagger.v3.oas.models.security.SecurityScheme.Type.HTTP
import io.swagger.v3.oas.models.servers.Server
import org.springdoc.core.customizers.OpenApiCustomizer
import org.springdoc.core.models.GroupedOpenApi
import org.springframework.beans.factory.annotation.Value
import org.springframework.context.annotation.Bean
import org.springframework.context.annotation.Configuration

private const val PROTOCOL_TLS: String = "https://"
private const val PROTOCOL_HTTP: String = "http://"
private const val REGION_EMEA: String = "eu-central-1"
private const val REGION_US: String = "us-east-1"
private const val AWS_CLOUD_XXX: String = "aws.cloud.xxx"
private const val DAYTONA: String = "daytona"

/**
 * Configuration of swagger. Please check app/pom.xml springdoc-openapi-maven-plugin.
 * Swagger config will be used both for exporting Swagger UI and for OpenAPI specification generation.
 */
@Configuration
class SwaggerConfig(

    @Value("\${info.app.msIdLowerCase}")
    val msIdLower: String,

    @Value("\${info.app.msIdUpperCase}")
    val msIdUpper: String,

    @Value("\${info.app.groupId}")
    val groupId: String,

    @Value("\${info.app.name}")
    val name: String,

    @Value("\${info.app.description}")
    val description: String,

    @Value("\${info.app.contact}")
    val contact: String,

    @Value("\${info.app.version}")
    val version: String,

    @Value("\${info.app.namespace}")
    val namespace: String,

    @Value("\${info.app.product}")
    val product: String,

    @Value("\${info.app.productDepotUrl}")
    val productDepotUrl: String,
) {

    val defaultExtensions get()  = mapOf(
        "x-xxx-msId" to msIdLower,
        "x-xxx-audience" to "component-internal",
        "x-xxx-2ndLevelSupportGroup" to "$namespace:global:2nd",
        "x-xxx-3rdLevelSupportGroup" to "$namespace:global:3nd"
    )

    /**
     * Swagger/OpenAPI-Config for the app itself
     * (See README.MD for test values)
     */
    @Bean
    fun appApi(): GroupedOpenApi {
        return GroupedOpenApi
            .builder()
            .group("app")
            .packagesToScan("$groupId.api.app.v1")
            .pathsToMatch("/**")
            .addOpenApiCustomizer(::infoCustomizer)
            .addOpenApiCustomizer(::tagsSorterCustomizer)
            .addOpenApiCustomizer(::serverUrlCustomizer)
            .build()
    }

    /**
     * Groups APIs from the testsupport package into one OpenAPI definition with description available under
     * [OpenAPI description URL]/testsupport.
     * Usage of GroupedOpenAPI enables to define multiple OpenAPI definitions. To add next OpenAPI definition for
     * the specified package or path, add another GroupedOpenAPI bean and another execution configuration in the
     * app/pom.xml file for springdoc-openapi-maven-plugin.
     */
    @Bean
    fun testSupportOpenApi(): GroupedOpenApi = GroupedOpenApi
        .builder()
        .group("testsupport")
        .packagesToScan(*arrayOf("$groupId.api.testsupport.v1.boundary"))
        .pathsToMatch("/**")
        .addOpenApiCustomizer(basicAuthCustomizer())
        .addOpenApiCustomizer(::tagsSorterCustomizer)
        .addOpenApiCustomizer(::testSupportOpenApiCustomizer)
        .build()

    /**
     * Defines basic authorization security scheme.
     */
    private fun basicAuthCustomizer() = OpenApiCustomizer { openAPI: OpenAPI ->
        openAPI
            .components(
                openAPI.components
                    .addSecuritySchemes(
                        "basic",
                        SecurityScheme().type(HTTP).scheme("basic")
                    )
            )
    }

    /**
     * Adjusts API definition to follow the API validation rule requiring alphabetically sorted tags.
     *
     * @see [
     * API validation rules](link removed)
     */
    private fun tagsSorterCustomizer(openAPI: OpenAPI) = openAPI.tags(openAPI.tags?.sortedBy { it.name } ?: emptyList())

    private fun infoCustomizer(openAPI: OpenAPI) = openAPI.info(
        Info()
            .title("XXX CV $name API")
            .description(description)
            .version(version)
            .extensions(
                mapOf(
                    "x-xxx-apiId" to "xxx-cv-${msIdLower.replace("-", "")}-api",
                ) + defaultExtensions
            )
            .contact(
                Contact()
                    .name(description)
                    .url("(link removed)")
                    .email(contact)
            )
    )

    /**
     * Adds a server URL to select in the SwaggerUI
     */
    private fun serverUrlCustomizer(openAPI: OpenAPI) {
        val urlPrefix = "$PROTOCOL_TLS$msIdLower"
        val urlPostFixEmea = "$REGION_EMEA.$product.$DAYTONA.$REGION_EMEA.$AWS_CLOUD_XXX/$msIdLower"
        val urlPostFixUs = "$REGION_US.$product.$DAYTONA.$REGION_US.$AWS_CLOUD_XXX/$msIdLower"

        openAPI.servers = listOf(

            // ---------------------------- LOCAL --------------------------------

            Server()
                .url(PROTOCOL_HTTP + "localhost:8080/" + msIdLower)
                .description("Local development"),

            Server()
                .url(PROTOCOL_TLS + "localhost:8081/" + msIdLower)
                .description("Local development (TLS)"),

            // ---------------------------- EUROPE -------------------------------

            Server()

                .url("$urlPrefix.test.$urlPostFixEmea")
                .description("EMEA-TEST"),

            Server()
                .url("$urlPrefix.e2e.$urlPostFixEmea")
                .description("EMEA-E2E"),

            Server()
                .url("$urlPrefix.int.$urlPostFixEmea")
                .description("EMEA-INT"),

            Server()
                .url("$urlPrefix.prod.$urlPostFixEmea")
                .description("EMEA-PROD"),

            // ------------------------------ US ----------------------------------

            Server()
                .url("$urlPrefix.e2e.$urlPostFixUs")
                .description("US-E2E"),

            Server()
                .url("$urlPrefix.prod.$urlPostFixUs")
                .description("US-PROD")
        )
    }

    /**
     * Customizes API definition for test support API.
     * Adjusts API definition to follow the API validation rules: sets version number in server URL
     * and removes version numbers from paths.
     *
     * @see [
     * API validation rules]((link removed))
     */
    private fun testSupportOpenApiCustomizer(openAPI: OpenAPI) {
        with(openAPI) {

            servers = listOf(
                Server().url("http://localhost:8080/$msIdLower/test-support/v1")
            )

            paths = paths
                .mapKeys { it.key.replaceFirst("/test-support/v1", "")}
                .let{ Paths().apply { putAll(it) } }

            info = Info()
                .title("XXX CV " + msIdUpper + "Test Support API")
                .description("Api shows how to use JPA, JMS and external services.")
                .version("1.0.0")
                .extensions(
                    mapOf(
                        "x-xxx-apiId" to "xxx-cv-$msIdLower-testsupport",
                        "x-xxx-tags" to listOf("test-support"),
                    ) + defaultExtensions

                ).contact(
                    Contact()
                        .name(name)
                        .url(productDepotUrl)
                        .email(contact)
                )
        }
    }
}

Where are these 12.4% duplicated lines from? Also from the class file (which I can't change as dev)?

gtoison commented 2 weeks ago

Ah right, the duplicates lines are part of SonarQube's built-in analysis, this is not from the plugin. If you open the corresponding source file in SonarQube there should be a vertical gray line at the left of the duplicated lines. Clicking that gray bar should tell you where the duplicates are image

gtoison commented 2 weeks ago

Thanks again for reporting the issue and the samples, I'm testing a fix for the bogus NP_NONNULL_FIELD_NOT_INITIALIZED_IN_CONSTRUCTOR: The compiled bytecode contains this method:

  // Method descriptor #104 ()V
  // Stack: 6, Locals: 1
  public DummyBE();
     0  aload_0 [this]
     1  aconst_null
     2  aconst_null
     3  aconst_null
     4  bipush 7
     6  aconst_null
     7  invokespecial com.bmw.cc.csvbe.persistence.AbstractAuditedBaseBE(java.util.UUID, java.time.OffsetDateTime, java.time.OffsetDateTime, int, kotlin.jvm.internal.DefaultConstructorMarker) [107]
    10  return
      Local variable table:
        [pc: 0, pc: 11] local: this index: 0 type: com.xxx.cases.entity.DummyBE

I suppose that this zero-arg constructor is required because it is a JPA entity and (as SpotBugs is saying) it seems to be assigning nulls to non-null fields. However this is generated code, so probably not very relevant to report this to the developpers.

rupert-jung-mw commented 2 weeks ago

The zero-arg constructor is automatically generated by the kotlin 'jpa' compiler plugin (which internally uses the no-args plugin), correct. That's basically a JPA-thing, it does just require a zero-arg-constructor and then sets the values via setters.