Streamline KDoc ambiguity links

[vmishenev]

This is an issue to discuss Streamline KDoc ambiguity links in KDoc. The current full text of the proposal can be found here.

[yanex]

In the following example, I find the parameter/property resolution rule unclear.

/** 
 * [A] - to the class, [abc] - to the property 
 * @constructor [A] - to the constructor, [abc] - to the parameter 
 */ 
class A(var abc: String)

What if we remove the var keyword? Should abc become unresolved, or are properties preferred over parameters? If the latter, why so?

[yanex]

Also, the order is defined by the Kotlin Language Specification: Linked scopes, Overload resolution and other sections.

The proposal mentions overload resolution. However, in the KDoc, there are no type/value arguments from which the candidate can be resolved. What should we do if the callable candidates are inside two different scopes with the same priority?

[vmishenev]

What if we remove the var keyword? Should abc become unresolved, or are properties preferred over parameters? If the latter, why so?

In this example, another rule is also used: the KDoc context of a class should correspond to the first line of a fake function.
Therefore, it makes sense to make [abc] unresolved.

/** 
 * [A] - to the class, [abc] is unresoved
 * @constructor [A] - to the constructor, [abc] - to the parameter 
 */ 
class A(abc: String) {
       fun fake() {
            abc // unavailable here
       }
}

Thank you for bringing that up. I will add it to the document.

The proposal mentions overload resolution.

It was mentioned because the section Overload resolution in the language specification specifically defines the order of scopes. I will rephrase this sentence in the proposal.

What should we do if the callable candidates are inside two different scopes with the same priority?

In this case, we should use the order of scopes - For declarations with the same priority and the same name, selecting a target declaration should be based on the order of scopes as implemented in the compiler.
For overloads in the same scope, we can take the first one, as we do in K1.

[marcopennekamp]

I have a number of comments, so I've organised them into a few sections.

First of all, though, it's an awesome step for KDocs to have these KEEPs now. Thanks for putting in the time to formulate these designs!

General

Lack of different perspectives

The problem section of the document is too specific for a language design proposal, in my opinion. It feels like there is too much emphasis/focus on Dokka's migration to K2.

The risk here is a lack of different perspectives: If we only formulate the problem (and from that the solution) based on the difficulties encountered during Dokka's and IntelliJ's migration to K2, we risk that the solution will fix specific problems for Dokka and IntelliJ from our perspective as developers, but disregard general user sentiment (due to not enough information being available during the design phase).

I think KDoc in general should be a first-class language entity and it deserves the same treatment as any other language feature. KDocs are part of the code, so we also need to consider its backwards compatibility with the same care as "hard" language features. We have to encourage the same engagement from Kotlin users as in other KEEPs, and we need to apply the same, broad use case analysis as in other KEEPs.

As this comment might not be specific enough, let me suggest the following: Let's do a bit of qualitative research about how Kotlin developers perceive and experience ambiguities in KDoc links. This will help us validate the proposed solution and maybe see a few issues from a different angle.

Stakeholders

I think we should bring in a stakeholder from the Kotlin IntellIJ plugin team so that the IDE perspective is sufficiently represented.

We should also probably bring in a stakeholder from the Language Evolution team, at least if we want to treat KDocs as first-class language features. This is a language design question and needs the input and approval of the team responsible for the language design.

Style

Overall, I think the KEEP should be edited for better readability and clarity. Some sentences are hard to understand due to the grammar/writing style.

"Ambiguity in other cases"

The "Ambiguity in other cases" section is unclear to me: Are these precedence rules defined by Dokka and the K1 Kotlin plugin independently, or are they general rules? How does the K2 implementation achieve the same behavior? Are the precedence rules explicitly present there as well or does it "just work?"

Maybe this section is too focused on the implementations and should focus on describing K1 behavior and contrasting it to/matching it with K2's behavior.

"Context of KDoc"

Primary constructor parameters

Here context is the set of all name bindings available by its short names in a current KDoc comment (scope vs context). For example, the KDoc context of a class should correspond to the first line of a fake function.

I disagree with this. We should be able to refer to primary constructor parameters (without a corresponding property) in the class's KDoc. I have a hunch that this will break a lot of current use cases in existing Kotlin code. (And this supports my suggestion to do user research about this.)

