Skip to content

Merge linter settings in extended TypeSpec configs - #11924

Open
Sylvester Kaczmarek (sylvesterkaczmarek) wants to merge 8 commits into
microsoft:mainfrom
sylvesterkaczmarek:fix/11849-merge-linter-config
Open

Sylvester Kaczmarek (sylvesterkaczmarek) wants to merge 8 commits into
microsoft:mainfrom
sylvesterkaczmarek:fix/11849-merge-linter-config

Conversation

@sylvesterkaczmarek

Copy link
Copy Markdown
Contributor

Summary

Fixes #11849.

Merge the linter entry one level when a TypeSpec config extends another config. Child linter fields now override matching parent fields without dropping unrelated inherited settings such as extends or enable.

Testing

  • pnpm vitest run test/config/config.test.ts in packages/compiler
  • pnpm change verify

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).
1 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

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.

🟢 Approval recommended

The implementation matches the stated merge semantics for linter under extends, includes targeted test coverage, and adds an appropriate .chronus fix entry.

Pull request overview

This PR updates the TypeSpec compiler’s config loading behavior so that when a tspconfig.yaml extends another config, the linter block is merged one level deep (similar in spirit to how emitter options are merged), preventing unrelated inherited linter settings from being dropped.

Changes:

  • Merge parent.linter and config.linter (shallowly) when both are present during extends resolution.
  • Add a new config scenario and unit test covering inherited linter.extends / linter.enable while overriding linter.disable.
  • Add a .chronus fix entry for @typespec/compiler.
File summaries
File Description
packages/compiler/src/config/config-loader.ts Implements one-level merge for linter when resolving extends.
packages/compiler/test/config/config.test.ts Adds a regression test asserting correct inherited vs overridden linter fields.
packages/compiler/test/config/scenarios/extends-linter/typespec-base.yaml New base config scenario defining linter.extends, enable, and disable.
packages/compiler/test/config/scenarios/extends-linter/tspconfig.yaml New child config scenario overriding only linter.disable.
.chronus/changes/fix-11849-merge-linter-config-2026-8-10-11-8-49.md Changelog entry documenting the fix for @typespec/compiler.
Review details
  • Files reviewed: 5/5 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

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.

🔵 Needs a closer look

Preserve inherited ruleset source metadata and update the handbook documentation.

Review details

Suppressed comments (2)

packages/compiler/src/config/config-loader.ts:140

  • When the parent linter contains a file: ruleset reference, this merge keeps the parent rule set but discards the YAML source that declared it. program.ts always supplies the final child config.file as the linter source, so diagnostics for a missing/invalid inherited ruleset are located at offset 0 of the child config instead of the parent linter.extends entry. Preserve the source/origin for inherited linter settings (or carry per-entry source metadata) and add an integration test for this case.
      merged.linter = { ...parent.linter, ...config.linter };

