Skip to content

feat(xtask): Add check to ensure proc-macro generated user-space items are documented or hidden#5030

Merged
bugadani merged 1 commit intoesp-rs:mainfrom
AnthonyGrondin:chore/documentation-check-workflow
Mar 18, 2026
Merged

feat(xtask): Add check to ensure proc-macro generated user-space items are documented or hidden#5030
bugadani merged 1 commit intoesp-rs:mainfrom
AnthonyGrondin:chore/documentation-check-workflow

Conversation

@AnthonyGrondin
Copy link
Copy Markdown
Contributor

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

  • I have updated existing examples or added new ones (if applicable).
  • I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
  • My changes were added to the CHANGELOG.md in the proper section.
  • I have added necessary changes to user code to the latest Migration Guide.
  • My changes are in accordance to the esp-rs developer guidelines

Extra:

Pull Request Details 📖

Description

This is a naive attempt at checking rustdoc coverage on a proper binary, using all of the generated pro-macros to ensure that expanded code in the user's app is either documented or hidden.

Related: #1233

  • Add a CI-facing xtask check that runs cargo rustdoc --document-private-items --show-coverage on examples/async/embassy_hello_world.
  • Fail when private-item doc coverage is not 100.0%, catching undocumented macro-generated items in user-facing apps.
  • Mark generated __main module in esp-hal-procmacros as #[doc(hidden)] and document example tasks to keep baseline coverage passing.

Testing

Tested locally by adding and removing #[doc(hidden)].
Will need a proper CI run to ensure it's fully working on all targets.