I understand the idea of having a dedicated @constructor section, but the Kotlin syntax deliberately hides the concept of a primary constructor (unless in specific cases where the constructor keyword is used) and merges it with the class's "primary" properties. This is a syntactic choice which should be honored in the KDoc design as well: primary constructor parameters are first-class citizens of the class's signature, even if they are not properties. We cannot have merged primary constructors and primary property declarations on one hand, and separate class and primary constructor KDocs on the other hand.

So perhaps in a non-@constructor class KDoc, the parameter can still be resolved. If it's also a property, the property simply takes precedence. And if we're in an @constructor context, the parameter takes precedence over the property.

"Priorities"

Nested class or outer class in sibling links

Here is an example with a nested class:

Where would A in foo's KDoc point? The nested class or the containing class? (I think the example needs this case since [A] in the other members is a self-link.)

class A {
    /** [A] - to the nested class A */
    class A
    /** [A] - to fun A(p: Int) */
    fun A(p: Int) { 
        return A()
    }

    /** [A] */
    val foo: String = ""
}

Multiple results in overload cases

In the case of overload ambiguity, the previous behaviour (to the first occurrence) is left unchanged.

I believe we shouldn't define ambiguity resolution in KDoc generally to require a single result. This seems like a requirement that's specific to Dokka, as the IDE can and wants to display multiple options to navigate to. In the given example, the best behavior in the IDE would be to display both overloaded functions and let the user select one.

Yes, there should be precedences (like self-links having higher precedence than all other targets), but within the same precedence, it'd be good to allow multiple results. Then the language specification essentially says "this KDoc link has multiple targets" and each tool can choose itself how to deal with that. Or we can define rules such as "first source occurrence" in the KEEP/specification which can be applied if the full list cannot be displayed/processed/used: "Either the full list of targets should be used, or the following disambiguation rule needs to be applied."

[vmishenev]

Thank you for your review. This document is the first step in starting to work on KDoc

Lack of different perspectives

I think KDoc in general should be a first-class language entity and it deserves the same treatment as any other language feature.

I totally agree with you. Currently, what we know about KDoc is only this page. We as the Dokka team have decided to start working on it incrementally. The actual questions of Dokka's and IntelliJ's migration to K2 serve as a good start point. This effort will benefit not only the Analysis API, but also users migrating to Dokka/IDE K2. Additionally, the reasons for this initiative are outlined in a ticket of planning.

Our current goal is to clarify the existing description of KDoc in discovered corner cases to stick some behavior. We should avoid introducing any new language features, additional syntax, or essential semantic changes. That is why we follow a fast track skipping certain stages of language design.

In the future, we plan to follow the full implementation cycle for real language features. In general, the problem of ambiguity requires additional disambiguating syntax. For example, one of the most popular issue requiring extra syntax is Kotlin/dokka#80 Future proposals will have an ability to get rid off ambiguity.

Stakeholders

I think we should bring in a stakeholder from the Kotlin IntellIJ plugin team so that the IDE perspective is sufficiently represented.

Sure, we are going to explicitly invite the Kotlin IntellIJ plugin team to participate in the discussion of future documents.

This document was discussed with the Language Evolution and the Kotlin Library teams, and it received their approval as well.

Style

Sorry about that. I hope I will have time to do several rounds of proofreading. I would appreciate it if you could point out mistakes.

"Ambiguity in other cases"

I think the section could be named better, but I couldn't come up with a better name.

Are these precedence rules defined by Dokka and the K1 Kotlin plugin independently, or are they general rules?

Dokka receives an ordered list of candidates from the K1 Kotlin plugin.

How does the K2 implementation achieve the same behavior? Are the precedence rules explicitly present there as well or does it "just work?"

Dokka K2 implements the same sorting by kind of declarations from the K1 Kotlin plugin, since the K2 Kotlin plugin did not have it at the time of writing this document. - https://youtrack.jetbrains.com/issue/KT-64190/K2-IDE-Analysis-API-KDoc-link-leads-to-a-function-instead-of-interface (already fixed).

Primary constructor parameters

We should be able to refer to primary constructor parameters (without a corresponding property) in the class's KDoc. I have a hunch that this will break a lot of current use cases in existing Kotlin code.

If I understand correctly, you mean a case like:

/**
 * [param] 
 */
class A(param: Int)

[param] is unresolved in the K1 Kotlin plugin and Dokka K1. I am not sure if the document break something here.

