Enable full test for class-methods-use-this rule

[delino]

Summary

• Uncommented the full test file for class-methods-use-this rule in rstest.config.mts
• Extended implementation to support ignoreClassesThatImplementAnInterface option (boolean or "public-fields")
• Extended implementation to support ignoreOverrideMethods option
• All tests now pass successfully

Context

This PR is a follow-up to #152 and #153, which enabled the core test. This PR enables the full test suite with advanced TypeScript-specific features.

Changes

Test Configuration (rstest.config.mts:38)

• Uncommented the line enabling the class-methods-use-this full test file

Implementation (internal/plugins/typescript/rules/class_methods_use_this/class_methods_use_this.go)

Extended the rule to support advanced TypeScript-ESLint options:

1. ignoreClassesThatImplementAnInterface:

   • When true: ignores all members of classes that implement interfaces
   • When "public-fields": only ignores public members, still checks private/protected members
   • When false: checks all members (default behavior)

2. ignoreOverrideMethods:

   • When true: ignores methods with the override modifier
   • When false: checks override methods (default behavior)

3. Implementation details:

   • Added helper functions to check if a class implements an interface
   • Added helper to check if a member has the override modifier
   • Added logic to determine whether to ignore members based on:
     • Class interface implementation status
     • Member visibility (public/private/protected)
     • Override modifier presence
   • Applied these checks to both methods and property initializers

Test Snapshots

• Created snapshot file with 40 test cases covering:
  • Basic method detection
  • Private/protected methods
  • Getter/setter detection
  • Classes implementing interfaces
  • Override methods
  • Property initializers
  • Combination of options

Test Results

All tests pass successfully:

• Valid test cases: ✓ (3.53s)
• Invalid test cases: ✓ (4.06s)
• Total: 2 passed

TypeScript-ESLint Reference

The implementation follows the behavior of the TypeScript-ESLint class-methods-use-this rule:
https://typescript-eslint.io/rules/class-methods-use-this/

🤖 Generated with Claude Code

[kdy1]

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

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

Enable the test for the complete class-methods-use-this rule and ensure it passes.

Objective

Uncomment the test file './tests/typescript-eslint/rules/class-methods-use-this/class-methods-use-this.test.ts' in /home/runner/work/rslint/rslint/packages/rslint-test-tools/rstest.config.mts (line 38) and ensure all tests pass.

Documentation & Resources

• TypeScript-ESLint class-methods-use-this rule: https://typescript-eslint.io/rules/class-methods-use-this/
• ESLint base rule: https://eslint.org/docs/latest/rules/class-methods-use-this
• TypeScript class semantics: https://www.typescriptlang.org/docs/handbook/2/classes.html

Implementation Steps

1. Uncomment the test file in rstest.config.mts (line 38)
2. Run tests
3. Fix any remaining implementation issues in the Go code

Critical Requirement

If CI fails, ensure complete implementation of class-methods-use-this:

• Build on previous core implementation
• Handle all edge cases: decorators, overloads, parameter properties
• Support TypeScript-specific features like abstract classes and private fields
• Ensure proper handling of inheritance and overridden methods
• Test with complex class hierarchies

Success Criteria

• Test file uncommented
• All full tests pass
• CI succeeds

---

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

[claude]

Code Review - PR #154: Enable full test for class-methods-use-this rule

Summary

This PR successfully extends the class-methods-use-this rule implementation to support advanced TypeScript-ESLint features: ignoreClassesThatImplementAnInterface and ignoreOverrideMethods options. The implementation is well-structured and comprehensive with 40 test cases passing.

---

✅ Strengths

1. Comprehensive Test Coverage: 40 test cases covering various scenarios including interface implementations, override methods, visibility modifiers, and edge cases.

2. Clean Code Structure: The implementation follows existing patterns in the codebase and maintains consistency with other rules like no_empty_function.

3. Well-Documented: Clear inline comments explaining the logic, especially for the complex shouldIgnoreInterfaceImpl function.

4. Proper Option Handling: Correctly handles the ignoreClassesThatImplementAnInterface option as both boolean and string ("public-fields") types.

5. Incremental Approach: Building on PRs Enable test for class-methods-use-this rule #152 and Enable test for class-methods-use-this rule #153 shows good development practice.

---

🔍 Code Quality Issues

1. Redundant Code Duplication (Lines 141-161)

The private name checking logic is duplicated across 4 different node types. Consider extracting this into a helper function:

