OutdatedDocumentation should allow us to use @param even when constructor properties have val

[matejdro]

Expected Behavior of the rule

Following code:

/**
 * @param firstParam Documentation for the first param
 * @param secondParam Documentation for the first param
 */
class MyClass(val firstParam: String, val secondParam: String)

fails the OutdatedDocumentation rule. Rule expects that all val constructor parameters are marked as @property instead.

But there is really no benefit to using @property that I can see.

Context

Main drawback of using @property is that there is no documentation when calling constructor of the class:

As opposed to @param:

However, even when using @param, IntellIJ still populates the property javadoc, so using @param actually gives us the best of both worlds:

Semi-related to the #4365

[severn-everett]

This looks more like a bug in KDoc, given that marking even one property as @property will cause the document generator to not display the Params: list. I've created an issue in YouTrack for Kotlin so that the language's authors can give feedback on this.

[matejdro]

Regardless, I still don't see why @param is considered invalid here? Both @param and @property would be equally valid here (since this constructor represents both).

I can submit PR to fix this, if desired.

[severn-everett]

It's invalid because firstParam and secondParam are properties, not parameters, given they possess the val keyword. If this is an issue with KDoc - and I believe it is, given that marking one property as @property and another as @param produces the same lack of rendering in the KDoc popup - I would wait to see what Jetbrains says about whether they agree that this is an issue before creating a potential workaround, as once that workaround is in, it'll have to be maintained.

[matejdro]

Well, at the same time, they are parameters, because they are part of the constructor.

[severn-everett]

Per KDoc's documentation:

@property name
Documents the property of a class which has the specified name. This tag can be used for documenting properties declared in the primary constructor, where putting a doc comment directly before the property definition would be awkward.

Yes, it's physically possible to mark a property in the constructor with @param, but that applies to lots of code that is flagged by other rules in Detekt as well, and I'm not seeing how this is different, especially given Jetbrains hasn't weighed in on the potential KDoc rendering issue yet.

[BraisGabin]

I agree with severn here. It seems like a bug in the Jetbrains side. @property It feels more natural to describe the property instead of the param and the documentation say that it is there for that reason. Let's wait to get an answer in the other issue.

[matejdro]

I agree with both of you but I just want to mention that answer from Jetbrains might take a while. In the meantime, the issue persists.

If they do not answer in a meaningful time frame, I would opt for adding allowParamOnConstructorProperties option to the OutdatedDocumentation that allows this (again, I'm prepared to make PR for this).

[severn-everett]

Just got a response from the JetBrains team: this was already identified as an issue in the IntelliJ plugin... three years ago. It doesn't look like there's any movement on fixing it anytime soon, so maybe we should allow the option like @matejdro suggests.

[cortinico]

I would opt for adding allowParamOnConstructorProperties option to the OutdatedDocumentation that allows this (again, I'm prepared to make PR for this).

Given that a fix from JB side would take some time, I believe that allowParamOnConstructorProperties sounds like a good compromise. If you end up making a PR @matejdro, please add a note with a reference to the YouTrack issue.