Copilot AI review requested due to automatic review settings February 20, 2026 02:02
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds a new xtask CI check to detect undocumented (or not-#[doc(hidden)]) proc-macro-generated items by running cargo rustdoc --document-private-items --show-coverage against a representative end-user binary example.

Changes:

  • Run a new CI-facing xtask step that executes cargo rustdoc on examples/async/embassy_hello_world and enforces 100% private-item doc coverage.
  • Update the embassy hello world async example to document an #[embassy_executor::task] function.
  • Hide the proc-macro generated __main module in esp-hal-procmacros with #[doc(hidden)] to keep rustdoc coverage clean.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
xtask/src/main.rs Adds the new “generated doc artifacts” rustdoc coverage check to CI checks.
examples/async/embassy_hello_world/src/main.rs Documents a task function to satisfy private-item doc coverage.
esp-hal-procmacros/src/rtos_main.rs Marks generated __main module as #[doc(hidden)] to avoid coverage failures from macro output.

Comment thread xtask/src/main.rs Outdated
Comment thread xtask/src/main.rs Outdated
Comment thread xtask/src/main.rs Outdated
@AnthonyGrondin AnthonyGrondin force-pushed the chore/documentation-check-workflow branch 2 times, most recently from bb303b2 to 99460f4 Compare February 24, 2026 18:15
Copilot AI review requested due to automatic review settings March 4, 2026 17:16
@AnthonyGrondin AnthonyGrondin force-pushed the chore/documentation-check-workflow branch from 99460f4 to 15383d2 Compare March 4, 2026 17:16
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Comment thread xtask/src/main.rs Outdated
Comment thread xtask/src/main.rs Outdated
@AnthonyGrondin AnthonyGrondin force-pushed the chore/documentation-check-workflow branch from 15383d2 to 0effe5f Compare March 4, 2026 18:41
Comment thread xtask/src/main.rs Outdated
@bjoernQ
Copy link
Copy Markdown
Contributor

bjoernQ commented Mar 5, 2026

the proc-macro changes look useful - not sure if just having unit tests for the macros wouldn't be enough

@bugadani
Copy link
Copy Markdown
Contributor

bugadani commented Mar 5, 2026

Also please add a line about this to the developer guidelines, maybe copilot is smart enough to catch unwanted changes early.

@AnthonyGrondin
Copy link
Copy Markdown
Contributor Author

the proc-macro changes look useful - not sure if just having unit tests for the macros wouldn't be enough

I'm not sure if this is easily feasible. Ultimately, it's what ends up in the final binary that matters. The current tests verify that the proc-macro matches a specific string, but documentation coverage is a bit more broad.

Copilot AI review requested due to automatic review settings March 5, 2026 19:08
@AnthonyGrondin
Copy link
Copy Markdown
Contributor Author

This makes me wonder if we should have a specifically tailored package to cover #[ram], #[handler], #[esp_rtos::main] all at once.

@AnthonyGrondin AnthonyGrondin force-pushed the chore/documentation-check-workflow branch from 49390a2 to 056aab7 Compare March 5, 2026 19:12
Comment thread .github/workflows/ci.yml Outdated
@bugadani
Copy link
Copy Markdown
Contributor

bugadani commented Mar 5, 2026

I don't know... you're testing a set of macros, and nothing really guarantees that we won't add more, that won't be tested. At that point, doc coverage is essentially the same as the unit tests - we need to keep something in mind in order to test it, but the extra check just brings in extra complication and takes time to run.

Not to mention this uses 2 unstable features so it's pretty much guaranteed to break at some point for one reason or another.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@AnthonyGrondin AnthonyGrondin force-pushed the chore/documentation-check-workflow branch from 056aab7 to 24a8c77 Compare March 5, 2026 19:24
@AnthonyGrondin
Copy link
Copy Markdown
Contributor Author

I don't know... you're testing a set of macros, and nothing really guarantees that we won't add more, that won't be tested. At that point, doc coverage is essentially the same as the unit tests - we need to keep something in mind in order to test it, but the extra check just brings in extra complication and takes time to run.

Not to mention this uses 2 unstable features so it's pretty much guaranteed to break at some point for one reason or another.

Yeah. This is quite an unstable situation, with no strong verification barriers. The current checked binary doesn't cover #[handler] but in my personal crate I saw it lacked documentation. If needed we can also introduce a binary in qa-test that spams all macros to cover all expansions.

The current architecture allows future extension like enforcing documentation coverage for all stabilized public items, if that's desired.

@github-actions
Copy link
Copy Markdown

New commits in main has made this PR unmergable. Please resolve the conflicts.

@github-actions github-actions Bot added the merge-conflict Merge conflict detected. Automatically added/removed by CI. label Mar 17, 2026
Copilot AI review requested due to automatic review settings March 17, 2026 14:08
@AnthonyGrondin AnthonyGrondin force-pushed the chore/documentation-check-workflow branch from 24a8c77 to d0575bd Compare March 17, 2026 14:08
@github-actions github-actions Bot removed the merge-conflict Merge conflict detected. Automatically added/removed by CI. label Mar 17, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.

Comment thread .github/workflows/ci-nightly.yml Outdated
Comment thread xtask/src/main.rs Outdated
Comment thread xtask/src/main.rs Outdated
Comment thread esp-hal-procmacros/src/interrupt.rs
Comment thread .github/workflows/ci-nightly.yml Outdated
@AnthonyGrondin AnthonyGrondin force-pushed the chore/documentation-check-workflow branch from d0575bd to 11671a4 Compare March 17, 2026 17:04
Copy link
Copy Markdown
Contributor

@bugadani bugadani left a comment

Choose a reason for hiding this comment

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

If you would please undo the CI and xtask changes, we could perhaps move forward here.

@AnthonyGrondin
Copy link
Copy Markdown
Contributor Author

What do you mean? I thought we wanted this to run in nightly CI?

@bugadani
Copy link
Copy Markdown
Contributor

Because the CI check needs to know about the potentially problematic macros, there's no point in running the CI check in the first place. The unit tests should be sufficient, I don't see the value in the complexity added to the xtask.

Copilot AI review requested due to automatic review settings March 18, 2026 14:51
@AnthonyGrondin AnthonyGrondin force-pushed the chore/documentation-check-workflow branch from 11671a4 to c2cc1c4 Compare March 18, 2026 14:51
…` proc-macro

- Add missing `#[doc(hidden)]` for the `__main` module generated by the
`esp_rtos::main` macro
@AnthonyGrondin AnthonyGrondin force-pushed the chore/documentation-check-workflow branch from c2cc1c4 to e856f1c Compare March 18, 2026 14:54
@AnthonyGrondin
Copy link
Copy Markdown
Contributor Author

Got it! Fixed and rebased

Copy link
Copy Markdown
Contributor

@bugadani bugadani left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

Comment thread esp-hal-procmacros/src/interrupt.rs
Comment thread esp-hal-procmacros/src/interrupt.rs
@bugadani bugadani enabled auto-merge March 18, 2026 14:58
@bugadani bugadani added this pull request to the merge queue Mar 18, 2026
Merged via the queue into esp-rs:main with commit 909db47 Mar 18, 2026
30 of 31 checks passed
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.

4 participants