// Helper to check if a member has a private name
isPrivateName := func(node *ast.Node) bool {
    var name *ast.Node
    switch node.Kind {
    case ast.KindMethodDeclaration:
        if method := node.AsMethodDeclaration(); method != nil {
            name = method.Name()
        }
    case ast.KindPropertyDeclaration:
        if prop := node.AsPropertyDeclaration(); prop != nil {
            name = prop.Name()
        }
    case ast.KindGetAccessor:
        if accessor := node.AsGetAccessorDeclaration(); accessor != nil {
            name = accessor.Name()
        }
    case ast.KindSetAccessor:
        if accessor := node.AsSetAccessorDeclaration(); accessor != nil {
            name = accessor.Name()
        }
    default:
        return false
    }
    
    if name != nil {
        _, nameType := utils.GetNameFromMember(ctx.SourceFile, name)
        return nameType == utils.MemberNameTypePrivate
    }
    return false
}

Location: class_methods_use_this.go:141-161

---

2. Redundant Function Call (Lines 85-95)

The getParentClass function duplicates logic from isInClass. Consider refactoring:

// Refactor to reuse getParentClass
isInClass := func(node *ast.Node) bool {
    return getParentClass(node) != nil
}

Location: class_methods_use_this.go:74-95

---

3. Nil Check Redundancy (Line 119)

The nil check for opts.IgnoreClassesThatImplementAnInterface is redundant since you also check for false on the same line. In Go, nil == false is not true for interface types. Consider:

// Current (line 119)
if opts.IgnoreClassesThatImplementAnInterface == nil || opts.IgnoreClassesThatImplementAnInterface == false {
    return false
}

// Improved - handle nil and false separately for clarity
if opts.IgnoreClassesThatImplementAnInterface == nil {
    return false
}
if boolVal, ok := opts.IgnoreClassesThatImplementAnInterface.(bool); ok && !boolVal {
    return false
}

Location: class_methods_use_this.go:119-121

---

🐛 Potential Bugs

1. Edge Case: Multiple Interface Implementation

The classImplementsInterface function returns true if any heritage clause has an implements keyword. This is correct, but the function name could be more precise (e.g., classHasImplementsClause).

Location: class_methods_use_this.go:98-115
Severity: Low (naming clarity, not a functional bug)

---

2. Accessor Auto-Accessors Edge Case (Lines 151-161)

The code checks for getters/setters, but TypeScript also has "accessor" auto-accessors. Verify the test cases cover accessor keyword properties (I see test cases 4-6 do cover this - good!).

Location: Test coverage verified ✓

---

⚡ Performance Considerations

1. Repeated Parent Traversal (Lines 86-95, 123)

Every call to getParentClass traverses the parent chain. For deeply nested classes, this could be optimized by caching the parent class during the initial traversal.

Impact: Minor - only called once per method, but worth noting.

---

2. Heritage Clause Iteration (Lines 108-113)

The loop could short-circuit earlier:

for _, clauseNode := range heritageClauses.Nodes {
    clause := clauseNode.AsHeritageClause()
    if clause != nil && clause.Token == ast.KindImplementsKeyword {
        return true  // ✓ Already does this - good!
    }
}

Location: Already optimized ✓

---

🔒 Security Concerns

No security issues identified. The code:

• ✅ Safely handles type assertions with ok checks
• ✅ Validates array bounds before access
• ✅ Doesn't execute user input
• ✅ Doesn't perform file system operations

---

🧪 Test Coverage

Excellent coverage with 40 test cases including:

• ✅ Basic methods (public/private/protected)
• ✅ Getters and setters
• ✅ Private names (#method)
• ✅ Classes implementing interfaces
• ✅ Override methods
• ✅ Property initializers
• ✅ Combinations of options

Suggestion: Consider adding test cases for:

1. Classes implementing multiple interfaces
2. Classes both extending and implementing (e.g., class Foo extends Bar implements Baz)
3. Nested classes where inner class implements interface

---

📝 Best Practices & Style

1. Consistent Error Messages ✓

Error messages properly distinguish between regular and private members. Good!

2. Option Parsing ✓

Follows the same pattern as no_empty_function rule. Consistent with codebase.

3. Comment Quality

Comments are clear and helpful. Consider adding a package-level comment explaining the rule's purpose.

---

🎯 Recommendations

Priority 1 (Should fix before merge):

1. Extract the private name checking logic to reduce duplication (lines 141-161)
2. Refactor isInClass to use getParentClass to avoid redundant logic

Priority 2 (Nice to have):

1. Add test cases for multiple interface implementations
2. Add package-level documentation
3. Consider renaming classImplementsInterface for clarity

Priority 3 (Future optimization):

1. Cache parent class lookup if performance becomes an issue

---

✨ Overall Assessment

Verdict: ✅ Approve with minor suggestions

This is a solid implementation that:

• Correctly implements the TypeScript-ESLint spec
• Has comprehensive test coverage
• Follows existing code patterns
• Handles edge cases properly

The identified issues are mostly about code quality and maintainability rather than correctness. The rule works as intended, and the test coverage gives confidence in its behavior.

Great work on this incremental improvement to the linting rule! 🎉

---

Review generated by Claude Code - kdybot/rslint PR #154