Skip to content

Report unknown rule names in diagnosticSeverity - #748

Closed
jfspencer wants to merge 3 commits into
Effect-TS:mainfrom
jfspencer:feat/unknown-rule-name-diagnostic
Closed

jfspencer wants to merge 3 commits into
Effect-TS:mainfrom
jfspencer:feat/unknown-rule-name-diagnostic

Conversation

@jfspencer

@jfspencer jfspencer commented Sep 14, 2026

Copy link
Copy Markdown

Adds a diagnostic for a diagnosticSeverity key that names a rule the running build does not provide. Such a key was previously accepted in silence, so a project pinning a rule at error believed it was enforced while nothing ran.

For example, this configuration now reports:

{
  "compilerOptions": {
    "plugins": [
      {
        "name": "@effect/language-service",
        "diagnosticSeverity": {
          "floatingEffect": "error",
          "importFromBarrel": "error"
        }
      }
    ]
  }
}
tsconfig.json(9,11): warning TS377134: Unknown Effect diagnostic rule `importFromBarrel` in `diagnosticSeverity`. This version of @effect/tsgo does not provide it, so this entry has no effect. effect(unknownRuleName)

A near miss carries the intended name instead:

tsconfig.json(9,11): warning TS377135: Unknown Effect diagnostic rule `floatingEfect` in `diagnosticSeverity`. Did you mean `floatingEffect`? effect(unknownRuleName)

Both the top-level diagnosticSeverity map and the map inside each overrides entry are covered.

How it is wired

Validation runs in two phases, because the two things it needs become available at different points in config parsing.

The offending node exists only while the file that declares it is being parsed, before any extends hop is merged — so RegisterValidateEffectPluginOptionsCallback, invoked from onPropertySet when compilerOptions.plugins is set, reports every name that does not resolve and anchors it on the key in the file that declares it. That includes a base config reached through extends, which is where a shared plugin block usually lives.

Whether the diagnostic is enabled, and at which severity, is a property of the merged configuration — so RegisterFinalizeEffectPluginDiagnosticsCallback, invoked from parseJsonConfigFileContentWorker once the chain is merged, drops or recategorizes what the first phase produced. This is what makes diagnostics: false and diagnosticSeverity.unknownRuleName work when they are inherited rather than declared in the file carrying the offending name. Splitting it this way is load-bearing: a single-phase check reads the declaring file's unmerged options, which silently ignores an inherited diagnostics: false and leaves a package unable to silence a name declared in a base config it does not own.

Both hooks sit beside the existing RegisterMergeCompilerOptionsCallback in the tsoptions patch. The diagnostic reaches tsc, the language server, --build and the incremental path through the config-file parsing diagnostics.

The accepted name set is rules.All plus the severity keys that are not rules in the registry, which is unusedDirective and the new unknownRuleName; both are now named constants so the two lists cannot drift apart.

Severity

The diagnostic defaults to warning and is configured through diagnosticSeverity.unknownRuleName like any other, so "unknownRuleName": "off" silences it. A warning is deliberate rather than an error: the same plugin block is read by the JS @effect/language-service, whose rule set differs, so a hard failure would break those projects on upgrade. Note that with the default ignoreEffectWarningsInTscExitCode: false a warning already fails tsc. One --build consequence is worth stating plainly: because these diagnostics land in the config-file parsing diagnostics, an offending config keeps its project out of date on every tsc --build, so every project extending a shared base with a bad key re-typechecks each invocation. That is bounded — measured, there is no cascade to dependent projects — and it matches how every existing warning-severity Effect rule already behaves. Whether warnings should count toward that flag is a general policy question rather than one specific to this diagnostic, so it is left alone here and noted in the issue.

Deliberately out of scope

An invalid severity value. ParseSeverity returns SeverityError for any unrecognized string, so "errror" promotes a rule to error rather than disabling it. That is loud and wrong rather than silent, so it wants its own decision about whether to report or to reject, and is noted as follow-up in the issue.

