Skip to content
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

[K2] Fix sample resolving #3361

Merged
merged 4 commits into from
Nov 23, 2023
Merged

[K2] Fix sample resolving #3361

merged 4 commits into from
Nov 23, 2023

Conversation

vmishenev
Copy link
Member

No description provided.

Fallback to default roots of the source set even if sample roots are assigned
`sourcePsiSafe` does not support Java KT-53669
@vmishenev vmishenev self-assigned this Nov 21, 2023
Copy link
Member

@IgnatBeresnev IgnatBeresnev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking care of it! It thought it'd be more difficult for some reason

private fun findPsiElement(sourceSet: DokkaSourceSet, fqLink: String): PsiElement? {
val ktSourceModule = samplesKotlinAnalysis.getModuleOrNull(sourceSet)
?: projectKotlinAnalysis.getModule(sourceSet)
// fallback to default roots of the source set even if sample roots are assigned
Copy link
Member

@IgnatBeresnev IgnatBeresnev Nov 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea to add a comment! nitpick: it explains the code below, but it doesn't explain the intention, which is the less obvious part :)

Suggested change
// fallback to default roots of the source set even if sample roots are assigned
// fallback to default roots of the source set even if sample roots are assigned,
// because `@sample` tag can contain links to local project functions

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate on what local project functions means? Why is it "local"? I did not understand.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understood the problem and the fix the following way:

If you have the code like

/src/kotlin/com/example/MyFile.kt

/**
 * @sample bar
 */
fun foo() {
    
}

fun bar() {}

where @sample references a function declared in the project sources itself, like literally the function below it, we need to use projectKotlinAnalysis. This is what I meant by a "local" function.

And if you store sample functions outside of the project sources, like in a separate directory somewhere, and you configure it using DokkaSourceSet#samples, then it needs to use samplesKotlinAnalysis. I'd consider this "external"

The problem was that it was always using samplesKotlinAnalysis, which pointed to external sample functions, but it needs to resolve "local" project functions as well

If I got the idea and the fix correctly, you can use whatever words you find more suitable, that was just a suggestion :) But I might've misunderstood it, in which case it definitely needs some sort of a comment :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have changed a little to avoid a "local" function.

// because `@sample` tag can contain links to functions from project sources

return analyze(ktSourceModule) {
resolveKDocTextLinkSymbol(fqLink)
?.sourcePsiSafe()
return ktSourceModules.firstNotNullOfOrNull { ktSourceModule ->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling listOfNotNull to then do list.firstNotNullOfOrNull seems excessive in this case, and could be avoided like so

    private fun findPsiElement(sourceSet: DokkaSourceSet, fqLink: String): PsiElement? {
        // fallback to default roots of the source set even if sample roots are assigned,
        // because `@sample` tag can contain links to local project functions
        return samplesKotlinAnalysis.findPsiElement(sourceSet, fqLink)
            ?: projectKotlinAnalysis.findPsiElement(sourceSet, fqLink)
    }

    private fun KotlinAnalysis.findPsiElement(sourceSet: DokkaSourceSet, fqLink: String): PsiElement? {
        val ktSourceModule = this.getModuleOrNull(sourceSet) ?: return null
        return analyze(ktSourceModule) {
            resolveKDocTextLinkSymbol(fqLink)
                ?.kotlinAndJavaSourcePsiSafe()
        }
    }

@vmishenev vmishenev merged commit cafdaa2 into master Nov 23, 2023
11 checks passed
@vmishenev vmishenev deleted the vmishenev/3359-fix-samples branch November 23, 2023 15:29
vmishenev added a commit that referenced this pull request Mar 20, 2024
* [K2] Fix sample

Fallback to default roots of the source set even if sample roots are assigned

* [K2] Resolve java samples as well

`sourcePsiSafe` does not support Java KT-53669
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[K2] Sample analysis tests no longer pass with K2 analysis
3 participants