Add new experimental rule to remove trailing comma's from argument and values lists (#709)

[paul-dingemans]

Given code below:

            val list1 = listOf("a", "b",)
            val list2 = listOf(
                "a",
                "b", // The comma before the comment should be removed without removing the comment itself
            )

            data class Foo1(
               val bar: Int, // The comma before the comment should be removed without removing the comment itself
            )
            data class Foo2(val bar: Int,)

this should be corrected as:

            val list1 = listOf("a", "b")
            val list2 = listOf(
                "a",
                "b" // The comma before the comment should be removed without removing the comment itself
            )

            data class Foo1(
               val bar: Int // The comma before the comment should be removed without removing the comment itself
            )
            data class Foo2(val bar: Int)

Closes #709

[paul-dingemans]

Tnx for the link to the minimal list of types to be supported. I was not aware that trailing comma's were possible at other places as well. I have added all of them except for COLLECTION_LITERAL_EXPRESSION. I can not create a code example in which this type occurs.

[paul-dingemans]

Also, I think the rule should work both ways - depending on what is set in editorconfig either allow or disallow trailing commas

Isn't the option to disable rules meant for this? Or would you to like this rule to be disbaled by default and that user have to opt-in on the rule?

[romtsn]

Tnx for the link to the minimal list of types to be supported. I was not aware that trailing comma's were possible at other places as well. I have added all of them except for COLLECTION_LITERAL_EXPRESSION. I can not create a code example in which this type occurs.

For COLLECTION_LITERAL_EXPRESSION the only case (that I know of) is when the collection literal is put as an argument for an annotation class, for example:

annotation class Annotation(val params: IntArray)

// usage
@Annotation([1, 2,])
val property: Int = 0

Would be nice if we can support this as well.

Also, I think the rule should work both ways - depending on what is set in editorconfig either allow or disallow trailing commas

Isn't the option to disable rules meant for this? Or would you to like this rule to be disbaled by default and that user have to opt-in on the rule?

What I meant is we should decide how the rule behaves based on the intellij editorconfig properties

ij_kotlin_allow_trailing_comma = true
ij_kotlin_allow_trailing_comma_on_call_site = true

If those are set to true, the rule should enforce trailing commas and throw a violation if they are not present where they should be. If those are set to false, the rule should disallow trailing commas and throw a violation if they are present where they shouldn't be. So it should work similar to final-newline rule in this regard.

[paul-dingemans]

I solved the COLLECTION_LITERAL_EXPRESSION based on your example. Also the rule is now configured based on the IntelliJ IDEA properties.

First I have tried to define a new property analog to insert_final_newline in class PropertyType of ec4j :

    public static final PropertyType<Boolean> insert_final_newline = new LowerCasingPropertyType<>( //
            "insert_final_newline", //
            "set to true to ensure file ends with a newline when saving and false to ensure it doesn't.", //
            PropertyValueParser.BOOLEAN_VALUE_PARSER, //
            BOOLEAN_POSSIBLE_VALUES //
    );

I could not get it working due to some Kotlin to Java interoperability problem. So I have implemented a new parser for the Kotlin Boolean as shown below:

        private var KOTLIN_BOOLEAN_VALUE_PARSER: PropertyValueParser<Boolean> =
            PropertyValueParser { name, value ->
                when {
                    value == null -> PropertyType.PropertyValue.invalid(
                        null,
                        "Property '$name' expects a boolean; found: null"
                    )
                    value.equals("true", ignoreCase = true) -> PropertyType.PropertyValue.valid(value, true)
                    value.equals("false", ignoreCase = true) -> PropertyType.PropertyValue.valid(value, false)
                    else -> PropertyType.PropertyValue.invalid(
                        value,
                        "Property '$name' expects a boolean. The parsed '$value' is not a boolean."
                    )
                }
            }

Of course, class above can also be moved to UsesEditorConfigProperties or another new class so that it can be reused for other boolean properties.

[Tapchicoma]

I could not get it working due to some Kotlin to Java interoperability problem.

Just curious - what was the exact problem? 🤔

[paul-dingemans]

