Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

docs for pallet macros are hard to find because of how we parse pallet macros #12333

Closed
sam0x17 opened this issue Sep 23, 2022 · 3 comments · Fixed by #12334
Closed

docs for pallet macros are hard to find because of how we parse pallet macros #12333

sam0x17 opened this issue Sep 23, 2022 · 3 comments · Fixed by #12334
Assignees
Labels
C1-low PR touches the given topic and has a low impact on builders. Z1-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder

Comments

@sam0x17
Copy link
Contributor

sam0x17 commented Sep 23, 2022

If you look at the majority of the pallet:: macros, you'll notice most of them don't actually have an explicit macro definition associated with them anywhere. This is because we use these macros essentially as markers that are picked up, acted upon, and removed by the pallet expansion macros. They don't actually exist anywhere in the code as macros in their own right.

This works great for our purposes, but it is very confusing and it makes it very difficult to track down docs for specific pallet macros because we don't create any kind of blank macro stub for these macros, since at compile-time the AST nodes associated with them actually get removed, so we can get away with never actually defining them. This also means there is no good place to put docs for these macros.

Instead we just dump all docs for pallet macros in the frame/support/src/lib.rs file and hope that people will find it. This has multiple unfortunate side effects:

  1. In the cargo-generated docs, users are confused to find that there is no macro with the name they are looking for, so it is hard to find out what it does unless you happen to know where the docs for it are. As a new developer to the substrate ecosystem I found this very confusing and I actually wasted a few hours before I realized what was going on.
  2. People who are using rust-analyzer and similar static analysis tools will find that when they hover over one of these macro calls, no docs will appear, and right clicking and clicking "go to definition" will result in a message saying no definition could be found, which is even more infuriating.
  3. I suspect, though this is not confirmed, that rust analyzer is even slower when it has to search around for a bunch of macros that actually have no definition anywhere, so fixing this could provide a slight performance improvement to rust-analyzer when used on the substrate repo, though I make no promises for this one.

To resolve this, I propose that we add stubs for every documented pallet:: macro, and split up the docs in frame/support/src/lib.rs, attaching the docs directly to their respective macro stubs. These stubs would then be exported as part of the pallet prelude. I have experimentally confirmed that this approach is enough to make rust-analyzer and rust docs happy and sensical -- macros will get their docs displayed when you hover over them and in the substrate docs you get a nice list of all the pallet macros which in turn will now actually come up in search.

I already have this working in a branch just wanted to make sure we have consensus that this is the way we want to go before I do it for the rest of the pallet macros

@sam0x17 sam0x17 added Z1-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder C1-low PR touches the given topic and has a low impact on builders. labels Sep 23, 2022
@sam0x17 sam0x17 self-assigned this Sep 23, 2022
@sam0x17 sam0x17 changed the title docs for pallet macros are hard to find because of the unique way in which we parse pallet macros docs for pallet macros are hard to find because of how we parse pallet macros Sep 23, 2022
@sam0x17
Copy link
Contributor Author

sam0x17 commented Sep 23, 2022

rust analyzer perspective:

before:
Screen Shot 2022-09-23 at 2 01 56 AM

after:
Screen Shot 2022-09-23 at 1 25 18 AM

@bkchr
Copy link
Member

bkchr commented Sep 23, 2022

I like the idea.

IMO these stubs just really just be stubs, meaning they just forward the input tokens. We can then add a doc comment that these macros only work in the context of the pallet macro. This should be good enough imo.

@sam0x17
Copy link
Contributor Author

sam0x17 commented Sep 23, 2022

Yes and I think I'm also going to add a link or comment next to the stubs pointing to where the code related to that stub actually is just as a reference for programmers looking for it

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
C1-low PR touches the given topic and has a low impact on builders. Z1-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder
Projects
None yet
2 participants