@sample tags should not be moved within kdoc

[liutikas]

Repro steps

java  -cp /ssd/ssd5/androidx-main/prebuilts/androidx/external/com/facebook/ktfmt/0.44/ktfmt-0.44.jar:..all.jars com.facebook.ktfmt.cli.Main --kotlinlang-style myfile.kt

• Try to format the following file:

package androidx.compose.runtime.saveable
 
/**
 * Remember the value produced by [init].
 *
 * It behaves similarly to [remember], but the stored value will survive the activity or process
 * recreation using the saved instance state mechanism (for example it happens when the screen is
 * rotated in the Android application).
 *
 * @sample androidx.compose.runtime.saveable.samples.RememberSaveable
 *
 * If you use it with types which can be stored inside the Bundle then it will be saved and
 * restored automatically using [autoSaver], otherwise you will need to provide a custom [Saver]
 * implementation via the [saver] param.
 *
 * @sample androidx.compose.runtime.saveable.samples.RememberSaveableCustomSaver
 *
 * You can use it with a value stored inside [androidx.compose.runtime.mutableStateOf].
 *
 * @sample androidx.compose.runtime.saveable.samples.RememberSaveableWithMutableState
 *
 * If the value inside the MutableState can be stored inside the Bundle it would be saved
 * and restored automatically, otherwise you will need to provide a custom [Saver]
 * implementation via an overload with which has `stateSaver` param.
 *
 * @sample androidx.compose.runtime.saveable.samples.RememberSaveableWithMutableStateAndCustomSaver
 *
 * @param inputs A set of inputs such that, when any of them have changed, will cause the state to
 * reset and [init] to be rerun
 * @param saver The [Saver] object which defines how the state is saved and restored.
 * @param key An optional key to be used as a key for the saved value. If not provided we use the
 * automatically generated by the Compose runtime which is unique for the every exact code location
 * in the composition tree
 * @param init A factory function to create the initial value of this state
 */
@Composable
fun <T : Any> rememberSaveable(
    vararg inputs: Any?,
    saver: Saver<T, out Any> = autoSaver(),
    key: String? = null,
    init: () -> T
): T { TODO() }

Expected

No changes are made

Actual

ktfmt format command will move @param tags to be next to @sample with @sample at the end.

resulting in

/**
 * Remember the value produced by [init].
 *
 * It behaves similarly to [remember], but the stored value will survive the activity or process
 * recreation using the saved instance state mechanism (for example it happens when the screen is
 * rotated in the Android application).
 *
 * @param inputs A set of inputs such that, when any of them have changed, will cause the state to
 *   reset and [init] to be rerun
 * @param saver The [Saver] object which defines how the state is saved and restored.
 * @param key An optional key to be used as a key for the saved value. If not provided we use the
 *   automatically generated by the Compose runtime which is unique for the every exact code
 *   location in the composition tree
 * @param init A factory function to create the initial value of this state
 * @sample androidx.compose.runtime.saveable.samples.RememberSaveable
 *
 * If you use it with types which can be stored inside the Bundle then it will be saved and restored
 * automatically using [autoSaver], otherwise you will need to provide a custom [Saver]
 * implementation via the [saver] param.
 *
 * @sample androidx.compose.runtime.saveable.samples.RememberSaveableCustomSaver
 *
 * You can use it with a value stored inside [androidx.compose.runtime.mutableStateOf].
 *
 * @sample androidx.compose.runtime.saveable.samples.RememberSaveableWithMutableState
 *
 * If the value inside the MutableState can be stored inside the Bundle it would be saved and
 * restored automatically, otherwise you will need to provide a custom [Saver] implementation via an
 * overload with which has `stateSaver` param.
 *
 * @sample androidx.compose.runtime.saveable.samples.RememberSaveableWithMutableStateAndCustomSaver
 */
@Composable
fun <T : Any> rememberSaveable(
    vararg inputs: Any?,
    saver: Saver<T, out Any> = autoSaver(),
    key: String? = null,
    init: () -> T
): T { TODO() }

Details

@sample tags are sensitive to their location within kdoc as that indicates that location where the documentation tool should insert the referenced code.

