fix(typescript-estree): ensure consistent TSMappedType AST shape #11086

dbarabashh · 2025-04-17T15:22:06Z

PR Checklist

Addresses an existing open issue: fixes Bug: Inconsistent TSMappedType AST shape for optional and readonly #11055
That issue was marked as accepting prs
Steps in Contributing were taken

Overview

var a: { [k in any]: any }              // No modifiers
var b: { [k in any]?: any }             // Optional modifier
var c: { readonly [k in any]: any }     // Readonly modifier
var d: { +readonly [k in any]?: any }   // +readonly and optional modifier
var e: { -readonly [k in any]: any }    // -readonly modifier

Example a: `{ [k in any]: any }`

{
  type: "TSMappedType",
  constraint: {
    type: "TSAnyKeyword"
  },
  key: {
    type: "Identifier",
    name: "k"
  },
  nameType: null,
  optional: false,             // Now has explicit false value
  readonly: undefined,         // Now has explicit undefined value
  typeAnnotation: {
    type: "TSAnyKeyword"
  }
}

Example b: `{ [k in any]?: any }`

{
  type: "TSMappedType",
  constraint: {
    type: "TSAnyKeyword"
  },
  key: {
    type: "Identifier",
    name: "k"
  },
  nameType: null,
  optional: true,              // Has true value because of ? modifier
  readonly: undefined,         // Now has explicit undefined value
  typeAnnotation: {
    type: "TSAnyKeyword"
  }
}

Example c: `{ readonly [k in any]: any }`

{
  type: "TSMappedType",
  constraint: {
    type: "TSAnyKeyword"
  },
  key: {
    type: "Identifier",
    name: "k"
  },
  nameType: null,
  optional: false,             // Now has explicit false value
  readonly: true,              // Has true value because of readonly modifier
  typeAnnotation: {
    type: "TSAnyKeyword"
  }
}

Example d: `{ +readonly [k in any]?: any }`

{
  type: "TSMappedType",
  constraint: {
    type: "TSAnyKeyword"
  },
  key: {
    type: "Identifier",
    name: "k"
  },
  nameType: null,
  optional: true,              // Has true value because of ? modifier
  readonly: "+",               // Has "+" value because of +readonly modifier
  typeAnnotation: {
    type: "TSAnyKeyword"
  }
}

Example e: `{ -readonly [k in any]: any }`

{
  type: "TSMappedType",
  constraint: {
    type: "TSAnyKeyword"
  },
  key: {
    type: "Identifier",
    name: "k"
  },
  nameType: null,
  optional: false,             // Now has explicit false value
  readonly: "-",               // Has "-" value because of -readonly modifier
  typeAnnotation: {
    type: "TSAnyKeyword"
  }
}

typescript-eslint · 2025-04-17T15:22:10Z

Thanks for the PR, @dbarabashh!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint.

netlify · 2025-04-17T15:22:24Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`656a3f9`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/6818278fcd0dae0008550592
😎 Deploy Preview	https://deploy-preview-11086--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 99 (🟢 up 1 from production) Accessibility: 100 (no change from production) Best Practices: 100 (no change from production) SEO: 98 (no change from production) PWA: 80 (no change from production) View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

nx-cloud · 2025-04-17T15:23:02Z

View your CI Pipeline Execution ↗ for commit 656a3f9.

Command	Status	Duration	Result
`nx run-many --target=build --exclude website --...`	✅ Succeeded	2s	View ↗
`nx run-many --target=clean`	✅ Succeeded	10s	View ↗

☁️ Nx Cloud last updated this comment at 2025-05-05 15:12:23 UTC

JoshuaKGoldberg · 2025-05-02T19:01:06Z

packages/typescript-estree/src/convert.ts

+              readonly: node.readonlyToken
+                ? node.readonlyToken.kind === SyntaxKind.ReadonlyKeyword ||
+                  getTextForTokenKind(node.readonlyToken.kind)
+                : undefined,


Note that readonly is an enum of true, + and -. So previously I proposed undefined as the default value. I guess false does no harm here.

Agreed that either could work, and also agreed on going with undefined. That feels to me like a more normal default when there are multiple truthy options ('+', '-'). I can see why false might make sense as a counterpoint to true, but visually I think it makes less sense than undefined.

JoshuaKGoldberg

🙌 The source code change looks good to me, thanks! Requesting changes on going with the ast-spec tests that other types of syntax conversions are verified with too.

JoshuaKGoldberg · 2025-05-02T19:04:03Z

packages/typescript-estree/tests/lib/convert.test.ts

@@ -449,5 +449,79 @@ describe('convert', () => {
      expect(tsMappedType.typeParameter).toBeUndefined();
      expect(Object.keys(tsMappedType)).toContain('typeParameter');
    });
