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

Switch proc macro docs to be on re-exports so we can have doc links to outer crate items #247

Open
sam0x17 opened this issue Oct 18, 2022 · 4 comments
Labels
C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. D0-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder. T1-FRAME This PR/Issue is related to core FRAME, the framework.

Comments

@sam0x17
Copy link
Contributor

sam0x17 commented Oct 18, 2022

Right now the frame proc macros (especially the pallet:: ones) follow the pattern of documenting proc macros inside their corresponding inner proc macro crates (i.e. frame_support_procedural). I standardized this in a number of places in paritytech/substrate#12333 / paritytech/substrate#12334.

A major reason for this is tools like rust-analyzer right now do not conform with the cargo doc behavior of prepending docs that are attached to re-exported items (i.e. macros, functions, traits, etc) and in fact completely ignore docs on re-exports (see my companion issue in rust-analyzer: rust-lang/rust-analyzer#13431).

This is why in paritytech/substrate#12334 I ended up switching all of the pallet:: macros to have their docs inside frame_support_procedural because then rust-analyzer will actually pick them up upon mouse-over.

The problem with this is inner crates can only generate doc links to items that are visible from the inner crate. For non-proc-macro crates the workaround is easy -- simply set up a circular reference between the inner crate and the outer crate so the outer crate items are visible/linkable from the inner crate. This doesn't work for proc macro crates, however, because rust restricts proc macro crates so that they are only allowed to publicly export proc macros (no other items are allowed), so even if we pub use outer_crate::* to make all the outer crate's items accessible within the inner crate, this will fail to compile since we are trying to export something other than a proc macro in a proc macro crate.

Thus we are at a bit of a stand-still. Ideally rust-analyzer would handle docs on re-exports properly the way cargo doc does and we would switch to only documenting proc macros where their re-exports are defined in the outer crate.

So this is blocked until rust-lang/rust-analyzer#13431 is addressed in rust-analyzer or some other solution (like better support for inter-crate doc linking) is added to rust.

One possible alternative solution would be to link directly to the docs.rs links for the outer items from the inner macro declarations. This has some obvious downsides (like rust-analyzer will open a web browser instead of following the links inline), but if this issue sits for a long period of time this might be the way to go.

Note: the one thing I haven't tried is trying to pub use the outer crate items in the inner proc macro crate while gating this with a #[cfg(doc)] directive. I doubt it will compile but it's the only thing I haven't tried so would be cool if someone could confirm (or if someone knows) that this doesn't work.

Related Issues

Related PRs

@bkchr
Copy link
Member

bkchr commented Oct 19, 2022

IMO the primary goal should be there to have proper cargo doc output and ignore rust-analyzer. Yes it is bad that this doesn't work, but this is a bug on their side and we should not scarify good docs for this.

@sam0x17
Copy link
Contributor Author

sam0x17 commented Oct 19, 2022

The pre-#12334 structure was that all of the pallet:: macros lacked any kind of stub (because the macro expansion of the pallet macro actually removes all these attributes) so all of their docs were on the docs for the pallet attribute macro.

paritytech/substrate#12334 introduced stubs for these macros with effectively duplicated docs from the pallet page which improved the developer experience both within cargo docs and for those using rust-analyzer. For those using cargo docs, now these macros actually come up in search as macros, and for rust-analyzer, hovering over any of these attributes displays the docs for that attribute from its respective stub and right clicking any of these attributes allowed you to "Go to Definition" without it failing like it did before.

Currently, the version of the docs that is on the pallet macro has working doc links to all the associated items that are mentioned in the docs, just like before. The copy of the docs that is on each stub has been largely stripped of links except those that link to other macros since we can only link to things within frame_support_procedural, but these links are all left in place on the copy of the docs that is on the pallet macro.

IMO this is the best compromise dev UX-wise until rust-analyzer fixes their issue, especially since one of the stated purposes of solving paritytech/substrate#12333 was to get rust-analyzer to behave properly with the pallet:: attributes for dev ux reasons. This was all discussed at length on paritytech/substrate#12333 and paritytech/substrate#12334 and the approach was approved without contest.

In the interim while we wait for rust-analyzer to fix their issue, it is true that we could move the docs for these stubs from frame_support_procedural to the pub use statements in frame_support and remove the extra docs entirely from the pallet macro (instead using doc links to link to all of the pallet:: attributes where we currently have that list of all the attributes), but this would result in a broken experience for users of rust-analyzer for the small benefit that the docs at least aren't duplicated in two places, so I say definitely not worth it for now.

I'm watching this closely and just want to keep this issue open until rust-analyzer releases their fix for this, and I have no problem taking responsibility for watching for that and managing this.

@bkchr
Copy link
Member

bkchr commented Oct 19, 2022

I don't assume that rust-analyzer will fix this in the near future. It is said that this requires some deeper changes in rust-analyzer. I can not imagine what their todo list looks like. However, we should not wait. Rust docs is more important. We could either copy the docs for now, if hover support is really that important (I doubt, because a lot of stuff is breaking all the time in rust analyzer because of the amount of macros etc we use). Or we just add some dummy docs mentioning to the user that this currently doesn't work in rust-analyzer and where they find the real docs.

@sam0x17
Copy link
Contributor Author

sam0x17 commented Oct 19, 2022

Right but we already did the copy that was part of paritytech/substrate#12334 so we should be good for now just wanted to describe this issue somewhere

@juangirini juangirini transferred this issue from paritytech/substrate Aug 24, 2023
@the-right-joyce the-right-joyce added C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. T1-FRAME This PR/Issue is related to core FRAME, the framework. D0-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder. and removed Z6-mentor labels Aug 25, 2023
bkchr pushed a commit that referenced this issue Sep 25, 2023
#1689)

Small tweak to #1642 to
incorporate the ideas from
#247.

I think this is the good middle ground, where we have good rust-docs,
and the RA users will also have some hope.

cc @wentelteefje @aaronbassett @sam0x17
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this issue Mar 25, 2024
paritytech#1689)

Small tweak to paritytech#1642 to
incorporate the ideas from
paritytech#247.

I think this is the good middle ground, where we have good rust-docs,
and the RA users will also have some hope.

cc @wentelteefje @aaronbassett @sam0x17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. D0-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder. T1-FRAME This PR/Issue is related to core FRAME, the framework.
Projects
Status: Backlog
Development

No branches or pull requests

5 participants