Docstrings for proc macros

[ptitjes]

No description provided.

[jplatte]

Thanks for working on this!

Maybe you're already aware, but there is an alternate PR at #1498 for supporting something like this. As per comments there, I prefer your approach though (as a frequent contributor, not a maintainer) so I'm glad there's now a PR for it :)

[ptitjes]

Thanks for working on this!

You're welcome! Please note that I am a very new Rust coder.

Maybe you're already aware, but there is an alternate PR at #1498 for supporting something like this. As per comments there, I would prefer an approach like this though (as a frequent contributor, not a maintainer) so I'm glad there's now a PR for it :)

No, I missed it. But it indeed seems the approach used here is more straightforward.

[ptitjes]

@jplatte I took the liberty to add a commit to add .idea to .gitignore. Also added an entry to the unreleased changelog.

[jplatte]

cc @bendk @badboy

[badboy]

oh yey, I did the same work in the past couple of days and it looks very similar. Given this was posted before I could even get to it I will spend some time on reviewing this instead.

[badboy]

Except for some naming and ordering this is the same as my implementation that I did 2 days ago and didn't yet push.

So ... good job! Minimal nits.

[mhammond]

This is great, thanks! I've a minor preference that we reused the existing docstring fixture for procmacros too, just so as things move forward it's much easier to test and rationalize about any differences between procmacros and UDL - so if you think that makes sense and have the energy to do that it would be great, but I'm not going to try and block this landing for that.

[ptitjes]

oh yey, I did the same work in the past couple of days and it looks very similar. Given this was posted before I could even get to it I will spend some time on reviewing this instead.

I am really sorry if stepped on your toes. There was no issue open, so I jumped on it as I needed it.

[ptitjes]

just so as things move forward it's much easier to test and rationalize about any differences between procmacros and UDL

Not that I am too lazy, but I really think it is better to have separate fixture to easily compare the generated code with UDL or procmacros. As the symbols have the same names, you can easily diff and spot the differences. For example:

--- uniffi-fixture-docstring-49c2dd0b0f909ef/uniffi/fixture/docstring/uniffi_docstring.kt	2023-11-27 19:27:01.335778828 +0100
+++ uniffi-fixture-docstring-proc-macro-a3df46f603a93fe/uniffi/fixture/docstring/proc/macro/uniffi_docstring_proc_macro.kt	2023-11-27 19:27:12.867338022 +0100

...

@@ -999,6 +999,9 @@
      */
     data class Test(
         
+        /**
+         * <docstring-variant-field>
+         */
         val `code`: Short
         ) : AssociatedEnumTest() {
         companion object

BTW, I think we should improve the docstring use in templates. Fields' docstring should really be used as a @param in the parent symbol, at for Kotlin. But that is a job for another time. (I will create an issue.)

[ptitjes]

@badboy @mhammond @jplatte All changes done.

[ptitjes]

I forgot to commit the Cargo.lock. Now we should be good.

[skeet70]

Traits and trait methods get docstrings, and in the test code the implementer of the callback trait inherits the CallbackTest docstring. Does the same happen with a

#[uniffi::export]
pub trait NotCallbackTest {
  /// <docstring-trait-method>
  fn other_test(&self);
}

#[uniffi::export]
impl NotCallbackTest for Object {
  fn other_test(&self) {}
}

or does the method that shows up from the impl require its own duplicate docstring be defined?

[mhammond]

BTW, I think we should improve the docstring use in templates. Fields' docstring should really be used as a @param in the parent symbol, at for Kotlin. But that is a job for another time. (I will create an issue.)

Yeah - I agree - and that's why I think a single fixture is better. IMO, the fact that the existing fixture uses such trivial strings is actually a mis-feature - it will eventually need to grow into richer strings, so we can test newlines, indentation, whitespace, and as you mention, even more advanced things like @param. If we assume we want the same behaviour in both cases, it seems having 2 discrete fixtures will make life more complicated as they diverge.

Not that I am too lazy, but I really think it is better to have separate fixture to easily compare the generated code with UDL or procmacros. As the symbols have the same names, you can easily diff and spot the differences. For example:

I'm mildly skeptical this will work in the future. If we take the @param example, we'd need to be very careful that both fixtures were changed in a lock-step way to ensure that remained true. More likely in reality is that someone will submit a PR which touches only 1 - either because they are not aware of the other fixture at all, or not aware they should be kept so lock-step to enable this kind of manual diffing. A single fixture would make that less likely and even allow CI to help - it's much easier to test for invariants inside a single fixture than across 2 discrete ones.

(I'm still not too bothered and still not going to try and block this landing for this, but figured I'd expand on my thinking here.)

[badboy]

I am really sorry if stepped on your toes. There was no issue open, so I jumped on it as I needed it.

no worries, I started working on this on the weekend.

BTW, I think we should improve the docstring use in templates. Fields' docstring should really be used as a @param in the parent symbol, at for Kotlin. But that is a job for another time. (I will create an issue.)

Yeah - I agree - and that's why I think a single fixture is better. IMO, the fact that the existing fixture uses such trivial strings is actually a mis-feature - it will eventually need to grow into richer strings, so we can test newlines, indentation, whitespace, and as you mention, even more advanced things like @param. If we assume we want the same behaviour in both cases, it seems having 2 discrete fixtures will make life more complicated as they diverge.

I actually get what you mean and I'd be willing to tackle this separately after we merge this PR.

[badboy]

GitHub complained about merge conflicts even after I resolved them in the web UI, so I opted to rebase manually and force-push. All I did was clear up the merge conflict in changelog.md.