+
+    describe('TSMappedType AST shape consistency', () => {


[Testing] We normally put fixtures in packages/ast-spec/src. Is there a reason not to here?

If not, I think packages/ast-spec/src/type/TSMappedType would be the right place to add new fixtures around readonly and/or optional mapped types.

codecov · 2025-05-02T19:05:03Z

Codecov Report

Attention: Patch coverage is 25.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 90.84%. Comparing base (f30a20e) to head (656a3f9).
Report is 19 commits behind head on main.

Files with missing lines	Patch %	Lines
packages/typescript-estree/src/convert.ts	25.00%	6 Missing ⚠️

❌ Your patch status has failed because the patch coverage (25.00%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #11086      +/-   ##
==========================================
+ Coverage   90.82%   90.84%   +0.01%     
==========================================
  Files         497      497              
  Lines       50204    50320     +116     
  Branches     8274     8311      +37     
==========================================
+ Hits        45600    45714     +114     
- Misses       4589     4591       +2     
  Partials       15       15

Flag	Coverage Δ
unittest	`90.84% <25.00%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
packages/typescript-estree/src/convert.ts	`30.74% <25.00%> (ø)`

... and 7 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

bradzacher

thanks -- just because my review time is limited I took a moment to fix up your fixtures (you had nested the folders incorrectly which meant the snapshots weren't generated -- we might wanna add an error for this...). And I added a few extra snapshots to cover some more cases.

this all LGTM!

dbarabashh · 2025-05-05T07:57:12Z

@bradzacher Thank you so much. I was just about to write a comment about the file structure.

and one more question, what should we do about the coverage rate?

JoshuaKGoldberg · 2025-05-05T15:12:05Z

coverage rate

#6116 strikes again! We can ignore the coverage drop in this PR.

ensure consistent TSMappedType ast shape

f133874

dbarabashh changed the title ~~fix(typescript-estree): Inconsistent TSMappedType AST shape~~ fix(typescript-estree): ensure consistent TSMappedType AST shape Apr 17, 2025

added mote tests

b6e71eb

dbarabashh changed the title ~~fix(typescript-estree): ensure consistent TSMappedType AST shape~~ fix(eslint-plugin): ensure consistent TSMappedType AST shape Apr 18, 2025

JoshuaKGoldberg reviewed May 2, 2025

View reviewed changes

JoshuaKGoldberg requested changes May 2, 2025

View reviewed changes

JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label May 2, 2025

dbarabashh added 3 commits May 3, 2025 16:03

updated tests

8027033

fix

6c3cb87

updated

9d64c32

bradzacher requested a review from JoshuaKGoldberg May 5, 2025 02:22

github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label May 5, 2025

bradzacher added 3 commits May 5, 2025 12:03

fix fixture folders

33112fe

split fixtures

3003d89

remove unit tests in favour of the fixture tests

e14c50f

bradzacher force-pushed the fix/ast-tsmappedtype-shape branch from 3c36704 to 6a655f2 Compare May 5, 2025 02:45

add optional fixtures

76f693f

bradzacher force-pushed the fix/ast-tsmappedtype-shape branch from 6a655f2 to 76f693f Compare May 5, 2025 02:45

bradzacher added 3 commits May 5, 2025 12:18

add fixture for readonly optional

a49f802

oops fix fixtures

6bb0a66

fix fixtures

656a3f9

bradzacher approved these changes May 5, 2025

View reviewed changes

bradzacher added the 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label May 5, 2025

JoshuaKGoldberg changed the title ~~fix(eslint-plugin): ensure consistent TSMappedType AST shape~~ fix(typescript-estree): ensure consistent TSMappedType AST shape May 5, 2025

JoshuaKGoldberg merged commit 4a97721 into typescript-eslint:main May 5, 2025
60 of 61 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(typescript-estree): ensure consistent TSMappedType AST shape #11086

fix(typescript-estree): ensure consistent TSMappedType AST shape #11086

dbarabashh commented Apr 17, 2025

typescript-eslint bot commented Apr 17, 2025

netlify bot commented Apr 17, 2025 •

edited

Loading

nx-cloud bot commented Apr 17, 2025 •

edited

Loading

JoshuaKGoldberg May 2, 2025 •

edited

Loading

JoshuaKGoldberg left a comment

JoshuaKGoldberg May 2, 2025

codecov bot commented May 2, 2025 •

edited

Loading

bradzacher left a comment

dbarabashh commented May 5, 2025

JoshuaKGoldberg commented May 5, 2025

fix(typescript-estree): ensure consistent TSMappedType AST shape #11086

fix(typescript-estree): ensure consistent TSMappedType AST shape #11086

Conversation

dbarabashh commented Apr 17, 2025

PR Checklist

Overview

Example a: { [k in any]: any }

Example b: { [k in any]?: any }

Example c: { readonly [k in any]: any }

Example d: { +readonly [k in any]?: any }

Example e: { -readonly [k in any]: any }

typescript-eslint bot commented Apr 17, 2025

netlify bot commented Apr 17, 2025 • edited Loading

✅ Deploy Preview for typescript-eslint ready!

nx-cloud bot commented Apr 17, 2025 • edited Loading

JoshuaKGoldberg May 2, 2025 • edited Loading

Choose a reason for hiding this comment

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg May 2, 2025

Choose a reason for hiding this comment

codecov bot commented May 2, 2025 • edited Loading

Codecov Report

bradzacher left a comment

Choose a reason for hiding this comment

dbarabashh commented May 5, 2025

JoshuaKGoldberg commented May 5, 2025

Example a: `{ [k in any]: any }`

Example b: `{ [k in any]?: any }`

Example c: `{ readonly [k in any]: any }`

Example d: `{ +readonly [k in any]?: any }`

Example e: `{ -readonly [k in any]: any }`

netlify bot commented Apr 17, 2025 •

edited

Loading

nx-cloud bot commented Apr 17, 2025 •

edited

Loading

JoshuaKGoldberg May 2, 2025 •

edited

Loading

codecov bot commented May 2, 2025 •

edited

Loading