packages/compiler/src/config/config-loader.ts:140

  • The handbook currently says root-level properties are shallow-merged except options (website/src/content/docs/docs/handbook/configuration/configuration.mdx:123-128). This new linter special case makes that public contract inaccurate; update the extension rules/example to document the one-level linter merge and that each nested field is replaced wholesale.
    if (parent.linter && config.linter) {
      merged.linter = { ...parent.linter, ...config.linter };
  • Files reviewed: 5/5 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@sylvesterkaczmarek

Copy link
Copy Markdown
Contributor Author

Addressed the latest review findings in 4a7d1ab. Inherited linter fields now preserve the YAML source that supplied them, so file ruleset diagnostics point to the parent config. The handbook also documents the one-level linter merge. The config test suite passes 33/33, and the compiler TypeScript build passes.

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.

🟡 Changes recommended

Two moderate issues remain in tsp info output and inherited diagnostic source locations.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

packages/compiler/src/config/types.ts:91

  • This new enumerable field leaks into tsp info: printInfoAction removes diagnostics, filename, and file before serializing the remaining TypeSpecConfig, but does not remove linterSource. Any config with a linter will therefore print internal YamlScript/Document/source data instead of just the resolved configuration; exclude this metadata from the info output (and add a regression test).
  linterSource?: Partial<Record<keyof LinterConfig, YamlScript>>;

packages/compiler/src/core/program.ts:433

  • The new source selection still runs through the language-server config cache, which serializes TypeSpecConfig with JSON.stringify/JSON.parse in server/compile-service.ts. That turns each YamlScript.doc into plain JSON, so getLocationInYamlScript cannot traverse the YAML document and inherited file: diagnostics lose their real linter.extends range (typically falling back to position 0); preserve/reparse the YAML document for this path and cover it through the server entrypoint.
        source: options.configFile?.linterSource?.extends
          ? { script: options.configFile.linterSource.extends, path: ["linter"] }
  • Files reviewed: 8/8 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread packages/compiler/test/config/config.test.ts Outdated
@sylvesterkaczmarek

Copy link
Copy Markdown
Contributor Author

Addressed both follow-up findings in f7819ce. Linter source metadata is now stored as config paths, survives the language-server JSON cache, and is reparsed before linter diagnostics; tsp info strips the internal metadata. Focused config/info tests pass 35/35, and the compiler TypeScript build passes.

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.

🔵 Needs a closer look

Tighten the diagnostic assertion in config.test.ts before approval.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

packages/compiler/test/config/config.test.ts:297

  • This assertion searches for one matching diagnostic and throws only when none is found, so the test can still pass with extra diagnostics or an unexpected severity. Please use the shared expectDiagnostics helper to assert the exact diagnostic list (and then inspect program.diagnostics[0] for the source span), as required by the compiler test conventions.
  • Files reviewed: 10/10 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@sylvesterkaczmarek

Copy link
Copy Markdown
Contributor Author

Addressed the final test-convention review in 5f8affb. The fixture now isolates the inherited ruleset error and expectDiagnostics asserts the exact file-not-found diagnostic before checking the parent YAML span. Focused config/info tests pass 35/35, and the compiler build remains clean.

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.

🔵 Needs a closer look

Three unresolved moderate findings require addressing before approval.

Review details

Suppressed comments (3)

packages/compiler/src/core/program.ts:435

  • linterSource.extends is recorded for every linter extends value, so this rereads and reparses the declaring config on every compilation even when all references are library rulesets. That adds avoidable I/O and YAML parsing to each language-server compile; only load the source when the final ruleset contains a file: reference, which is the only path where extendRuleSet consumes source.
    const linterSourcePath = options.configFile?.linterSource?.extends;
    if (linterSourcePath) {
      try {
        const [script] = parseYaml(await host.readFile(linterSourcePath));

packages/compiler/src/core/program.ts:441

  • The new failure path here has no regression test: the added integration test only covers successfully rereading the inherited config. Please add a compile test that makes the declaring config unreadable after config resolution and verifies compilation still reports the ruleset diagnostic via the documented fallback, so this race-handling branch cannot silently lose diagnostics or targets.
      } catch {
        // The config was readable when it was loaded. If it disappeared between
        // config resolution and compilation, fall back to the final config source.
      }
    }

packages/compiler/test/config/config.test.ts:107

  • This fixture only uses different keys for the parent and child (enable vs. disable), so it would still pass if the implementation incorrectly deep-merged a single field's rule map. Add a case where both configs define the same enable or disable field and assert that the child replaces the entire parent map, as documented.
  it("merges linter settings from an extended config", async () => {
    const config = await loadTestConfig("extends-linter");
    deepStrictEqual(config.linter, {
      extends: ["test/all"],
      enable: { "test/base-rule": true },
      disable: { "test/child-rule": "Child exemption" },
    });
  • Files reviewed: 10/10 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

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.

🟡 Changes recommended

Unresolved critical and moderate diagnostic source-location issues remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

packages/compiler/src/core/cli/actions/info.ts:36

  • printInfoAction is the user-facing path, but the new test invokes getPrintableConfig directly. If this call is removed or the action serializes the original config, the test still passes while tsp info leaks linterSource; cover printInfoAction output (for example by capturing console.log) rather than only the helper.
  const { filename, ...restOfConfig } = getPrintableConfig(config);

packages/compiler/src/core/program.ts:448

  • When the inherited config cannot be reread, this fallback attaches the child YAML as the source even though the inherited file: entry is not present under linter.extends in that file. getLocationInYamlScript therefore returns the child file at offset 0, so the missing-ruleset diagnostic is reported at the wrong location; avoid falling back to the child source for this case (or synthesize a source for linterSourcePath) and assert the target in the fallback test.
    linterSource ??= options.configFile?.file
      ? { script: options.configFile.file, path: ["linter"] }
      : undefined;
  • Files reviewed: 10/10 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread packages/compiler/src/core/program.ts

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.

Copilot review overview

🔵 Needs a closer look

Add an end-to-end printInfoAction assertion proving internal metadata is omitted.

Review effort: Lite
Findings: None

Resolved since last review (1)

@pkg-pr-new

pkg-pr-new Bot commented Sep 28, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/@typespec/compiler@11924

commit: 418bb8f

@github-actions

Copy link
Copy Markdown
Contributor

All changed packages have been documented.

  • ✅ @typespec/compiler
Show changes

@typespec/compiler - fix ✏️

Merge linter settings from extended TypeSpec configuration files.

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.

Copilot review overview

🟢 Approval recommended

No unresolved review issues were identified.

Review effort: Lite
Findings: None

Copilot AI lite review requested due to automatic review settings September 29, 2026 22:55
@sylvesterkaczmarek

Copy link
Copy Markdown
Contributor Author

Mark Cowlishaw (@markcowl) Timothee Guerin (@timotheeguerin) iscai-msft catalinaperalta Laurent Mazuel (@lmazuel) I refreshed this branch onto current main and addressed the remaining Copilot request with an end-to-end printInfoAction regression. The test loads a real virtual tspconfig.yaml that produces linterSource, captures the actual tsp info output, verifies the resolved linter configuration remains visible, and asserts internal linterSource/diagnostic metadata is omitted. Validation on the current branch: compiler build passes, focused info.test.ts passes 3/3, compiler lint reports 0 warnings/errors, repository Prettier leaves the new test unchanged, and git diff --check is clean.\n\n@copilot review

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.

Copilot review overview

🔵 Needs a closer look

Inherited linter diagnostics currently use the wrong source path.

Review effort: Lite
Findings: None

Copilot AI lite review requested due to automatic review settings September 30, 2026 08:17
@sylvesterkaczmarek

Copy link
Copy Markdown
Contributor Author

Addressed the late source-path concern in 55d67e8. If an inherited config becomes unreadable after resolution, the compiler no longer misattributes its file-ruleset diagnostic to the child tspconfig at offset 0; it reports the diagnostic without a YAML target instead. The existing fallback regression now asserts NoTarget. Full compiler validation passes: 163 test files / 4,479 tests, compiler build, compiler lint (0 warnings/errors), and git diff --check. Copilot review

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.

Copilot review overview

🟢 Approval recommended

No unresolved issues were identified, and the requested tests and documentation are included.

Review effort: Lite
Findings: None

Signed-off-by: Sylvester Kaczmarek <16242628+sylvesterkaczmarek@users.noreply.github.com>
Signed-off-by: Sylvester Kaczmarek <16242628+sylvesterkaczmarek@users.noreply.github.com>
Signed-off-by: Sylvester Kaczmarek <16242628+sylvesterkaczmarek@users.noreply.github.com>
Signed-off-by: Sylvester Kaczmarek <16242628+sylvesterkaczmarek@users.noreply.github.com>
Signed-off-by: Sylvester Kaczmarek <16242628+sylvesterkaczmarek@users.noreply.github.com>

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.

Copilot review overview

🟡 Changes recommended

Resolve the linter override source-metadata issue before approval.

Review effort: Lite
Findings: 1 Medium severity

Open (1)

Comment thread packages/compiler/src/core/program.ts
Copilot AI lite review requested due to automatic review settings September 30, 2026 23:18
@sylvesterkaczmarek

Copy link
Copy Markdown
Contributor Author

Copilot review

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.

Copilot review overview

🟢 Approval recommended

No unresolved blocking issues were identified.

Review effort: Lite
Findings: None

Resolved since last review (1)

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

compiler:core Issues for @typespec/compiler meta:website TypeSpec.io updates

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Config extends should it merge the linter entry

2 participants