fix(eslint-rules): base-hook-no-forbidden-runtime - analyze base hook dependencies per symbol, not per file - #36498
Open
Hotell wants to merge 4 commits into
Open
Conversation
📊 Bundle size report✅ No changes found |
|
Pull request demo site: URL |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes @nx/workspace-base-hook-no-forbidden-runtime by switching from file-level transitive dependency analysis (which missed relative imports and conflated sibling exports) to symbol-level analysis that follows only the identifiers a referenced symbol actually depends on, improving correctness for barrels and re-exports.
Changes:
- Remove the
watchedPackagesallowlist and analyze all imports (including relative and subpath specifiers) for forbidden runtime reach. - Replace file-reach DFS with a memoized, cycle-safe symbol walk (
findForbiddenRuntime) that respects value-vs-type positions and keeps barrels transparent. - Expand fixtures and tests to cover relative multi-hop chains, barrel transparency, wrapper-package benign vs deep exports,
.d.tstraversal, and subpath-specifier normalization.
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tools/eslint-rules/rules/base-hook-no-forbidden-runtime.ts | Refactors rule to compute forbidden-runtime reach per symbol (not per file) and removes watchedPackages. |
| tools/eslint-rules/rules/base-hook-no-forbidden-runtime.spec.ts | Updates/expands tests to validate symbol-level behavior and new coverage cases. |
| tools/eslint-rules/rules/fixtures/base-hook-no-forbidden-runtime/tsconfig.json | Adds path mappings for new stub packages and subpath specifiers used in tests. |
| tools/eslint-rules/rules/fixtures/base-hook-no-forbidden-runtime/stubs/wrapper-pkg/index.ts | New wrapper-package barrel used to validate benign vs deep exports. |
| tools/eslint-rules/rules/fixtures/base-hook-no-forbidden-runtime/stubs/wrapper-pkg/useBenign.ts | Benign wrapper export fixture. |
| tools/eslint-rules/rules/fixtures/base-hook-no-forbidden-runtime/stubs/wrapper-pkg/useBenignRef.ts | Benign dependency hop fixture. |
| tools/eslint-rules/rules/fixtures/base-hook-no-forbidden-runtime/stubs/wrapper-pkg/useDeep.ts | Deep wrapper export fixture that reaches forbidden runtime. |
| tools/eslint-rules/rules/fixtures/base-hook-no-forbidden-runtime/stubs/wrapper-pkg/useDeepInner.ts | Inner hop that directly imports forbidden runtime. |
| tools/eslint-rules/rules/fixtures/base-hook-no-forbidden-runtime/stubs/watched-pkg/index.ts | Adjusts watched package barrel types to support new test scenarios. |
| tools/eslint-rules/rules/fixtures/base-hook-no-forbidden-runtime/stubs/watched-pkg/heavy.ts | Updates heavy type to embed forbidden-runtime type and adds clean sibling type. |
| tools/eslint-rules/rules/fixtures/base-hook-no-forbidden-runtime/stubs/unlisted-pkg/index.ts | Adds unlisted package fixture to ensure non-allowlisted packages are still analyzed. |
| tools/eslint-rules/rules/fixtures/base-hook-no-forbidden-runtime/stubs/typed-dist-pkg/index.d.ts | Adds .d.ts fixture to validate declaration-file traversal for type coupling. |
| tools/eslint-rules/rules/fixtures/base-hook-no-forbidden-runtime/stubs/relay-pkg/index.ts | Adds relay package fixture to validate benign export remains clean across boundaries. |
| tools/eslint-rules/rules/fixtures/base-hook-no-forbidden-runtime/stubs/heavy-runtime/sub.ts | Adds subpath export fixture to validate package-name normalization for forbidden runtimes. |
| tools/eslint-rules/rules/fixtures/base-hook-no-forbidden-runtime/stubs/barrel-pkg/index.ts | Adds barrel package fixture with clean + dirty sibling exports. |
| tools/eslint-rules/rules/fixtures/base-hook-no-forbidden-runtime/stubs/barrel-pkg/clean.ts | Clean sibling export fixture. |
| tools/eslint-rules/rules/fixtures/base-hook-no-forbidden-runtime/stubs/barrel-pkg/dirty.ts | Dirty sibling export fixture that imports forbidden runtime. |
| tools/eslint-rules/rules/fixtures/base-hook-no-forbidden-runtime/src/local-trigger.ts | Adds local relative re-export fixture used in multi-hop relative chain test. |
| tools/eslint-rules/rules/fixtures/base-hook-no-forbidden-runtime/src/local-heavy.ts | Adds local module that imports forbidden runtime for the relative-chain test. |
| tools/eslint-rules/rules/fixtures/base-hook-no-forbidden-runtime/src/local-light.ts | Adds local module that stays clean for relative-chain “valid” coverage. |
| tools/eslint-rules/rules/fixtures/base-hook-no-forbidden-runtime/src/local-barrel.ts | Adds local barrel fixture to validate folder-barrel transparency. |
… file `base-hook-no-forbidden-runtime` had two defects that made it miss the leaks it exists to catch while reporting ones that were not real. 1. Imports were only tracked when the module specifier exactly matched a fixed `watchedPackages` allowlist. Relative imports and unlisted packages were never analyzed, so `useDropdownBase_unstable` -> `./useButtonTriggerSlot` -> `../../utils/useTriggerSlot` -> `@fluentui/react-tabster` went undetected. 2. Reach was computed from the *file* declaring the imported symbol. A binding therefore inherited every dependency of its defining module and of every barrel it was re-exported through, e.g. importing `useARIAButtonProps` from `@fluentui/react-aria` inherited `useActiveDescendant` -> tabster. Reach is now computed from the symbol's own declaration, following only the identifiers that declaration references. Alias hops resolve through barrels to the leaf declaration, so a clean export no longer inherits its siblings' dependencies. Forbidden runtimes are detected at module specifier boundaries and via the leaf declaration's owning package. `watchedPackages` is removed; every import is analyzed. The walk deliberately stops at namespace bindings (`import * as ns`) and whole-module symbols, which bind an entire module rather than a symbol. Declaration files are traversed, so type coupling is still detected when a package resolves to built output. Also fixes subpath specifiers (`tabster/sub`) not being normalized to the package name, and memoizes results per binding and per Program. Repo-wide this moves the rule from 0 reported violations to 6, all with verified chains into `tabster`. Measured cost is ~90ms per package (3.3% of rule time).
The fixed `base-hook-no-forbidden-runtime` rule surfaces 6 pre-existing violations that would otherwise put master in a failing state on merge. Suppress them at the reference site so the directives disappear on their own once the underlying dependency is moved, rather than disabling the rule at file or config level. Four of the five directives share one root cause: `useTriggerSlot` calls `useTabsterAttributes`, which reaches `tabster` through `useTabster`. The fifth is `AvatarGroupPopover` state/props embedding Popover, which reaches `tabster` through `useModalAttributes`. No behavior change; comments only.
… symbol A type reference followed both type *and* value positions, so crossing a `typeof SomeComponent` boundary turned an API-surface question into a walk of that component's implementation. `AvatarGroupPopoverBaseProps` was reported as tabster-dependent purely because `AvatarGroupPopoverSlots` declares `popoverSurface: NonNullable<Slot<typeof PopoverSurface>>` and the walk continued into `usePopoverSurface_unstable` -> `useModalAttributes`. Extracting a component's type does not consume its runtime. The modes are now symmetric and non-mixing: a runtime query follows only value positions, a type query follows only type positions. Direct forbidden imports are still caught in type mode, since the module edge is checked before any walking. react-avatar no longer needs a suppression and is untouched by this change.
Upstream microsoft#36497 separated the tabster logic from these base hooks, so all four directives are now unused and the accompanying `none` change files are redundant.
Hotell
force-pushed
the
fix/base-hook-rule-transitive-tracking
branch
from
August 4, 2026 15:45
1a19f5c to
7a52b4c
Compare
Hotell
marked this pull request as ready for review
August 4, 2026 16:25
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Previous Behavior
@nx/workspace-base-hook-no-forbidden-runtimereported 0 violations across the whole repo, including the one it was written to catch:Two defects caused this.
1. Imports were only tracked when the specifier exactly matched a
watchedPackagesallowlist.trackedPackageswas{'@fluentui/react-tabster', 'tabster'}, so relative imports and every other package were never analyzed. The transitive machinery could only fire when the directly imported package was already@fluentui/react-tabster— i.e. only for the case a plainno-restricted-importsalready covers. Subpath specifiers (tabster/sub) also slipped through, since the check compared the raw specifier rather than the package name.2. Reach was computed from the file declaring the imported symbol, not the symbol.
A binding inherited every dependency of its defining module and of every barrel it was re-exported through.
useButton.tsimportsuseARIAButtonPropsfrom@fluentui/react-aria; because the react-aria barrel also exportsuseActiveDescendant,useButtonBase_unstablewas reported as tabster-dependent. Same for a symbol referencing an unrelated sibling export of its own file.New Behavior
Reach is computed per symbol. The walk starts at the symbol's own declaration and follows only the identifiers that declaration references. Alias hops resolve through barrels to the leaf declaration, so a clean export never inherits its siblings' dependencies. Forbidden runtimes are detected at module-specifier boundaries and via the leaf declaration's owning package (catching
export { x } from 'tabster'chains thatgetAliasedSymbolcollapses).watchedPackagesis removed — every import is analyzed, relative ones included.Deliberate bounds: namespace bindings (
import * as ns) and whole-module symbols stop at the specifier check, since they bind a module rather than a symbol. Without this,React.useRefwalks into React's type graph and hits tabster's declaration-merged augmentations, producingcannot reference `*` from `react` because `*` depends on forbidden runtime `tabster`. Declaration files are traversed, so type coupling is still caught when a package resolves to built output.forbiddenRuntimesstays['tabster']. Wrapper packages are intentionally not banned: at symbol granularity,useOnKeyboardNavigationChange->useKeyborgRef->keyborgis legitimately clean, whileuseTabsterAttributes->useTabster->tabsteris not. Banning@fluentui/react-tabsterwholesale would reintroduce exactly the conflation this PR removes.Findings
Rebased onto master after #36497, which separated the tabster logic from these base hooks. The rule now reports zero violations repo-wide and this PR needs no suppressions and touches no published package.
The chains it found before that fix, each verified by hand:
react-comboboxuseDropdown.tsx:75useButtonTriggerSlotreact-tabster/src/hooks/useTabster.tsreact-comboboxuseCombobox.tsx:76useInputTriggerSlotreact-tabster/src/hooks/useTabster.tsreact-tag-pickeruseTagPickerButton.tsx:37useButtonTriggerSlotreact-tabster/src/hooks/useTabster.tsreact-tag-pickeruseTagPickerInput.tsx:79useInputTriggerSlotreact-tabster/src/hooks/useTabster.tsAll four shared one root cause —
useTriggerSlotcallinguseTabsterAttributes— which is what #36497 addressed.Everything keyborg-only stayed correctly silent throughout, verified by reading the react-tabster sources:
useActiveDescendant,useListboxSlot,Listbox,useFocusWithin,useIsNavigatingWithKeyboard,createFocusOutlineStyle.Performance
The symbol walk is memoized per binding and per
ts.Program. Measured onreact-combobox(66 files) withTIMING=15 npx eslint src:@typescript-eslint/no-deprecatedreact-hooks/static-componentscompat/compat@nx/workspace-base-hook-no-forbidden-runtime@typescript-eslint/naming-conventionimport/no-extraneous-dependenciesreact-hooks/rules-of-hooksEnd-to-end A/B on the same package:
npx eslint srcwith rule offnpx eslint srcwith rule on~190ms, ~2% of lint time. For context, ~4.7s of that 9s is fixed ESLint startup + TS Program construction (measured by linting a single file), and two unrelated rules account for 76% of all rule execution time.
Tests
98 passing. New coverage for the cases that previously had none:
./local-trigger->./local-heavy-> runtime) — theuseDropdownshapeuseBenign->useBenignRef-> benign dep) vs a deep one (useDeep->useDeepInner-> runtime) — the keyborg/tabster splituseActiveDescendantshape.d.tsExisting type-leak fixtures were reshaped:
HeavyTypenow genuinely embeds a forbidden-runtime type instead of merely living in a file that imports one.Type positions vs value positions
An earlier iteration of this PR also flagged
react-avatar. That was a false positive worth calling out, because it is the same class of bug as the file-level reach this PR replaces.AvatarGroupPopoverSlotsdeclaresand the walker crossed that
typeofboundary intoPopoverSurface's implementation, reachinguseModalAttributes->tabster. Extracting a component's type does not consume its runtime.The two modes are now symmetric and never mix: a runtime query follows only value positions, a type query follows only type positions. Direct forbidden imports are still caught in type mode, since the module edge is checked before any walking.
stubs/component-pkglocks both directions —typeof Widgetin a props type is valid, referencingWidgetas a value is still reported.Verification
yarn nx run-many -t lint --projects=tag:vNextexits 0 with 0 errors and 0 occurrences of the rule.Notes
tools/eslint-rulesisprivate: trueandpackages/eslint-pluginis untouched.packages/andchange/diff zero againstupstream/master— this PR is confined totools/eslint-rules/.packages/eslint-plugin/src/internal.jsis unchanged ('error', no options).Known limitations (follow-ups)
import()is not followedimport './x') have no identifier to resolve, so they need a separate per-file passns.foo) is not resolved; trivial to add, but needs areact/@typesguard first or the React type-graph false positives returnRelated Issue(s)