Skip to content

refactor(rust): enforce style guide rules for pub/module visibility - #4743

Open
chet wants to merge 1 commit into
NVIDIA:mainfrom
chet:gh-issue-4558
Open

refactor(rust): enforce style guide rules for pub/module visibility#4743
chet wants to merge 1 commit into
NVIDIA:mainfrom
chet:gh-issue-4558

Conversation

@chet

@chet chet commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

This enables the unreachable-pub lint for every Rust package, enforcing
the module visibility rules introduced in
#4522, and is the last PR following #4593, #4600, #4630, #4635, #4647, #4660, #4661, #4662, #4663, #4664, #4665, and #4682.

Default-feature builds exposed a small set of declarations that
all-feature builds kept reachable through test and benchmark APIs. This
scopes those declarations to their callers while keeping those
feature-gated APIs public only when their features are enabled.

Primary callouts are:

  • Add -D unreachable-pub beside -D warnings in the shared Cargo
    compiler flags so packages outside the workspace lint table are covered.
  • Narrow the remaining API Core, BMC mock, Health, NVLink manager, and
    carbide-lints declarations to the visibility their callers require.
  • Keep the Health benchmark and NVLink test-support APIs public only when
    their features are enabled, without adding lint allowances.
  • Update the custom-lint fixture to compile under the same visibility
    rule it helps enforce.
  • Leave runtime behavior, wire contracts, and generated source unchanged.

Related issues

This supports #4558.

This supports the updated Rust visibility scoping guidelines in
STYLE_GUIDE.md, established in
#4522.

Type of Change

  • Add - New feature or capability
  • Change - Changes in existing functionality
  • Fix - Bug fixes
  • Remove - Removed features or deprecated functionality
  • Internal - Internal changes (refactoring, tests, docs, etc.)

Breaking Changes

  • This PR contains breaking changes

Testing

  • Unit tests added/updated
  • Integration tests added/updated
  • Manual testing performed
  • No testing required (docs, internal refactor, etc.)

Additional Notes

The workspace now contains 109 Rust packages, including 17 that do not
inherit the root workspace lint table. Configuring the compiler lint in
.cargo/config.toml covers those packages without adding a custom lint or
changing their manifests.

No new allow(unreachable_pub) exceptions are introduced. The existing
exceptions remain limited to the generated-code and ABI boundaries reviewed
in #4682. The Health benchmark
and NVLink test-support APIs instead use feature-specific visibility so their
public surface only exists in the configuration that exports it.

Local CodeRabbit and read-only Claude reviews completed before the final fix
round. The applicable Health default-build finding is incorporated; the final
feature-boundary changes also received a fresh read-only review.

This enables the `unreachable-pub` lint for every Rust package, enforcing
the module visibility rules introduced in
NVIDIA#4522, and is the last PR
following NVIDIA#4593,
NVIDIA#4600,
NVIDIA#4630,
NVIDIA#4635,
NVIDIA#4647,
NVIDIA#4660,
NVIDIA#4661,
NVIDIA#4662,
NVIDIA#4663,
NVIDIA#4664,
NVIDIA#4665, and
NVIDIA#4682.

Default-feature builds exposed a small set of declarations that
all-feature builds kept reachable through test and benchmark APIs. This
scopes those declarations to their callers while keeping those
feature-gated APIs public only when their features are enabled.

Primary callouts are:
- Add `-D unreachable-pub` beside `-D warnings` in the shared Cargo
compiler flags so packages outside the workspace lint table are covered.
- Narrow the remaining API Core, BMC mock, Health, NVLink manager, and
`carbide-lints` declarations to the visibility their callers require.
- Keep the Health benchmark and NVLink test-support APIs public only when
their features are enabled, without adding lint allowances.
- Update the custom-lint fixture to compile under the same visibility
rule it helps enforce.
- Leave runtime behavior, wire contracts, and generated source unchanged.

This supports NVIDIA#4558.

This supports the updated Rust visibility scoping guidelines in
`STYLE_GUIDE.md`, established in
NVIDIA#4522.

Signed-off-by: Chet Nichols III <chetn@nvidia.com>
@chet
chet requested a review from a team as a code owner August 8, 2026 05:08
@coderabbitai

coderabbitai Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Summary by CodeRabbit

  • Refactor
    • Tightened internal API visibility across services, health monitoring, hardware mocks, and developer tooling without changing runtime behavior.
    • Improved separation between production, test, and benchmark interfaces.
  • Bug Fixes
    • Updated service validation tests to use the default configured chart version.
  • Tests
    • Refined compiler diagnostic fixtures and test-only builders for clearer access boundaries.
  • Chores
    • Enabled stricter Rust compiler linting to catch unintentionally exposed APIs.

Walkthrough

The PR enables the workspace-wide unreachable-pub lint and narrows internal APIs across API-core, health, NVLink, BMC mock, and lint crates. Test and benchmark interfaces retain scoped access where required.

Changes

Internal API visibility

Layer / File(s) Summary
Lint policy and API-core visibility
.cargo/config.toml, crates/api-core/src/*
The workspace denies unreachable-pub. API-core constants, types, methods, and test builders use restricted visibility. Xplane tests use the default service configuration.
Mock and health API boundaries
crates/bmc-mock/src/hw/*, crates/health/src/sink/*
The Lenovo rack mock and health event mappers use scoped visibility. OTLP constructors separate production, test, and benchmark access.
Switch certificate monitor execution paths
crates/nvlink-manager/src/switch_cert_monitor.rs
Production and test-support monitor APIs are separated. Shared construction and iteration logic injects result creation.
Carbide lint implementation visibility
lints/carbide-lints/src/*
Internal lint statics, types, methods, and helpers use crate or private visibility.
Lint fixture visibility expectations
lints/carbide-lints/tests/fixtures/app/src/*
The fixture narrows transaction and async method visibility. The expected diagnostic reflects the private declaration.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the primary change: enforcing Rust visibility rules through the unreachable-pub lint.
Description check ✅ Passed The description directly explains the lint enablement, visibility changes, feature boundaries, testing, and unchanged runtime behavior.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@chet

chet commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai full_review, thanks!

@coderabbitai

coderabbitai Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

@chet I will perform a complete new review of pull request #4743.

🐇🔍

✅ Action performed

Full review finished.

@chet
chet enabled auto-merge (squash) August 8, 2026 06:35
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.

1 participant