-
Notifications
You must be signed in to change notification settings - Fork 372
Adjust the Streamline KDoc ambiguity links
KEEP according to Dokka / Analysis API design discussions
#443
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
base: main
Are you sure you want to change the base?
Conversation
Hi, please pay close attention to all the examples and details in the tag-sections chapter. I tried to gather all the information on them I could from our discussions and from the original KEEP, however, I might have misunderstood something. And I'm still not quite sure about the details of this multi-resolution. In the KEEP I mentioned that we should return all the symbols the compiler could resolve this reference to (i.e. all symbols from some scope), not just symbols of the same kind. It seems like a valid and requested feature, and there won't be too many symbols with the same name in the same scope anyways. It's probably more important than just handling overloads by returning multiple symbols of the same kind. /**
* @see Foo - now it's able to point to the function
*/
interface Foo
fun Foo(): Foo = object : Foo {} |
The proposal is already merged to the |
…discussions See KT-76607
d6a1d6f
to
9a3f9b8
Compare
proposals/KEEP-0389-kdoc-streamline-KDoc-ambiguity-references.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comprehensive update! 🙌
I left a lot of comments in the review, but I think my main points of contention are:
- We need clear definitions for the key concepts, such as the "KDoc context", the exact steps of the resolution algorithm, and generally just more precise formulations for a lot of things.
- I think the resolution algorithm could be described more clearly by mentioning the exact layers (self-links, scopes, global references, packages), essentially rearranging some sections a bit to arrive at a single, well-described algorithm.
- We need to discuss property resolution and whether (1) properties should be resolved as first segments during multi-segment resolution and (2) legally accessing a property (e.g.
[name.length]
) in KDoc references is desired in the future.
And this definitely needs another round of copyediting, but let's get the specifics right first. 👍
proposals/KEEP-0389-kdoc-streamline-KDoc-ambiguity-references.md
Outdated
Show resolved
Hide resolved
proposals/KEEP-0389-kdoc-streamline-KDoc-ambiguity-references.md
Outdated
Show resolved
Hide resolved
proposals/KEEP-0389-kdoc-streamline-KDoc-ambiguity-references.md
Outdated
Show resolved
Hide resolved
proposals/KEEP-0389-kdoc-streamline-KDoc-ambiguity-references.md
Outdated
Show resolved
Hide resolved
proposals/KEEP-0389-kdoc-streamline-KDoc-ambiguity-references.md
Outdated
Show resolved
Hide resolved
proposals/KEEP-0389-kdoc-streamline-KDoc-ambiguity-references.md
Outdated
Show resolved
Hide resolved
* Before performing a scope transformation for relative names, the resolver has to make sure that there are no | ||
non-function declarations matching some prefix of the given name in a more local scope. | ||
If such a declaration is found in some scope, then there is no need to process all further scopes, | ||
as the compiler would resolve this prefix to the declaration in this scope. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the wording is quite roundabout here. For example, "prefix" can suggest a lot of incorrect things, like a prefix of the string name, or more than the first segment (a multi-segment prefix of a multi-segment name).
I would specifically concentrate in the definitions in this whole section on "resolving the first segment" and whenever that resolves to a non-function, we have deterministically picked the scope that we want to apply relative resolution to.
And then we don't need any special wording for "if a conflicting declaration is found in a local scope, the resolver should not proceed to further scopes/global retrieval", because the outlined algorithm clearly stops where the first segment is resolved to a non-function.
proposals/KEEP-0389-kdoc-streamline-KDoc-ambiguity-references.md
Outdated
Show resolved
Hide resolved
proposals/KEEP-0389-kdoc-streamline-KDoc-ambiguity-references.md
Outdated
Show resolved
Hide resolved
proposals/KEEP-0389-kdoc-streamline-KDoc-ambiguity-references.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the additions and proofreading 👍
I have not properly checked the 'Implementation details' section, as I am not familiar with the implementation, although the described logic seems correct.
I would also like to see the final version of KEEP after editing.
proposals/KEEP-0389-kdoc-streamline-KDoc-ambiguity-references.md
Outdated
Show resolved
Hide resolved
proposals/KEEP-0389-kdoc-streamline-KDoc-ambiguity-references.md
Outdated
Show resolved
Hide resolved
proposals/KEEP-0389-kdoc-streamline-KDoc-ambiguity-references.md
Outdated
Show resolved
Hide resolved
proposals/KEEP-0389-kdoc-streamline-KDoc-ambiguity-references.md
Outdated
Show resolved
Hide resolved
proposals/KEEP-0389-kdoc-streamline-KDoc-ambiguity-references.md
Outdated
Show resolved
Hide resolved
proposals/KEEP-0389-kdoc-streamline-KDoc-ambiguity-references.md
Outdated
Show resolved
Hide resolved
proposals/KEEP-0389-kdoc-streamline-KDoc-ambiguity-references.md
Outdated
Show resolved
Hide resolved
proposals/KEEP-0389-kdoc-streamline-KDoc-ambiguity-references.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Great work!
I've left mostly some questions regarding some of the examples, as the other parts already have a lot of good feedback!
proposals/KEEP-0389-kdoc-streamline-KDoc-ambiguity-references.md
Outdated
Show resolved
Hide resolved
proposals/KEEP-0389-kdoc-streamline-KDoc-ambiguity-references.md
Outdated
Show resolved
Hide resolved
proposals/KEEP-0389-kdoc-streamline-KDoc-ambiguity-references.md
Outdated
Show resolved
Hide resolved
proposals/KEEP-0389-kdoc-streamline-KDoc-ambiguity-references.md
Outdated
Show resolved
Hide resolved
val A = 5 | ||
|
||
fun usage() { | ||
A.B.C() // UNRESOLVED |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you help me to understand, why we don't resolve this link, but resolve the link above, with fun A()
? (and the same for example with receiver below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the compiler does so. Paste these code snippets in your IDE and see how the resolution changes depending on the type of A
here. If it encounters something other than a function while trying to resolve some segment (A
in this case), i.e. a property or a class, it stops and considers the reference to be unresolved. So the term "non-function" is correct here. I'll try to make it more transparent
The resolver should iterate through scopes from local to global | ||
and look for a chain of nested classifiers starting in the current scope. | ||
If this chain is found, | ||
the resolver just picks this classifier member scope and looks for symbols with the given short name. | ||
However, if, at some point of this symbols-by-segment search, | ||
the resolver comes across some non-function symbol from which | ||
it's impossible to continue the chain, the resolver should stop and consider this link as unresolved. | ||
Examples of such cases are classes with no nested classifiers matching the next segment and properties. | ||
Again, see [following chapter](#restrictions-of-multi-segment-names-resolution) for more info. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can I ask a bit of an unrelated question?
Why did the question about properties (e.g., [name.length]
) even appear in the discussion? I mean, this isn't a supported KDoc syntax, and it is not mentioned in the document (as far as I see).
I mean, maybe I'm missing something obvious, or compiler-related knowledge, but for me, this feels like a whole new feature for KDoc, similar to overloads, and so should be discussed totally separately.
proposals/KEEP-0389-kdoc-streamline-KDoc-ambiguity-references.md
Outdated
Show resolved
Hide resolved
proposals/KEEP-0389-kdoc-streamline-KDoc-ambiguity-references.md
Outdated
Show resolved
Hide resolved
proposals/KEEP-0389-kdoc-streamline-KDoc-ambiguity-references.md
Outdated
Show resolved
Hide resolved
proposals/KEEP-0389-kdoc-streamline-KDoc-ambiguity-references.md
Outdated
Show resolved
Hide resolved
Streamline KDoc ambiguity links
KEEP according to Dokka / Analysis API design discussions
2. Then the resolver considers the [lexical scope](https://en.wikipedia.org/wiki/Scope_(computer_science)#Lexical_scope_vs._dynamic_scope_2) | ||
of the documented declaration, i.e., member scope of every outer declaration in the order of their | ||
locality. | ||
These scopes also contain context/type/value parameters of these outer declarations. | ||
Then, in each scope, the resolver tries to acquire matching declarations contained in this member scope. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before your changes, did the K2 implementation already consider lexical scopes?
This section is about the implementation before the KEEP, right? So it should reflect the state of the current algorithm in master
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It did, see here
`A.B.C` implies that somewhere there is a chain of two nested classifiers `A` and `B` with `B` | ||
containing some member `C`. | ||
The resolver should try to find a classifier `A` in each of these scopes and, if found, | ||
replace the original scope with the member scope of this class. | ||
If no suitable classes are found in some scope, this scope should be discarded. | ||
Then the resolver should take all the modified scopes and repeat the search for nested classifiers (`B` in this case) | ||
until just the short name `C` is left. | ||
Then it's just a single-segment name search in the constructed list of scopes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this just a two-step process where we:
- Search for the classifier
A
in any of the scopes. - Perform a chained search through the member scope of
A
, thenB
, etc.
Or does your approach explicitly support walking up the scopes if the chained search fails in a member scope?
For example:
class A {
class B
}
class Foo {
class A
/** [A.B] */
fun foo() {}
}
The two-step process would not be able to resolve A.B
because it always resolves to the nested class A. Is your approach as described here intended to resolve A.B
to the B
nested class of the top-level A
?
since it is banned in such languages as Java and C#. | ||
|
||
Here is another example: | ||
While preferring parameters / properties of the documented declaration over other symbols, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part is very confusing. From the start of the doc we lay the ground rule: "self-links always have the highest priority". And then we suddenly reveal that actually they're not. This is especially confusing because of the naming: 'self-links' work for everything but 'self'.
I suggest that we mention this 'peculiarity' of K1 in the K1 implementation
section
No description provided.