I could not get it working due to some Kotlin to Java interoperability problem.

Just curious - what was the exact problem? 🤔

Java definition in PropertyType class of ec4j

    public static final PropertyType<Boolean> insert_final_newline = new LowerCasingPropertyType<>( //
            "insert_final_newline", //
            "set to true to ensure file ends with a newline when saving and false to ensure it doesn't.", //
            PropertyValueParser.BOOLEAN_VALUE_PARSER, //
            BOOLEAN_POSSIBLE_VALUES //
    );

Copy this to my class resulted, after automatic kotlin conversion, in:

        val insert_final_newline: PropertyType<Boolean> = LowerCasingPropertyType<Any>( //
            "insert_final_newline",  //
            "set to true to ensure file ends with a newline when saving and false to ensure it doesn't.",  //
            PropertyValueParser.BOOLEAN_VALUE_PARSER,  //
            PropertyType.BOOLEAN_POSSIBLE_VALUES //
        )

The LowerCasingPropertyType<Any> can not be resolved. Error displayed is None of the following functions can be called with the arguments supplied..

Looking at the definition of LowerCasingPropertyType<Any>:

    public static class LowerCasingPropertyType<T> extends PropertyType<T> {

        public LowerCasingPropertyType(String name, String description, PropertyValueParser<T> parser,
                Set<String> possibleValues) {
            super(name, description, parser, possibleValues);
        }

        public LowerCasingPropertyType(String name, String description, PropertyValueParser<T> parser,
                String... possibleValues) {
            super(name, description, parser, possibleValues);
        }

        @Override
        public String normalizeIfNeeded(String value) {
            return value == null ? null : value.toLowerCase(Locale.US);
        }

    }

Kotlin can not distinct the method signatures. Apparently it can not distinct between Set<String> possibleValues and String... possibleValues. I wouldn't know how to resolve this. But if you do know, I would like to know how I could have found out that myself.

[ScottHamper]

Hey @paul-dingemans ,

I think the automatic Kotlin conversion produced invalid code. Specifically, if we compare the following Java:

PropertyType<Boolean> insert_final_newline = new LowerCasingPropertyType<>(

With the following Kotlin:

insert_final_newline: PropertyType<Boolean> = LowerCasingPropertyType<Any>(

Notice that on the right-hand side of the Java version, the generic type is specified with an empty <>. In Java, this tells the compiler to automatically determine the generic type by looking at the left-hand side of the assignment. In this case, that generic type is Boolean, and so the right-hand side ends up being an instance of LowerCasingPropertyType<Boolean>.

However, in the Kotlin conversion, a generic type of Any is used and an instance of LowerCasingPropertyType<Any> cannot by assigned to an identifier of type PropertyType<Boolean>. I'm still a newbie when it comes to Kotlin, but it looks like we can get Kotlin to automatically determine the generic type on the right-hand side by omitting the <> completely. In other words, we should be able to do the following:

insert_final_newline: PropertyType<Boolean> = LowerCasingPropertyType(

[paul-dingemans]

Hi @ScottHamper,
Tnx for trying to help out on this. I tried your suggestions bu they didn't help to fix the problem. But you inspired me to take an extra glance on it. Now it is solved by defining the property as follows:

        private val allowTrailingCommaPropertyType: PropertyType<Boolean> = PropertyType.LowerCasingPropertyType(
            "ij_kotlin_allow_trailing_comma",
            "ij_kotlin_allow_trailing_comma description",
            PropertyValueParser.BOOLEAN_VALUE_PARSER,
            mutableSetOf("true", "false")
        )

Crucial part in this was to define the allowed values as a set instead of an array.

[addyi]

@paul-dingemans Thank you for taking time to implement this. I'm also interested in this rule. @romtsn Are there any plans on how to go forward with this PR?

[paul-dingemans]

@shashachu Is there any chance that this PR will get merged?

[romtsn]

Sorry for holding this off for so long @paul-dingemans , I finalized the rule by making it work bi-directional (as mentioned in #1032 (comment)), so it also can add missing trailing commas, if configured like that. Thanks for prior art 👍