So perhaps in a non-@constructor class KDoc, the parameter can still be resolved

I think It can be a language feature worth considering separately, as we do not support it properly, in K1.

Nested class or outer class in sibling links

The idea here is to imitate resolving in Kotlin code to keep consistency between KDoc and code. For example, If we change String to A in foo:

class A {
   /** [A] - to the nested class A */
   class A
   
   /** [A] - to the nested class A */
   val foo: A  // the type `A` refers to the nested class A
}

I will add it to the document. Thank you.

Multiple results in overload cases

It could also be a dedicated feature deserving another KEEP. We have a very popular request - Kotlin/dokka#80 that might be addressed as well. Should we introduce special syntax to display multiple overloads, e.g. foo(*)? That is also a question. As you mentioned, we might need research.
It is better to not change the current behavior of overloads.

[marcopennekamp]

Thanks for your in-depth reply! I think my review was overall a bit too critical—at least I should've mentioned the positives. I'm really happy that KDoc is moving into the language as a first-class feature. Most of the precedence rules outlined in the document are sensible and I agree with them.

I also really like the approach of "the references resolvable in the KDoc are equal to the resolvable reference at this place in the code." I think it's really elegant.

So overall, it's a commendable effort!

Our current goal is to clarify the existing description of KDoc in discovered corner cases to stick some behavior. We should avoid introducing any new language features, additional syntax, or essential semantic changes.

The KEEP should definitely mention this, not just a planning ticket.

That is why we follow a fast track skipping certain stages of language design.

I would think that precisely because we're describing existing behavior, we should be intentionally careful, instead of trying to fast-track a specification which might not yet be the design we want.

Until now, we've had no clear specification of KDoc. This gives a huge opportunity for a great design (and an accompanying vision) which adheres to what people do now, but also brings KDoc into a well-footed framework.

If the goal is to specify the existing behavior of KDoc, there should be a comprehensive design attempt which unifies KDoc behavior into a single specification. I worry that these two KEEP proposals don't cover all the areas—at least it is not obvious if they do, as KDoc surely isn't just a combination of ambiguity resolution and extension function resolution. (Maybe it is? If so, let's make it more explicit!)

In other words: if we start off with a fragmented design from the get-go (with fragmented design documents), it's unlikely that we'll achieve the cohesion that we need.

This document was discussed with the Language Evolution and the Kotlin Library teams, and it received their approval as well.

OK, that's great! I wasn't aware of this.

Sorry about that. I hope I will have time to do several rounds of proofreading. I would appreciate it if you could point out mistakes.

I could do an editing pass once the contents of the document have been agreed upon, if you like. :)

[param] is unresolved in the K1 Kotlin plugin and Dokka K1. I am not sure if the document break something here.

Oh! I only tried it with K2, so if it's unresolved in K1, it does not break backwards compatibility. Sorry, I should've checked in K1 as well.

Regarding not adding this in the initial specification, I think it's less a feature and more a design fix. The behavior is at odds with the way Kotlin intertwines class and constructor syntax (as I outlined in my first comment), so in this specific case, I'd advocate for adding this behavior to the initial KDoc specification. Otherwise, the initial specification contains a broken design which needs to be fixed in a subsequent KEEP.

Also, the ability for [param] to be unresolvable to the constructor parameter would introduce backwards compatibility issues later, if a user refers to e.g. another function in the same file with the name:

/**
 * The function [param] can be used to get a default value for the parameter.
 */
class A(param: Int)

fun param(): Int = 12

If we change constructor parameter resolution later, we're actually breaking the initial spec's backwards compatibility, because the constructor parameter would have higher precedence than a function in the file. Or we need to introduce totally convoluted precedence rules specifically for constructor parameters (i.e. a function in the same file has a higher precedence than the constructor parameter).

It is better to not change the current behavior of overloads.

I disagree, because the specification will then limit the ability of the IDE to display multiple options for the user. It makes more sense for the specification to dictate which (multiple) overloads can be resolved from a given reference (maybe it is a subset of overloads if we add additional disambiguation syntax), but not how that list should be used. Or, at the very least, allow the option to either display the whole list or resolve to the first occurrence.

Note that this feature is independent of additional overload disambiguation syntax. So to me it seems strange to require a new KEEP for it, when we can fix it with the initial effort. Again, to me the behavior of K1 feels more like a design error than a missing feature.