Bug: [consistent-indexed-object-style] Unsafe autofix with regards to circular references [8.12.1 regression] #10224

JavaScriptBach · 2024-10-29T04:50:12Z

Before You File a Bug Report Please Confirm You Have Done The Following...

I have tried restarting my IDE and the issue persists.
I have updated to the latest version of the packages.
I have searched for related issues and found none that matched my issue.
I have read the FAQ and my problem is not listed.

Playground Link

https://typescript-eslint.io/play/#ts=5.4.3&fileType=.tsx&code=FAUwHgDg9gTgLgAjgTwiBBZZBhAljAYwFcAbAQxgBVV0BeYBBAHwQCMooSQyA7B5hDyIBbViBj8WAbwQBtAAoJcPBAGc4MZQHMAugC5MOfMXJUaCAL4BuYMAD0dhOGjwk5rHkKkK1NACYEegdGAXZObj5gxhYhUXF7R2iEACUQAlgAEwAedU0eLQAaQ08THxo-AD4rIA&eslintrc=N4KABGBEBOCuA2BTAzpAXGUEKQAIBcBPABxQGNoBLY-AWhXkoDt8B6MgeyeUuX0Ra1mAE0QAPRMNocARgCtEZOn0JJ0URNGgdokcGAC%2BIA0A&tsconfig=N4KABGBEDGD2C2AHAlgGwKYCcDyiAuysAdgM6QBcYoEEkJemy0eAcgK6qoDCAFutAGsylBm3TgwAXxCSgA&tokens=false

Repro Code

export type MyCircularType =
  | boolean
  | number
  | { [P in string]: MyCircularType };

ESLint Config

module.exports = {
  parser: "@typescript-eslint/parser",
  rules: {
    "@typescript-eslint/consistent-indexed-object-style": "error"
  },
};

tsconfig

{
  "compilerOptions": {
    // ...
  }
}

Expected Result

This should not be a lint error. The autofix produces code that does not typecheck:

export type MyCircularType =
  | boolean
  | number
  | Record<string, MyCircularType>;

2456: Type alias 'MyCircularType' circularly references itself. 2:13 - 2:27

This appears to be intended behavior, according to microsoft/TypeScript#35164

Actual Result

Autofixed into code that doesn't typecheck:

export type MyCircularType =
  | boolean
  | number
  | Record<string, MyCircularType>;

Additional Info

Probably caused by #10160.

The text was updated successfully, but these errors were encountered:

JavaScriptBach · 2024-11-04T23:01:44Z

Took a cursory look at this. It seems like there is an already an attempt to have this rule not fail if there are circular index signatures, but it doesn't handle the following cases:

The in operator

type MyCircularType2 = { [P in "foo" | "bar"]: MyCircularType2 };

Circular references between 2 different type aliases

type Bar = { [k: string]: Foo };
type Foo = string | Bar;

I'm guessing that fixing 1) is tractable, but I'm not sure about 2).

@kirkwaiblinger I can try to write a PR to fix one or both if you have some pointers? Is https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/src/rules/consistent-indexed-object-style.ts#L70-L75 the place to fix?

bradzacher · 2024-11-04T23:33:08Z

It's doubtful that we can handle the second case.
But the first case should be handleable and should already be handled by the logic.

kirkwaiblinger · 2024-11-05T01:59:16Z

Yeah, case 1 was for sure an oversight. I simply didn't think of possibly-circular types in #10160. Seems reasonable to check the circularity detection logic you've linked for the TSIndexSignature case, and try to port it to the TSMappedType case.

tbh Case 2 feels like it could be detectable too if you want to wanted to implement a cycle detection algorithm for the type variables. Questionable how worthwhile this is, perhaps. But, conceptually I'd think we could analyze syntactically:

type Foo's declaration depends on Bar and string
Bar's declaration is visible in this file. So we'll check what types its declaration depends on
type Bar's declaration depends on Foo and string
we have found a cycle.

(note that I'm spitballing; I haven't tried doing something like this and don't know of an example in this repo offhand)

kirkwaiblinger · 2024-11-05T02:03:32Z

@kirkwaiblinger I can try to write a PR to fix one or both if you have some pointers? Is https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/src/rules/consistent-indexed-object-style.ts#L70-L75 the place to fix?

Following from previous comment, I think that the issue for 1 is that

typescript-eslint/packages/eslint-plugin/src/rules/consistent-indexed-object-style.ts

Lines 146 to 208 in 9c956ee

    
           TSMappedType(node): void { 
        
             const key = node.key; 
        
             const scope = context.sourceCode.getScope(key); 
        
             const scopeManagerKey = nullThrows( 
        
               scope.variables.find( 
        
                 value => value.name === key.name && value.isTypeVariable, 
        
               ), 
        
               'key type parameter must be a defined type variable in its scope', 
        
             ); 
        
             // If the key is used to compute the value, we can't convert to a Record. 
        
             if ( 
        
               scopeManagerKey.references.some( 
        
                 reference => reference.isTypeReference, 
        
               ) 
        
             ) { 
        
               return; 
        
             } 
        
             const constraint = node.constraint; 
        
             if ( 
        
               constraint.type === AST_NODE_TYPES.TSTypeOperator && 
        
               constraint.operator === 'keyof' && 
        
               !isParenthesized(constraint, context.sourceCode) 
        
             ) { 
        
               // This is a weird special case, since modifiers are preserved by 
        
               // the mapped type, but not by the Record type. So this type is not, 
        
               // in general, equivalent to a Record type. 
        
               return; 
        
             } 
        
             // There's no builtin Mutable<T> type, so we can't autofix it really. 
        
             const canFix = node.readonly !== '-'; 
        
             context.report({ 
        
               node, 
        
               messageId: 'preferRecord', 
        
               ...(canFix && { 
        
                 fix: (fixer): ReturnType<ReportFixFunction> => { 
        
                   const keyType = context.sourceCode.getText(constraint); 
        
                   const valueType = context.sourceCode.getText( 
        
                     node.typeAnnotation, 
        
                   ); 
        
                   let recordText = `Record<${keyType}, ${valueType}>`; 
        
                   if (node.optional === '+' || node.optional === true) { 
        
                     recordText = `Partial<${recordText}>`; 
        
                   } else if (node.optional === '-') { 
        
                     recordText = `Required<${recordText}>`; 
        
                   } 
        
                   if (node.readonly === '+' || node.readonly === true) { 
        
                     recordText = `Readonly<${recordText}>`; 
        
                   } 
        
                   return fixer.replaceText(node, recordText); 
        
                 }, 
        
               }), 
        
             }); 
        
           },

simply doesn't hit any of the logic in

typescript-eslint/packages/eslint-plugin/src/rules/consistent-indexed-object-style.ts

Lines 67 to 79 in 9c956ee

    
           const scope = context.sourceCode.getScope(parentId); 
        
           const superVar = ASTUtils.findVariable(scope, parentId.name); 
        
           if (superVar) { 
        
             const isCircular = superVar.references.some( 
        
               item => 
        
                 item.isTypeReference && 
        
                 node.range[0] <= item.identifier.range[0] && 
        
                 node.range[1] >= item.identifier.range[1], 
        
             ); 
        
             if (isCircular) { 
        
               return; 
        
             } 
        
           }

So I'd guess that copying/porting/unifying the logic would address point 1.

kirkwaiblinger · 2024-11-05T02:07:42Z

Note that the "in operator" here isn't really an "in operator". It's just the syntax that distinguishes a mapped type (type T = { [K in Foo]: SomeType<K> }) from an index signature in a type literal (type T = { [k: string]: number; otherProperty: string}, also comes in interface flavor, and in class bodies). They look similar but have vastly capabilities (I learned while working on this 🤣 ).

Mentioning this clarification in case it helps make sense of the current code referring to TSMappedType.

JavaScriptBach added bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels Oct 29, 2024

kirkwaiblinger added accepting prs Go ahead, send a pull request that resolves this issue and removed triage Waiting for team members to take a look labels Oct 29, 2024

JavaScriptBach mentioned this issue Nov 7, 2024

fix(eslint-plugin): [consistent-indexed-object-style] handle circular mapped types #10301

Merged

3 tasks

JoshuaKGoldberg closed this as completed in #10301 Nov 14, 2024

github-actions bot added the locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. label Nov 22, 2024

github-actions bot locked as resolved and limited conversation to collaborators Nov 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug: [consistent-indexed-object-style] Unsafe autofix with regards to circular references [8.12.1 regression] #10224

Bug: [consistent-indexed-object-style] Unsafe autofix with regards to circular references [8.12.1 regression] #10224

JavaScriptBach commented Oct 29, 2024

JavaScriptBach commented Nov 4, 2024

bradzacher commented Nov 4, 2024

kirkwaiblinger commented Nov 5, 2024 •

edited

Loading

kirkwaiblinger commented Nov 5, 2024

kirkwaiblinger commented Nov 5, 2024 •

edited

Loading

Bug: [consistent-indexed-object-style] Unsafe autofix with regards to circular references [8.12.1 regression] #10224

Bug: [consistent-indexed-object-style] Unsafe autofix with regards to circular references [8.12.1 regression] #10224

Comments

JavaScriptBach commented Oct 29, 2024

Before You File a Bug Report Please Confirm You Have Done The Following...

Playground Link

Repro Code

ESLint Config

tsconfig

Expected Result

Actual Result

Additional Info

JavaScriptBach commented Nov 4, 2024

bradzacher commented Nov 4, 2024

kirkwaiblinger commented Nov 5, 2024 • edited Loading

kirkwaiblinger commented Nov 5, 2024

kirkwaiblinger commented Nov 5, 2024 • edited Loading

kirkwaiblinger commented Nov 5, 2024 •

edited

Loading

kirkwaiblinger commented Nov 5, 2024 •

edited

Loading