Using ktfmt 0.44

[davidtorosyan]

Thank you @liutikas for the thorough report!

If I remember right, this is intentional, and all tags appear at the end of rendered kdocs. They're also generally multiline, so if you have a block like this:

A
@sample B
C

You might be surprised to find that C is actually included in the @sample.

That said, let me double check and see if @sample should get special handling. Let me know if you have any docs or examples showing that this is the case!

cc @tnorbye

[davidtorosyan]

Here's a screenshot showing that Android Studio (I don't know about other rendering engines) likes to move the samples to the end.

Closing as not planned, please comment if you think there's still something here to look into.

[liutikas]

You might be surprised to find that C is actually included in the @sample.

Can you explain why you find it surprising that C in included? It is also not included in the sample, but rather just its own paragraph

That said, let me double check and see if @sample should get special handling. Let me know if you have any docs or examples showing that this is the case!

I'll discuss this with jetbrains offline next week during google/jetbrains sync to clarify what they intend specs to be.

In general, if you don't support this in the tool, we'll likely have to fork ktfmt which i'd really like to avoid.

[davidtorosyan]

Can you explain why you find it surprising that C in included? It is also not included in the sample, but rather just its own paragraph

Sure, what I mean is that it is included in the sample, kinda. You'll find that with that setup, C is not in its own paragraph. It also doesn't appear in sample, but that's just because everything after the first ref is discarded (as far as I can tell).

Let me know what you find out from jetbrains. And like I mentioned, if you can share a sample of how a kdoc renderer would actually process your example, it'll be easier to evaluate what's best to do.

[davidtorosyan]

@liutikas here's a better example, which demonstrates what I mean. You'll notice in the rendered kdoc that the paragraph between the samples is lost (the line that reads "But you can go even higher!")

Also, here's how Android Studio formats the example you gave in the post:

[tnorbye]

The Kotlin plugin in the IDE does a really poor job rendering KDocs; it doesn't actually run dokka, it just has a light simulation, so it often gets things wrong (remember how we had the same issue a while back with hanging indents as well).

What I usually do is to run Dokka to see how the documentation is rendered. I have a checkout of the Dokka github project, then I go into examples/gradle/dokka-gradle-example and modify the same project's documentation, then run ./gradlew dokkaHtml, then open the HTML pages under ./build/dokka/html to see how Dokka renders it.

And this confirms that Dokka does move all embedded samples to a single section at the end.

For example, given this patch:

diff --git a/examples/gradle/dokka-gradle-example/src/main/kotlin/demo/HelloWorld.kt b/examples/gradle/dokka-gradl
e-example/src/main/kotlin/demo/HelloWorld.kt
index 172e18f7c..2d973277c 100644
--- a/examples/gradle/dokka-gradle-example/src/main/kotlin/demo/HelloWorld.kt
+++ b/examples/gradle/dokka-gradle-example/src/main/kotlin/demo/HelloWorld.kt
@@ -9,10 +9,27 @@ class Greeter(val name: String) {

     /**
      * Prints the greeting to the standard output.
+     * @sample demo.Greeter.groupBySample
+     *
+     * And here's a different sample:
+     *
+     * @sample demo.Greeter.groupBySample2
+     *
+     * And yet another one:
+     * @sample demo.Greeter.groupBySample3
      */
     fun greet() {
         println("Hello $name!")
     }
+
+fun groupBySample() {
+    // ommitted code 1 //
+}
+fun groupBySample2() {
+    // ommitted code 2 //
+}
+fun groupBySample3() {
+    // ommitted code 3 //
 }

I get the following rendering:

[davidtorosyan]

Thanks for confirming @tnorbye. This also gives me two ideas:

1. Can we stick a dokka renderer in the ktfmt sample site? That would help us validate issues like this more quickly.
2. While ktfmt (correctly) moves @sample tags to the end, it brings the paragraph attached to them as well. While this is also accurate, it may give the false impression that those lines will get rendered by dokka. If we instead pull all that text to the top, the user would at least know something was up, and perhaps reformat their doc string. Would this be useful or too heavy handed?

[liutikas]

TL;DR from the discussion between google and jetbrains:

• kdoc doesn't currently have a spec
  • medium/long term spec will be worked on by the Dokka team
• jetbrains so far have mostly used a single @sample tag or code snippets for location specific samples and then a separate tool to extract the code to be able to compile it
  • This doesn't work well for androidx as our samples often use imports that are broader than what the library itself depends on (e.g. compose foundation giving an example how to use this composable with compose material library)
• Dokka team at the moment is busy with K2 work
• Jetbrains agrees that multiple samples that are location specific make sense as a usecase
  • https://github.com/Kotlin/dokka/issues/3055 will be used to discuss what the future for location sensitive samples should be
  • Implementation will take a while as it will likely require changes to Intellij, UAST/PSI, Dokka, Dackka (our doc tool)

Would ktfmt be willing to take a PR to add orderDocTags as an option to the formatter so we could adopt ktfmt without forking/ugly hacks?

[davidtorosyan]

Thanks for the update @liutikas! Great to hear from jetbrains about it too.

Would ktfmt be willing to take a PR to add orderDocTags as an option to the formatter so we could adopt ktfmt without forking/ugly hacks?

I think @cgrushko can probably speak to this better than I can, but my understanding is that we want to keep options to an absolute minimum.

If the spec was updated (or committed to be updated) to support inline sample tags, then it seems like a no-brainer that we'd implement that.

But since you're asking for an option, that tells me you'd want this ability even if it doesn't make it into the spec. Am I understanding right? My only question there is - what scenario would be supporting exactly? If none of the existing tools support rendering inline sample tags, what use would it be for ktfmt to support it?

---

That said, I think there's some precedent from #17, where @strulovich added a flag to disable optimizing imports.

Basically, we could take the stance that ktfmt can be opinionated when it comes to spaces and newlines, but actually reordering whole lines is optional and can be disabled. That's already the case for imports, and maybe we can have it be the case for kdoc too.

Thoughts?

[liutikas]

I was curious if yall are willing to add an option to disable tag reordering?

We finally migrated to ktfmt and @sample reordering is causing us issues.

[davidtorosyan]

I think my question from last time still stands:

what scenario would be supporting exactly? If none of the existing tools support rendering inline sample tags, what use would it be for ktfmt to support it?

One thing I can suggest is ditching @sample tags entirely and just using paragraphs. If standard tooling doesn't support inline tags, not sure if it makes sense for us to.

cc @hick209

[liutikas]

what scenario would be supporting exactly? If none of the existing tools support rendering inline sample tags, what use would it be for ktfmt to support it?

In androidx, we use a dokka-devsite-plugin built on top of dokka. This tool is used to generate d.android.com documentation for all of androidx code. dokka-devsite-plugin is also used by Android Gradle Plugin, Google Play Service and few other projects to generate their public facing user documentation. As androidx.compose developed, we found that supporting inline and more than one @sample was extremely helpful for complex methods, for example rememberSavable (source) (rendered documentation).

Since, we have also added support to Android Studio to render @sample tags (https://proandroiddev.com/finally-working-samples-in-android-studio-7edd8bd05bba) which doesnt fix the order, but does support rendering of multiple sample tags.

Have we made a mistake forking kdoc that had a vague spec for this? Possibly! Sadly, we have a huge codebase now, with close to a thousand @sample tags that we cannot change without making documentation worse for library users.

[omarismail94]

I created https://github.com/facebook/ktfmt/pull/474 to disable ordering the doc tags, following a similar route to that in disabling unused imports. This is how our Kdoc would look like:

The only diff it makes is removing the space between the last @sample and the first @param, which I think we can live with (please correct me if im wrong @liutikas )

Reading the discussion above, this seems like the path of least resistance?

[davidtorosyan]

@omarismail94 thanks for contributing! That said, right now my thinking is to go the route of special-casing @sample. That way users wouldn't have to choose between using @sample and any kdoc ordering.

@liutikas I want to check two things:

1. Would that change work for you? i.e. instead of treating @sample as true tag when formatting, treat it as a paragraph? Other tags would still get reordered.
2. Any interest in implementing this? If not we'll give it a shot, but it might take some time to get to.

Once we have the code / results to compare we can make the final decision, but I think there's enough utility here to warrant the special case.

cc @hick209

[liutikas]

1. Would that change work for you? i.e. instead of treating @sample as true tag when formatting, treat it as a paragraph? Other tags would still get reordered.

Yes, it would work and in fact be a more preferred solution.

2. Any interest in implementing this? If not we'll give it a shot, but it might take some time to get to.

Can you ballpark estimate how large of a task would this be?

[omarismail94]

I have an alternate PR, https://github.com/facebook/ktfmt/pull/475, to treat sample tags as a paragraph. Let me know what your thoughts are!

These are the changes that would be made to our AndroidX codebase as a result: https://android-review.git.corp.google.com/c/platform/frameworks/support/+/3117811 , (they don't affect rendering)

[davidtorosyan]

Thanks @omarismail94! I actually just implemented this in 2ba3b097656a8447e1c14c6f52a5854a5aac939b, in which I classify @sample as a priority tag.

Priority tags appear before other tags, but only if they're before them prior to formatting.

@liutikas I hope this will work for your use case. Let me know if you can do any validation on your side (I at least checked the example you provided) and we can work on releasing a new version.

[omarismail94]

@davidtorosyan I just pulled your change and ran locally. Besides the tests failing, the change still isn't working for us. See screenshot below of diff:

The change causes a lot of movement in our codebase, see here for diff: https://android-review.git.corp.google.com/c/platform/frameworks/support/+/3118751

[davidtorosyan]

@omarismail94 I just fixed the tests with: babb277d0cbb795f2173fcc98cc39e6ed278b102

See screenshot below of diff:

I thought there might be cases like this. I'm not sure if that particular example makes sense to preserve though. Please note that @see (and all other tags) are multiline, and all rendering engines I know of would group Example usage under the @see. This is probably not useful for anyone, and I don't think you could resolve this by any change specific to the @sample tag.

The case in the OP on the other hand has clear utility.

The change causes a lot of movement in our codebase, see here for diff:

I don't have access to that link. In general I don't think movement is bad thing, you'd expect that when turning on a formatter. I'm more worried about if the end result is worse or not. Could you provide some more screenshots maybe?

[omarismail94]

@davidtorosyan I think the change works ! Below are examples of the type of diffs Ive seen. If the line seperator can't be re-added, when do you think you'd be able to release a new version with this change?

---

• separator between @sample and Kdoc list now removed. Personally, I think it's cleaner with the new line separating. Is there a way to get that back?

---

• @see is now at the bottom - follows how renderer outputs html

---

I thought there might be cases like this. I'm not sure if that particular example makes sense to preserve though. Please note that @see (and all other tags) are multiline, and all rendering engines I know of would group Example usage under the @see. This is probably not useful for anyone, and I don't think you could resolve this by any change specific to the @sample tag.

Our intended function was probably to have Example usage above the sample tag, not in the @see block. Excellent catch! We will fix this by making @sample inlined so it outputs properly.

[omarismail94]

Can we add:

            prev.text.startsWith("@sample") -> true

In ParagraphListBuilder to get the new line back?

[davidtorosyan]

@omarismail94 if we do this:

separator between @sample and Kdoc list now removed. Personally, I think it's cleaner with the new line separating. Is there a way to get that back?

It should be by inserting a newline between priority tags and other tags. i.e. @sample shouldn't get a newline when it's inlined with other tags.

That said I don't think special casing that is worth it. It shouldn't affect rendering, and IMO would be a controversial style change. Our policy is to avoid changing the formatting for style preference (e.g. placing a newline between tags or not).

when do you think you'd be able to release a new version with this change?

I think we can work on this now, will update soon.

[omarismail94]

Great, thanks!!

[hick209]

Just created this release here https://github.com/facebook/ktfmt/releases/tag/v0.50

Closing issue, but please let us know if you are still facing issues

[omarismail94]

Thanks @hick209 and @davidtorosyan for pushing this through! JAR works great! Hope to see it as part of the IntelliJ plugin soon

[hick209]

Good call out @omarismail94, let me check why it was not published alongside the release