Also out of scope, both noted in the issue: the additionalProperties in the shipped JSON schema still accepts any key with a valid severity value, and unknownRuleName/unusedDirective are absent from the generated diagnosticSeverity properties (adding non-rule keys there is a change to a published artifact and a separate call); and the raw-JSON config path (parseOwnConfigOfJson) is unhooked, because with no source file there is no node to anchor on and a location-less warning is worse than none.

Tests

internal/effectconfigcheck unit tests cover the top-level map, an overrides entry, an extends chain, the spelling suggestion, off and error severity, an unrelated plugin, diagnostics: false, and four inheritance cases: a base silencing a child through unknownRuleName, a base silencing a child through diagnostics: false, a child silencing a name declared in a base, and a child raising the severity of a name declared in a base while the anchor stays on the base. effect-v4 fixtures unknownRuleName, unknownRuleName_valid and unknownRuleName_overrides carry baselines.

internal/effecttest/runner.go now surfaces parsedConfig.Errors alongside the per-file diagnostics, because a diagnostic reported against the tsconfig never reaches the per-file collections. No existing baseline changed.

No generated docs, schema, metadata or Oxlint preset changed, because unknownRuleName is not a rule in the registry, which matches how unusedDirective is handled. Includes a minor changeset.

Validation: pnpm setup-repo, pnpm lint (0 issues, deadcode clean), pnpm check, pnpm test, and go test -race ./internal/effectconfigcheck/.... pnpm test reports Go 21 packages ok / 0 FAIL, and vitest 3 failed | 122 passed; those three failures are pre-existing on macOS in test/experimental-oxlint.test.ts, where the expectation is built from an mkdtemp path under /var/folders/... while the code under test returns the realpath /private/var/folders/.... They are identical on an unmodified checkout of main at ae1ed026.

Closes #747

🤖 Generated with Claude Code

jfspencer and others added 3 commits September 14, 2026 14:25
A `diagnosticSeverity` key naming a rule the running build does not provide
was accepted in silence, so a project pinning a rule at `error` believed it
was enforced while nothing ran. The same plugin block is read by the JS
`@effect/language-service`, whose rule set differs, which is how a config
drifts into naming rules this build has never had.

Validate the configured names when `compilerOptions.plugins` is parsed, so
the diagnostic is anchored on the offending key in the config file that
declares it and reaches both `tsc` and the language server through the
config-file parsing diagnostics. Both the top-level map and the map inside
each `overrides` entry are covered, and a near miss carries the intended
name.

The diagnostic defaults to `warning` and is configured through
`diagnosticSeverity.unknownRuleName`, alongside `unusedDirective` as a
severity key that is not a rule in the registry.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The enable and severity gate was read from the options of the file being
parsed, which is built before any extends hop is merged. Three measured
consequences: an inherited `diagnostics: false` was ignored, an inherited
`diagnosticSeverity.unknownRuleName` did not silence a name declared in a
child, and a package could not silence a name declared in a base config it
does not own.

Split the work across the two seams where each half is actually available.
The offending node exists only while its own file is parsed, so that phase
now reports unconditionally and keeps the anchor. The setting that governs
the report belongs to the merged configuration, so a second callback runs
from parseJsonConfigFileContentWorker once the chain is merged and drops or
recategorizes what the first phase produced.

Register the callbacks in the tests through the etscheckerhooks init rather
than assigning the globals from parallel subtests, which `go test -race`
reported as 25 data races.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
FinalizeDiagnostics receives the whole config-error slice and returns a
replacement, so it holds drop authority over diagnostics it does not own,
restrained only by a code predicate. The rest of the suite reads the Effect
diagnostics through a filter that would hide the loss of that predicate, so
a regression dropping every other config error in the file could land green.

Assert on the unfiltered slice instead, with an unknown compiler option
alongside an unknown rule name at `unknownRuleName: "off"`, which is the arm
that rebuilds the slice. Removing the predicate turns this test red.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
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.

diagnosticSeverity silently accepts a rule name that does not exist

1 participant