Skip to content

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
microsoft:masterfrom
Hotell:fix/base-hook-rule-transitive-tracking
Open

fix(eslint-rules): base-hook-no-forbidden-runtime - analyze base hook dependencies per symbol, not per file#36498
Hotell wants to merge 4 commits into
microsoft:masterfrom
Hotell:fix/base-hook-rule-transitive-tracking

Conversation

@Hotell

@Hotell Hotell commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Previous Behavior

@nx/workspace-base-hook-no-forbidden-runtime reported 0 violations across the whole repo, including the one it was written to catch:

useDropdownBase_unstable
  -> ./useButtonTriggerSlot          (relative)
    -> ../../utils/useTriggerSlot    (relative)
      -> @fluentui/react-tabster useTabsterAttributes
        -> useTabster -> tabster

Two defects caused this.

1. Imports were only tracked when the specifier exactly matched a watchedPackages allowlist.

const source = node.source.value;
if (typeof source !== 'string' || !trackedPackages.has(source)) {
  return; // everything else dropped
}

trackedPackages was {'@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 plain no-restricted-imports already 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.ts imports useARIAButtonProps from @fluentui/react-aria; because the react-aria barrel also exports useActiveDescendant, useButtonBase_unstable was 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 that getAliasedSymbol collapses).

watchedPackages is 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.useRef walks into React's type graph and hits tabster's declaration-merged augmentations, producing cannot 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.

forbiddenRuntimes stays ['tabster']. Wrapper packages are intentionally not banned: at symbol granularity, useOnKeyboardNavigationChange -> useKeyborgRef -> keyborg is legitimately clean, while useTabsterAttributes -> useTabster -> tabster is not. Banning @fluentui/react-tabster wholesale 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:

Location Symbol Enters forbidden runtime at
react-combobox useDropdown.tsx:75 useButtonTriggerSlot react-tabster/src/hooks/useTabster.ts
react-combobox useCombobox.tsx:76 useInputTriggerSlot react-tabster/src/hooks/useTabster.ts
react-tag-picker useTagPickerButton.tsx:37 useButtonTriggerSlot react-tabster/src/hooks/useTabster.ts
react-tag-picker useTagPickerInput.tsx:79 useInputTriggerSlot react-tabster/src/hooks/useTabster.ts

All four shared one root cause — useTriggerSlot calling useTabsterAttributes — 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 on react-combobox (66 files) with TIMING=15 npx eslint src:

Rule Time (ms) Relative
@typescript-eslint/no-deprecated 1298.800 48.7%
react-hooks/static-components 738.624 27.7%
compat/compat 132.010 4.9%
@nx/workspace-base-hook-no-forbidden-runtime 87.984 3.3%
@typescript-eslint/naming-convention 70.976 2.7%
import/no-extraneous-dependencies 50.545 1.9%
react-hooks/rules-of-hooks 50.042 1.9%

End-to-end A/B on the same package:

Run Time
npx eslint src with rule off 8.97s
npx eslint src with rule on 9.16s

~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:

  • relative multi-hop chain (./local-trigger -> ./local-heavy -> runtime) — the useDropdown shape
  • package barrel and local folder barrel: clean export valid, dirty sibling still reported
  • wrapper package with a benign export (useBenign -> useBenignRef -> benign dep) vs a deep one (useDeep -> useDeepInner -> runtime) — the keyborg/tabster split
  • relay package consuming only the benign export — the useActiveDescendant shape
  • clean sibling in the same file as a forbidden import
  • symbol declared in a .d.ts
  • unlisted bare package, subpath specifier

Existing type-leak fixtures were reshaped: HeavyType now 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. AvatarGroupPopoverSlots declares

popoverSurface: NonNullable<Slot<typeof PopoverSurface>>;

and the walker crossed that typeof boundary into PopoverSurface's implementation, reaching useModalAttributes -> 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-pkg locks both directions — typeof Widget in a props type is valid, referencing Widget as a value is still reported.

Verification

yarn nx run-many -t lint --projects=tag:vNext exits 0 with 0 errors and 0 occurrences of the rule.

Notes

  • No change files: tools/eslint-rules is private: true and packages/eslint-plugin is untouched. packages/ and change/ diff zero against upstream/master — this PR is confined to tools/eslint-rules/.
  • packages/eslint-plugin/src/internal.js is unchanged ('error', no options).

Known limitations (follow-ups)

  • dynamic import() is not followed
  • side-effect imports (import './x') have no identifier to resolve, so they need a separate per-file pass
  • namespace member access (ns.foo) is not resolved; trivial to add, but needs a react/@types guard first or the React type-graph false positives return

Related Issue(s)

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

📊 Bundle size report

✅ No changes found

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

Pull request demo site: URL

@Hotell Hotell changed the title fix(eslint-rules): analyze base hook dependencies per symbol, not per file fix(eslint-rules): base-hook-no-forbidden-runtime - analyze base hook dependencies per symbol, not per file Aug 4, 2026
@Hotell
Hotell requested a lite review from Copilot August 4, 2026 11:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 watchedPackages allowlist 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.ts traversal, 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.

Comment thread tools/eslint-rules/rules/base-hook-no-forbidden-runtime.ts
Hotell added 4 commits August 4, 2026 17:31
… 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
Hotell force-pushed the fix/base-hook-rule-transitive-tracking branch from 1a19f5c to 7a52b4c Compare August 4, 2026 15:45
@Hotell
Hotell marked this pull request as ready for review August 4, 2026 16:25
@Hotell
Hotell requested a review from a team as a code owner August 4, 2026 16:25
@Hotell
Hotell requested review from a team and mainframev August 4, 2026 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants