feat: Port TypeScript-ESLint rule consistent-indexed-object-style

[delino]

Summary

This PR ports the TypeScript-ESLint rule consistent-indexed-object-style to rslint, which enforces consistent usage of indexed object type definitions in TypeScript.

Follow-up task of PR #78.

Implementation Details

• ✅ Created new rule in internal/plugins/typescript/rules/consistent_indexed_object_style/
• ✅ Enforces consistent placement of indexed object styles
• ✅ Supports two configuration modes: "record" (default) and "index-signature"
• ✅ Detects and prevents circular type references
• ✅ Registered in TypeScript plugin registry

Rule Behavior

The rule ensures that indexed object types are consistently defined using either Record utility type or index signature syntax.

Configuration Options

• record (default): Prefer Record<K, V> utility type
• index-signature: Prefer { [key: K]: V } index signature syntax

Invalid Patterns (record mode - default)

// Index signatures that should use Record
interface Foo { [key: string]: any; }
type Bar = { [key: string]: any };
type Baz = { [K in string]: any };

Valid Patterns (record mode - default)

type Foo = Record<string, any>;
type Bar = Readonly<Record<string, any>>;
interface Foo { [key: string]: any; bar: string; } // Mixed properties OK
interface Foo { [key: string]: Foo; } // Circular references OK

Invalid Patterns (index-signature mode)

// Record types that should use index signatures
type Foo = Record<string, any>;
type Bar = Record<number, string>;

Valid Patterns (index-signature mode)

interface Foo { [key: string]: any; }
type Bar = { [key: string]: any };

Test Coverage

• ✅ Comprehensive test suite ported from TypeScript-ESLint repository
• ✅ Valid test cases covering various scenarios (60+ cases)
• ✅ Invalid test cases with expected error detection (50+ cases)
• Tests include:
  • Interface declarations with index signatures
  • Type literals with index signatures
  • Mapped types
  • Record type references
  • Circular type references (properly detected and handled)
  • Both configuration modes
  • Edge cases (nested types, function signatures, generic types)

Test Plan

• CI tests pass
• Manual testing with example code
• Integration with existing linter setup

References

• Rule documentation: https://typescript-eslint.io/rules/consistent-indexed-object-style/
• TypeScript-ESLint source: https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/src/rules/consistent-indexed-object-style.ts
• TypeScript-ESLint tests: https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/tests/rules/consistent-indexed-object-style.test.ts
• Related PR feat: Port TypeScript-ESLint rule consistent-generic-constructors #78: feat: Port TypeScript-ESLint rule consistent-generic-constructors #78

Files Changed

• internal/config/config.go - Added import and rule registration (2 lines)
• internal/plugins/typescript/rules/consistent_indexed_object_style/consistent_indexed_object_style.go - Complete rule implementation (~450 lines)
• internal/plugins/typescript/rules/consistent_indexed_object_style/consistent_indexed_object_style_test.go - Comprehensive test suite (~280 lines)

Notes

This is a draft PR. The core functionality is implemented and comprehensive tests have been added. All test cases from the TypeScript-ESLint implementation have been ported.

🤖 Generated with Claude Code

Co-Authored-By: Claude noreply@anthropic.com

[kdy1]

🤖 This pull request has been linked to DevBird Task #1140

View the task details and manage the automated development workflow in DevBird.

Learn more about DevBird here or the announcement blog post here.

[kdy1]

📋 DevBird Task Prompt

Objective

Port the TypeScript-ESLint rule consistent-indexed-object-style to rslint (Go implementation).

Documentation & Resources

• Rule documentation: https://typescript-eslint.io/rules/consistent-indexed-object-style/
• TypeScript-ESLint source: https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/src/rules/consistent-indexed-object-style.ts
• TypeScript-ESLint tests: https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/tests/rules/consistent-indexed-object-style.test.ts

Rule Description

Enforces consistent usage of type imports.

Implementation Requirements

1. Create directory internal/plugins/typescript/rules/consistent_indexed_object_style/
2. Implement rule from scratch in consistent_indexed_object_style.go
3. Port all test cases from TypeScript-ESLint
4. Create comprehensive consistent_indexed_object_style_test.go
5. Register the rule

Test Requirements

• Port ALL test cases
• All tests must pass

Success Criteria

• Full implementation
• All tests passing

---

This comment was automatically added by DevBird. You can disable this feature in DevBird Settings.

[claude]

Pull Request Review: consistent-indexed-object-style Rule

Summary

This PR ports the TypeScript-ESLint rule consistent-indexed-object-style to rslint. The implementation is comprehensive and follows the project's conventions well.

Code Quality & Best Practices

Strengths

1. Good Structure - follows established patterns like consistent_generic_constructors
2. Comprehensive Options Parsing - handles multiple option formats correctly at lines 24-38
3. Proper Rule Registration - correctly registered in config.go following alphabetical ordering
4. Thorough Test Coverage - 60+ valid cases and 50+ invalid cases with good edge case coverage

Issues to Address

1. Incomplete Rule Documentation Comment (Line 14)

The comment says 'enforces consistent usage of type imports' which is a copy-paste error. Should be:
// ConsistentIndexedObjectStyleRule enforces consistent usage of indexed object type definitions

2. Unused Helper Function (Line 422)

The getSourceText helper function is defined but never used. This should either be removed if not needed, or used if it was intended for autofix functionality.

3. Missing Autofix Functionality

The rule detects violations but doesn't provide autofixes. Similar rules like consistent_generic_constructors provide fixes. This rule would benefit from autofixes to convert between Record and index signature styles.

4. Duplicate Code (Lines 265-288)

isDeeplyReferencingType and isDeeplyReferencingTypeParam are nearly identical and could be consolidated.

Potential Bugs

Critical Issues

1. Incomplete Mapped Type Check (Line 139)

The canConvertMappedTypeToRecord function may not properly distinguish between:

• { [K in string]: K } - References type parameter (cannot convert)
• { [K in string]: string } - Doesn't reference type parameter (can convert)

2. Missing Readonly/Optional Modifier Checks

The rule doesn't account for readonly or optional modifiers in index signatures. These should be handled or documented as limitations.

Performance Considerations

• Good use of early returns and null checks
• checkTypeReference function uses recursion and could benefit from depth limiting

Test Coverage

Excellent coverage with edge cases, configuration modes, and various TypeScript patterns.

Additional test suggestions:

• readonly/optional modifiers
• nested Record types
• union key types
• symbol keys

Recommendations

Priority High

1. Fix the documentation comment (line 14)
2. Remove unused getSourceText function OR implement autofixes
3. Verify mapped type parameter reference detection

Priority Medium

4. Consider adding autofix functionality
5. Document limitations regarding readonly/optional modifiers
6. Refactor duplicate circular reference detection functions

Priority Low

7. Add suggested edge case tests
8. Add depth limiting to recursive type checking

Conclusion

This is a solid implementation with excellent test coverage. Main concerns are the documentation error, missing autofix functionality, and potential edge cases with mapped types.

Recommendation: Request changes to address High priority items before merging.

Great work on porting this rule!

[claude]

Code Review: Port TypeScript-ESLint rule consistent-indexed-object-style

Thank you for this comprehensive PR! The implementation is well-structured and follows the codebase patterns. Here's my detailed review:

✅ Strengths

1. Comprehensive Test Coverage: The test suite is excellent with 60+ valid cases and 50+ invalid cases covering edge cases, circular references, nested types, and both configuration modes.

2. Proper Circular Reference Detection: The isDeeplyReferencingType function correctly identifies circular type references (e.g., interface Foo { [key: string]: Foo; }) which prevents invalid conversions.

3. Robust Type Checking: The implementation properly handles:

   • Multiple key types (string, number, symbol)
   • Union types as keys
   • Mapped types
   • Generic interfaces
   • Nested type structures

4. Follows Codebase Patterns: The code structure matches other rules like consistent_generic_constructors with proper option parsing and rule listener setup.

🐛 Issues Found

1. Incorrect Rule Description (Line 14)

The comment says "enforces consistent usage of type imports" but should say "enforces consistent usage of indexed object types".

// CURRENT (incorrect):
// ConsistentIndexedObjectStyleRule enforces consistent usage of type imports

// SHOULD BE:
// ConsistentIndexedObjectStyleRule enforces consistent usage of indexed object types

2. Redundant Node Kind Check

In multiple listeners (lines 48, 95, 126, 152), you check the node kind twice - once in the listener registration and again inside the handler.

// Lines 43-50 - redundant check
ast.KindInterfaceDeclaration: func(node *ast.Node) {
    if opts.Style != "record" {
        return
    }
    
    if node.Kind != ast.KindInterfaceDeclaration { // <- This is redundant
        return
    }

The listener is already registered for that specific kind, so the second check is unnecessary. This pattern appears in all four listeners.

3. Missing Circular Reference Check for Type Literals (Line 90-117)

The ast.KindTypeLiteral handler doesn't check for circular references like the interface handler does. This could lead to false positives.

// Should add before line 114:
// Check for circular references (similar to interface handler)
indexSig := member.AsIndexSignatureDeclaration()
if indexSig == nil {
    return
}

// Need to get the parent type alias name for circular check
// This is trickier for type literals without a named parent

Note: This is complex because type literals may not have a parent name to check against. You may need to add context tracking or skip this check for anonymous type literals.

4. Unused Function (Line 430-439)

The getSourceText function is defined but never used in the codebase. It should either be:

• Removed if not needed
• Used if it was intended for autofix functionality
• Kept with a comment explaining it's for future use

5. Incomplete Constructor Type Handling (Line 375)

The code checks for ast.KindConstructorType but only handles ast.KindFunctionType. Constructor types should also be checked:

case ast.KindFunctionType, ast.KindConstructorType:
    // For function/constructor types, check parameters and return type
    if typeNode.Kind == ast.KindFunctionType {
        // ... handles function type
    }
    // Missing: else if typeNode.Kind == ast.KindConstructorType { ... }

🎯 Potential Issues

6. Missing readonly Modifier Support

The rule doesn't appear to handle readonly modifiers on index signatures. Looking at the test cases:

• Line 89: type Foo = { readonly [key: string]: any }; is marked as invalid
• But there's no special handling for readonly in the code

This should work correctly, but consider whether readonly Record<K, V> vs Readonly<Record<K, V>> matters for the conversion suggestion.

7. Optional Modifier Support (?) for Mapped Types

Line 141 in tests has: type Foo = { [K in string]?: any };

This is flagged as invalid (should prefer Record), but Record<K, V> doesn't support optional values. This might be a false positive unless you intend to suggest Partial<Record<K, V>>.

📝 Code Quality Suggestions

1. Add godoc comments for all exported and helper functions
2. Consider extracting the recursive type checking into a separate utility package if other rules need it
3. Add nil safety checks in more places (though current implementation seems safe)
4. Consider adding context limit to checkTypeReference to prevent infinite recursion in pathological cases

🔒 Security & Performance

• ✅ No security concerns identified
• ✅ Recursive functions have proper nil checks
• ⚠️ Deep recursion in checkTypeReference could be slow on deeply nested types, but unlikely to be problematic in practice
• ✅ No external dependencies or unsafe operations

🧪 Test Coverage

• ✅ Excellent coverage of valid and invalid cases
• ✅ Both configuration modes tested
• ✅ Edge cases covered
• ⚠️ Could add tests for:
  • Extremely deeply nested types (performance)
  • Malformed AST nodes (error handling)
  • Mixed readonly/optional modifiers

📋 Recommendations

Must Fix (Blocking):

1. Fix the incorrect rule description comment

Should Fix (High Priority):
2. Remove redundant node kind checks
3. Handle constructor types in checkTypeReference
4. Clarify optional modifier handling for mapped types

Nice to Have:
5. Add circular reference check for type literals (or document why it's skipped)
6. Remove or document unused getSourceText function
7. Add godoc comments for helper functions

🎉 Overall Assessment

This is a solid implementation that demonstrates good understanding of the TypeScript AST and the rslint architecture. The core logic is sound, test coverage is excellent, and the code follows project conventions. The issues identified are mostly minor refinements that would improve code quality and correctness.

Recommendation: ✅ Approve with minor changes

Great work on porting this complex rule! 🚀

[claude]

Code Review: consistent-indexed-object-style Rule

Overall Assessment

This is a well-implemented PR that successfully ports the TypeScript-ESLint consistent-indexed-object-style rule to rslint. The implementation is comprehensive with excellent test coverage. Here's my detailed feedback:

---

✅ Strengths

1. Excellent Test Coverage

• 60+ valid test cases covering various scenarios
• 50+ invalid test cases with proper error detection
• Tests cover both configuration modes (record and index-signature)
• Edge cases properly handled: circular references, nested types, function signatures, generic types
• Test organization is clear and well-commented

2. Robust Circular Reference Detection

The implementation correctly identifies and skips circular type references (lines 77-80, 119-122), preventing false positives:

if isDeeplyReferencingType(interfaceDecl.Name(), indexSig.Type) {
    return
}

This is a critical feature that prevents the rule from incorrectly flagging recursive types like interface Foo { [key: string]: Foo; }.

3. Comprehensive Type Checking

• Handles all major TypeScript type constructs: interfaces, type literals, mapped types, Record types
• Properly validates Record key types (string, number, symbol, literal types, unions)
• Recursive type traversal in checkTypeReference covers arrays, unions, intersections, tuples, etc.

4. Proper Option Parsing

The rule correctly handles multiple option formats:

• Array format: ["index-signature"]
• Object format: {style: "record"}
• String format: "index-signature"

---

🐛 Potential Issues

1. Incorrect Comment in Rule Definition (Line 14)

// ConsistentIndexedObjectStyleRule enforces consistent usage of type imports

Issue: The comment says "type imports" but should say "indexed object types"

Fix: Update to:

// ConsistentIndexedObjectStyleRule enforces consistent usage of indexed object types

2. Redundant Node Kind Checks

In multiple listener functions (lines 48-49, 95-96, 136-137, 162-163), there are redundant kind checks:

if node.Kind != ast.KindInterfaceDeclaration {
    return
}

Issue: This check is redundant since the listener is already registered for that specific kind in the RuleListeners map.

Suggestion: Remove these redundant checks for cleaner code, or add a comment explaining if there's a specific reason for the defensive programming.

3. Unused Function (Lines 463-472)

func getSourceText(ctx rule.RuleContext, start, end int) string {
    // ... implementation
}

Issue: This function is defined but never used anywhere in the codebase.

Fix: Remove the unused function unless it's planned for future use.

4. Missing Constructor Type Handling

The checkTypeReference function handles KindFunctionType but only partially handles KindConstructorType (lines 385-404):

case ast.KindFunctionType, ast.KindConstructorType:
    if typeNode.Kind == ast.KindFunctionType {
        // ... only handles FunctionType
    }

Issue: Constructor types are listed in the case but not actually handled.

Fix: Either remove KindConstructorType from the case or add proper handling for it.

5. Mapped Type Edge Case

The canConvertMappedTypeToRecord function (lines 241-277) checks if the mapped type references the type parameter to prevent conversion, but it doesn't check for:

• Readonly modifiers that might need Readonly<Record<...>>
• Optional modifiers that might need Partial<Record<...>>

Impact: Medium - mapped types with modifiers might be incorrectly flagged

Example:

type Foo = { readonly [K in string]: any };  // Should suggest Readonly<Record<string, any>>
type Bar = { [K in string]?: any };          // Should suggest Partial<Record<string, any>>

---

🔍 Code Quality Observations

1. Good Defensive Programming

• Extensive nil checks throughout the code
• Safe type assertions with nil checks
• Bounds checking in getSourceText (even though unused)

2. Clear Code Organization

• Helper functions are well-named and focused
• Logical separation of concerns (checking vs. reporting)
• Consistent error handling pattern

3. Minor: Code Duplication

The functions isDeeplyReferencingType (lines 279-291) and isDeeplyReferencingTypeParam (lines 293-305) are identical.

Suggestion: Consider consolidating these into a single function or add comments explaining the distinction if there's a semantic difference.

---

⚡ Performance Considerations

Positive:

• Early returns prevent unnecessary processing
• No obvious performance bottlenecks
• Efficient use of AST traversal

Consideration:

• The recursive checkTypeReference function could potentially hit deep call stacks with very nested types, but this is unlikely in practice and proper for the use case.

---

🔒 Security Concerns

No security issues identified. The code:

• Doesn't execute arbitrary code
• Doesn't access filesystem outside of normal operations
• Properly handles user input through option parsing
• No unsafe type assertions or panics

---

📋 Test Quality

Excellent Coverage:

• ✅ Both configuration modes tested
• ✅ Edge cases covered (circular refs, nested types, generics)
• ✅ Positive and negative test cases
• ✅ Tests match TypeScript-ESLint behavior

Suggestions:

1. Consider adding a test for extremely nested types to verify no stack overflow
2. Test with TypeScript 5.x specific features if applicable

---

🎯 Recommendations

High Priority:

1. ✅ Fix the incorrect comment on line 14
2. ✅ Remove the unused getSourceText function
3. ✅ Fix or remove incomplete ConstructorType handling

Medium Priority:

4. Consider handling readonly/optional modifiers in mapped types
5. Consolidate duplicate functions or document why they exist separately

Low Priority:

6. Remove redundant kind checks if not needed for defensive programming
7. Add inline comments explaining the circular reference detection algorithm

---

📝 Summary

Verdict: Approve with minor fixes recommended

This is high-quality code that successfully implements the TypeScript-ESLint rule behavior. The implementation is robust, well-tested, and follows Go best practices. The issues identified are minor and mostly cosmetic. The core logic for detecting and reporting violations is sound.

Recommended next steps:

1. Address the high-priority items (incorrect comment, unused function, constructor type handling)
2. Consider the medium-priority suggestions for completeness
3. Merge once fixes are applied

Great work on this implementation! The test coverage and edge case handling (especially circular references) are particularly commendable.

---

🤖 Generated with Claude Code

[claude]

Pull Request Review: TypeScript-ESLint consistent-indexed-object-style Rule

Summary

This PR successfully ports the consistent-indexed-object-style rule from TypeScript-ESLint to rslint. The implementation is well-structured and follows the established patterns in the codebase. Overall, this is a solid contribution with comprehensive test coverage.

Code Quality & Best Practices

Strengths

• Excellent Code Organization: The rule implementation follows the established pattern seen in other rules (e.g., class_literal_property_style)
• Comprehensive Helper Functions: Well-separated concerns with dedicated helpers for different type checks (isRecordType, isValidRecordKeyType, canConvertMappedTypeToRecord, etc.)
• Proper AST Traversal: Correctly uses the listener pattern with appropriate node kind filters
• Good Null Safety: Consistent null checks throughout (if node == nil, if typeRef == nil, etc.)
• Configuration Handling: Supports multiple option formats (string, array, map) which is user-friendly

Areas for Improvement

1. Incorrect Documentation Comment (Line 13)

// ConsistentIndexedObjectStyleRule enforces consistent usage of type imports

This should be:

// ConsistentIndexedObjectStyleRule enforces consistent usage of indexed object types

2. Redundant Kind Checks

In multiple places, there are redundant kind checks:

// Lines 47-49, 94-96, 136-138, 162-164
if node.Kind != ast.KindInterfaceDeclaration {
    return
}

These checks are unnecessary since the listener is already registered for that specific ast.Kind. The framework ensures only nodes of that kind will be passed to the listener. Removing these would improve performance slightly.

3. Unused Helper Function

The getSourceText function (lines 454-463) is defined but never used in the codebase. Consider removing it or documenting why it's there for future use.

4. Potential Performance Concern in Recursive Type Checking

The checkTypeReference function performs deep recursive traversal without depth limiting. For deeply nested types, this could potentially cause stack overflow. Consider adding a depth limit parameter:

func checkTypeReferenceWithDepth(targetName string, typeNode *ast.Node, depth int) bool {
    if depth > 100 { // reasonable limit
        return false
    }
    // ... rest of implementation with depth+1 for recursive calls
}

Potential Bugs & Issues

1. Mapped Type Analysis May Be Too Strict

In canConvertMappedTypeToRecord (line 242-277), the check:

if mappedType.Type != nil && isDeeplyReferencingTypeParam(typeParam.Name(), mappedType.Type) {
    return false
}

This prevents conversion when the mapped type value references the key type parameter (e.g., { [K in string]: K }), which is correct. However, the function name isDeeplyReferencingTypeParam is misleading - it checks for ANY reference, not just deep ones.

2. Missing Edge Cases in isValidRecordKeyType

The isValidRecordKeyType function (line 219-239) accepts ast.KindLiteralType but doesn't validate that it's actually a string/number/symbol literal. TypeScript allows Record<"a" | "b", any> but not Record<true, any>. Consider adding validation:

case ast.KindLiteralType:
    literalType := typeNode.AsLiteralTypeNode()
    if literalType == nil || literalType.Literal == nil {
        return false
    }
    // Check that literal is string, number, or bigint
    switch literalType.Literal.Kind {
    case ast.KindStringLiteral, ast.KindNumericLiteral, ast.KindBigIntLiteral:
        return true
    default:
        return false
    }

3. Circular Reference Detection May Not Cover All Cases

The circular reference detection looks solid for direct references, but consider edge cases like:

type Foo = { [key: string]: Bar };
type Bar = Foo; // indirect circular reference

This is likely acceptable to flag, but worth documenting the intended behavior.

Performance Considerations

Positive Aspects

• Single AST Traversal: Uses the listener pattern efficiently
• Early Returns: Good use of early returns to avoid unnecessary processing
• Minimal Allocations: Doesn't create unnecessary temporary objects

Concerns

• Deep Recursion: As mentioned above, checkTypeReference could be optimized with depth limiting
• Redundant Checks: The kind checks mentioned earlier add unnecessary branching

Security Concerns

No security issues identified. The code:

• Doesn't perform any file I/O
• Doesn't execute external commands
• Properly handles untrusted input (AST nodes) with null checks
• Doesn't have any obvious injection vulnerabilities

Test Coverage

Excellent Coverage

• 110+ test cases covering both valid and invalid scenarios
• Tests both configuration modes (record and index-signature)
• Covers edge cases:
  • Empty interfaces/types
  • Mixed properties
  • Generic types
  • Circular references
  • Nested types
  • Function signatures
  • Comments
  • Complex value types

Test Quality

The tests are comprehensive and well-organized. They accurately port the TypeScript-ESLint test suite, which is a strong validation of correctness.

Missing Test Cases to Consider

1. Readonly modifiers on index signatures: interface Foo { readonly [key: string]: any; }
2. Symbol key type: type Foo = { [key: symbol]: any };
3. Template literal types as keys: type Foo = Record<\prefix-${string}`, any>;`
4. Multiple index signatures (TypeScript allows this in some cases)

Additional Observations

Positive Changes

1. Configuration Update: The rule is properly configured as "warn" in rslint.json, which is appropriate for a stylistic rule
2. Self-Application: The PR applies the rule to the codebase itself (website/env.d.ts), demonstrating dogfooding
3. Proper Registration: The rule is correctly registered in internal/config/config.go

Commit History

The commit history shows good iterative development:

1. Initial implementation
2. API compatibility fixes
3. Circular reference handling fix
4. Configuration adjustment

This demonstrates proper testing and refinement.

Recommendations

High Priority

1. Fix the documentation comment on line 13
2. Remove unused getSourceText function or document its purpose
3. Add depth limiting to checkTypeReference to prevent potential stack overflow

Medium Priority

1. Remove redundant node.Kind checks in listeners (lines 47-49, 94-96, 136-138, 162-164)
2. Improve validation in isValidRecordKeyType to handle literal type validation
3. Rename isDeeplyReferencingTypeParam to referencesTypeParam for clarity

Low Priority

1. Add additional test cases for edge cases mentioned above
2. Consider adding inline documentation for the circular reference detection algorithm

Conclusion

This is a high-quality PR that successfully ports a non-trivial TypeScript-ESLint rule to rslint. The implementation is robust, well-tested, and follows established patterns. The issues identified are mostly minor and don't block merging, but addressing them would improve code quality and maintainability.

Recommendation: Approve with minor suggested improvements.

Great work! 🎉