Enable test for class-methods-use-this rule

[delino]

Summary

• Uncommented the test file for the class-methods-use-this rule in rstest.config.mts to enable testing
• Implemented the class-methods-use-this rule in Go
• Registered the rule in the configuration

Context

This PR is a follow-up to #151 which enabled the ban-tslint-comment rule. This PR enables the class-methods-use-this rule which detects class methods that don't use this and could be made static.

Changes

1. Test Configuration (rstest.config.mts:37)

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

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

   • Created new Go implementation for the class-methods-use-this rule
   • Detects class methods, getters, setters, and class fields that don't use this or super
   • Supports options:
     • exceptMethods: array of method names to ignore
     • enforceForClassFields: boolean to control checking of class fields (default: true)
   • Properly handles:
     • Instance methods, getters, setters
     • Private methods (with # prefix)
     • Generator methods
     • Arrow functions and function expressions in class fields
     • Nested functions and arrow functions (correctly scoping this)
   • Skips constructors, static methods, and abstract methods

3. Registration (internal/config/config.go)

   • Added import for the class_methods_use_this package
   • Registered the rule as @typescript-eslint/class-methods-use-this

Implementation Details

The rule implementation:

• Traverses class members (methods, getters, setters, properties)
• Checks if each member's body uses this or super keywords
• Handles nested function scoping correctly (doesn't look inside nested function expressions, but does check arrow functions as they capture this)
• Respects exception list for method names
• Reports errors with messageId 'missingThis' and appropriate description

Test Plan

• Run npm test to verify all tests pass
• Check CI pipeline for any failures
• Fix any implementation issues if tests fail in CI

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

[claude]

Code Review: Enable test for class-methods-use-this rule

Summary

This PR implements the class-methods-use-this rule in Go and enables its test. Overall, the implementation follows the repository patterns and correctly translates the TypeScript ESLint rule logic. However, there are several issues that need to be addressed.

---

Critical Issues

1. Incorrect AST Traversal Logic (Bug)

Location: internal/plugins/typescript/rules/class_methods_use_this/class_methods_use_this.go:64-99

The usesThisOrSuper function has a critical bug in how it handles nested functions:

// Don't descend into nested function expressions or arrow functions
// unless they're arrow functions (which capture 'this')
if child.Kind == ast.KindFunctionExpression || child.Kind == ast.KindFunctionDeclaration {
    functionDepth++
    if functionDepth > 0 {
        return false // Don't descend
    }
}

Problem: The functionDepth variable is initialized to 0, so functionDepth > 0 is always false on the first nested function, meaning the traversal will still descend into the first nested function. This should be:

if child.Kind == ast.KindFunctionExpression || child.Kind == ast.KindFunctionDeclaration {
    return false // Don't descend into nested functions
}

The functionDepth tracking is unnecessary since we're not tracking depth recursively - we just want to prevent descending into nested function expressions/declarations entirely.

2. Arrow Function Traversal Logic Issue

Location: internal/plugins/typescript/rules/class_methods_use_this/class_methods_use_this.go:76-82

The current implementation calls containsThisInArrow(child) but then continues traversal with return true. This is problematic:

if child.Kind == ast.KindArrowFunction {
    // Check if the arrow function uses 'this'
    if containsThisInArrow(child) {
        found = true
        return false
    }
    return true // Continue checking
}

Problem: After calling containsThisInArrow, you're still traversing into the arrow function's children with return true. This means you're checking the arrow function twice - once in the helper function and once in the main traversal. You should return false to prevent descending further:

if child.Kind == ast.KindArrowFunction {
    if containsThisInArrow(child) {
        found = true
    }
    return false // Don't descend, we already checked it
}

3. Incomplete Handling of Arrow Function Bodies

Location: internal/plugins/typescript/rules/class_methods_use_this/class_methods_use_this.go:302-310

When checking property declarations with arrow functions, the code only handles block bodies:

if init.Kind == ast.KindArrowFunction {
    fn := init.AsArrowFunction()
    if fn != nil && fn.Body != nil && fn.Body.Kind == ast.KindBlock {
        body = fn.Body
    }
}

Problem: Arrow functions can have expression bodies (e.g., () => this.value). You need to check expression bodies too:

if init.Kind == ast.KindArrowFunction {
    fn := init.AsArrowFunction()
    if fn != nil && fn.Body != nil {
        body = fn.Body
    }
}

---

Code Quality & Best Practices

4. Inconsistent Return Type from GetNameFromMember

Location: internal/plugins/typescript/rules/class_methods_use_this/class_methods_use_this.go:124

The GetNameFromMember function returns two values (string, MemberNameType), but the second value (which indicates if it's private) is being used inconsistently:

name, isPrivate := utils.GetNameFromMember(ctx.SourceFile, method.Name())
if isPrivate {
    // ... handle private
}

Issue: The variable is named isPrivate, but MemberNameType is an enum with multiple values (Private, Quoted, Normal, Expression), not a boolean. This works due to Go's zero-value semantics, but it's misleading.

Suggestion: Use a more descriptive variable name:

name, nameType := utils.GetNameFromMember(ctx.SourceFile, method.Name())
if nameType == utils.MemberNameTypePrivate {
    // ... handle private
}

5. Repetitive Method Name Extraction Logic

Location: internal/plugins/typescript/rules/class_methods_use_this/class_methods_use_this.go:124-177, 202-221

The logic for getting method names and building display names is duplicated across multiple sections of the code.

Suggestion: Extract into helper functions at the package level to improve maintainability:

func getMethodNameFromNode(ctx rule.RuleContext, node *ast.Node) string {
    // Consolidate the name extraction logic
}

---

Performance Considerations

6. Inefficient String Concatenation

Location: Multiple locations in getMethodName function

The function builds strings using concatenation in multiple places:

return "private getter " + name
return "getter '" + name + "'"

Impact: For this use case (small strings, infrequent calls), the performance impact is negligible. However, for consistency with Go best practices, consider using fmt.Sprintf for clarity:

return fmt.Sprintf("private getter %s", name)
return fmt.Sprintf("getter '%s'", name)

7. Traversal Efficiency

The usesThisOrSuper function uses ast.ForEachChild with a closure that captures the found variable. This is a reasonable approach, but note that ForEachChild will continue traversing even after finding this.

The current implementation does return false to stop traversal when found = true, which is good. However, ensure this is working as expected in the actual ast.ForEachChild implementation.

---

Security Concerns

No significant security issues identified. The rule implementation:

• Doesn't execute arbitrary code
• Doesn't perform file system operations
• Doesn't handle user input beyond parsing options
• Uses standard AST traversal patterns

---

Test Coverage

8. Test File Enabled

The PR correctly enables the test file in rstest.config.mts:

'./tests/typescript-eslint/rules/class-methods-use-this/class-methods-use-this-core.test.ts',

Good: The test file includes comprehensive test cases covering:

• Basic methods without this
• Nested functions (which should not trigger the rule)
• Arrow functions (which should be checked)
• Methods with comments containing "this"
• Property access like window.this (false positives)

9. Missing TypeScript-Specific Tests

Location: The file class-methods-use-this.test.ts is still commented out in rstest.config.mts:38

Question: Are there TypeScript-specific test cases in the commented test file that should also be enabled? The PR description doesn't explain why only the core tests are enabled.

---

Additional Observations

10. Good Practices Followed

✅ Proper option parsing with defaults
✅ Skip constructors, static methods, and abstract methods
✅ Handles private identifiers with # prefix
✅ Supports generator methods
✅ Respects exception list for method names
✅ Clear message IDs and descriptions

11. Documentation

The PR description is excellent and provides:

• Clear summary of changes
• Implementation details
• Reference to TypeScript-ESLint rule
• Test plan checklist

Suggestion: Consider adding code comments explaining the nuanced behavior around nested functions vs arrow functions, as this is a subtle aspect of JavaScript's this binding.

---

Recommendations

High Priority (Must Fix)

1. Fix the functionDepth bug in usesThisOrSuper (chore(ci): Add Claude Code action #1)
2. Fix arrow function traversal to prevent double-checking (chore: update rule-manifest.json [auto] #2)
3. Handle arrow function expression bodies (feat: Set up TypeScript ESTree infrastructure #3)

Medium Priority (Should Fix)

4. Use proper enum comparison instead of treating MemberNameType as boolean (feat: Add TypeScript version checking to typescript-estree #4)
5. Extract duplicate method name logic into helper functions (feat: Port complete ESTree type definitions from typescript-estree to Go #5)

Low Priority (Nice to Have)

6. Consider using fmt.Sprintf for string formatting (feat: Port comment handling logic from TypeScript-ESTree #6)
7. Add inline comments explaining this binding semantics (feat: Comprehensive testing infrastructure for rule development #11)
8. Clarify why TypeScript-specific tests are not enabled (feat: Implement main parsing functionality for typescript-estree #9)

---

Test Plan Verification

Before merging:

• Fix the critical bugs identified above
• Run npm test to verify all tests pass
• Manually test edge cases:
  • Arrow functions with expression bodies
  • Deeply nested functions
  • Methods with both nested functions and arrow functions
• Verify the fix handles all test cases in class-methods-use-this-core.test.ts

---

Great work on this implementation! The overall structure is solid and follows the repository patterns well. Once the critical bugs are fixed, this should work correctly. Let me know if you need clarification on any of the feedback.

[kdy1]

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

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 class-methods-use-this core functionality and ensure it passes.

Objective

Uncomment the test file './tests/typescript-eslint/rules/class-methods-use-this/class-methods-use-this-core.test.ts' in /home/runner/work/rslint/rslint/packages/rslint-test-tools/rstest.config.mts (line 37) 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
• Go AST visitor pattern: https://disaev.me/p/writing-useful-go-analysis-linter/

Implementation Steps

1. Uncomment the test file in rstest.config.mts (line 37)
2. Run tests to check current state
3. If tests fail, examine or create Go implementation at /home/runner/work/rslint/rslint/internal/plugins/typescript/rules/class_methods_use_this/
4. Fix bugs or implement missing functionality

Critical Requirement

If CI fails, implement the class-methods-use-this rule from scratch:

• Reference: https://typescript-eslint.io/rules/class-methods-use-this/
• Detect class methods that don't use this and could be static
• Support options: exceptMethods (array of method names to ignore), enforceForClassFields (boolean)
• Handle instance methods, getters, setters
• Skip constructors, static methods, and abstract methods
• Use TypeScript type information to handle decorators properly
• Traverse AST to find class method declarations
• Check method body for this references
• Follow rslint codebase patterns

Success Criteria

• Test file uncommented in rstest.config.mts
• All core tests pass
• CI succeeds

---

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

[claude]

Pull Request Review: class-methods-use-this Rule Implementation

Overall Assessment

This PR implements the class-methods-use-this rule in Go, following the TypeScript-ESLint specification. The implementation is generally well-structured and comprehensive. Below are my findings across code quality, potential issues, performance, security, and test coverage.

---

Code Quality & Best Practices

Strengths ✅

1. Good code organization: The rule is well-structured with clear helper functions (isExceptedMethod, isInClass, getMethodName)
2. Comprehensive scope tracking: The scopeInfo stack-based approach correctly handles nested scopes
3. Proper options parsing: Handles both array and map option formats
4. Clear comments: Helper functions are well-documented
5. Consistent with codebase patterns: Follows the same patterns as ban_tslint_comment and other rules

Areas for Improvement 🔧

1. Potential Logic Issue with Arrow Functions in Class Fields (class_methods_use_this.go:352)

// Arrow function listener (exits property initializer if needed)
rule.ListenerOnExit(ast.KindArrowFunction): exitPropertyInit,

Issue: This listener calls exitPropertyInit for ALL arrow functions, but exitPropertyInit should only be called for arrow functions that are property initializers. This could lead to incorrect scope popping if an arrow function appears elsewhere.

Expected behavior:

• Arrow function as property initializer: class A { foo = () => {} } ✅ Should exit
• Arrow function in method body: class A { foo() { const x = () => {} } } ❌ Should NOT exit property scope

Suggested fix: Add a check in exitPropertyInit to verify the arrow function is actually a property initializer:

exitPropertyInit := func(node *ast.Node) {
    if currentScope != nil && currentScope.node == node {
        // Only exit if this arrow function is a direct initializer
        parent := node.Parent
        if parent != nil && parent.Kind == ast.KindPropertyDeclaration {
            // ... existing code ...
        }
        currentScope = currentScope.upper
    }
}

2. Missing Listener for FunctionExpression Exit in Property Initializers (class_methods_use_this.go:346-347)

The code has entry listeners for function expressions in properties (enterProperty at line 233) but the exit listener for ast.KindFunctionExpression (line 347) calls exitNestedFunction, not exitPropertyInit. This means:

• class A { foo = function() {} } might not be properly checked

Issue: Function expressions as property initializers should use exitPropertyInit, but they're using exitNestedFunction which marks hasThis = true by default.

Test case to verify: Line 227-237 in test file expects error for class A { foo = function() {} }

3. Redundant isInClass Check (class_methods_use_this.go:176-178)

The isInClass check in enterMethod may be unnecessary since methods/getters/setters can only exist within classes in TypeScript/JavaScript. This is a minor optimization opportunity.

4. Variable Shadowing in Options Parsing (class_methods_use_this.go:32)

if opts, ok := optsArray[0].(map[string]interface{}); ok {
    optsMap = opts  // 'opts' shadows the outer 'opts' variable
}

While this works, it's a subtle naming issue that could be clearer:

if optsData, ok := optsArray[0].(map[string]interface{}); ok {
    optsMap = optsData
}

---

Potential Bugs 🐛

Critical Issues

1. Arrow Functions in Class Fields May Not Be Properly Handled
As mentioned above in code quality, the exit listener for arrow functions (line 352) unconditionally calls exitPropertyInit which could cause scope stack corruption.

Scenario that might fail:

class A {
  foo() {
    const callback = () => this; // This 'this' should mark foo() as using 'this'
    return callback();
  }
}

Expected: No error (method uses this via arrow function)
Potential behavior: Might incorrectly report error if scope is popped prematurely

2. Nested Function Scope Handling
Lines 313-324 create a boundary scope for function expressions/declarations with hasThis: true. However, this might not propagate the this usage to the parent scope correctly.

Scenario:

class A {
  foo() {
    const arrow = () => this; // Should mark foo() as using 'this'
  }
}

The code correctly doesn't create a boundary for arrow functions (line 316 only checks for KindFunctionExpression and KindFunctionDeclaration), so this should work. ✅

---

Performance Considerations ⚡

Current Performance

The implementation is efficient:

1. O(n) traversal: Single AST walk
2. Minimal allocations: Stack-based scope tracking with pointer references
3. No redundant lookups: Caches scope information

Minor Optimizations

1. exceptMethods lookup (line 54-60): Consider using a map[string]bool instead of linear search for O(1) lookup:

exceptMethodsMap := make(map[string]bool)
for _, method := range opts.ExceptMethods {
    exceptMethodsMap[method] = true
}
isExceptedMethod := func(methodName string) bool {
    return exceptMethodsMap[methodName]
}

This matters if exceptMethods has many entries.

2. Repeated GetNameFromMember calls (lines 186, 192, 198, 260): These are called during exception checking and could be optimized by caching the result.

---

Security Concerns 🔒

No security issues identified. ✅

The implementation:

• Doesn't execute user code
• Doesn't perform file I/O
• Doesn't use unsafe operations
• Properly handles nil checks (lines 78, 115, 128, 141, 185, 191, 197, 238, 259, 290)

---

Test Coverage 🧪

Test File Analysis

The test file (class-methods-use-this-core.test.ts) is comprehensive with 46 test cases covering:

Valid cases (15 tests): ✅

• Constructors
• Methods using this and super
• Static methods
• Arrow functions inheriting this
• Exception list functionality
• enforceForClassFields: false option

Invalid cases (31 tests): ✅

• Methods without this
• Getters/setters without this
• Private methods (#foo)
• Class fields with function/arrow expressions
• Generator methods
• Computed property names
• Nested functions with this in wrong scope

Edge Cases Well Covered

• Private identifiers: Lines 146-165, 275-309
• Generator methods: Lines 167-223
• Nested classes: Lines 311-345
• Static blocks: Line 443-445

Recommendation

The test coverage appears excellent. When the PR runs CI, monitor for:

1. Arrow function edge cases
2. Deeply nested function scenarios
3. Complex property initializers

---

Recommendations Summary

High Priority 🔴

1. Fix arrow function exit listener to only call exitPropertyInit for actual property initializers
2. Verify function expression handling in property initializers works correctly

Medium Priority 🟡

1. Consider optimizing exceptMethods with a map for O(1) lookup
2. Add integration test for the specific edge cases mentioned

Low Priority 🟢

1. Remove redundant isInClass checks if they're always true
2. Rename shadowed variables for clarity

---

Conclusion

This is a solid implementation with good test coverage. The main concerns are around the arrow function exit listener which could cause scope tracking issues. I recommend:

1. Before merging: Fix the arrow function exitPropertyInit logic
2. Monitor CI: Watch for test failures that might reveal the scope issues
3. Post-merge: Consider the performance optimizations if exceptMethods is commonly used with many entries

The implementation demonstrates good understanding of the TypeScript-ESLint rule and Go patterns. With the suggested fixes, this will be a robust addition to the codebase.

Great work! 🎉

[claude]

Code Review: Enable test for class-methods-use-this rule

Thank you for implementing the class-methods-use-this rule! This is a comprehensive implementation. Here's my detailed review:

Strengths

1. Well-structured scope tracking: The implementation correctly uses a stack-based approach with scopeInfo to track nested function scopes and whether this/super is used.

2. Comprehensive coverage: Handles methods, getters, setters, and class fields with proper consideration for modifiers (static, abstract).

3. Options parsing: Correctly handles both array and direct object formats for options, consistent with other rules in the codebase.

4. Good separation of concerns: Helper functions like isExceptedMethod, isInClass, and getMethodName make the code readable and maintainable.

Issues to Address

1. Potential bug with arrow function scope handling (internal/plugins/typescript/rules/class_methods_use_this/class_methods_use_this.go:367-371)

Arrow functions don't create their own this binding - they inherit from the enclosing scope. However, the current implementation only calls enterPropertyInit for arrow functions without creating a nested function boundary. This is correct for property initializers, but there's a potential issue:

ast.KindArrowFunction: func(node *ast.Node) {
    enterPropertyInit(node)
},

When an arrow function appears inside a method body (not as a property initializer), it should NOT create a scope boundary because it lexically captures this. The current code handles this correctly by not calling enterNestedFunction for arrow functions, which is good. However, consider adding a comment to clarify this intentional behavior:

// Arrow functions lexically capture 'this' from enclosing scope,
// so we don't create a boundary for nested arrows (only property initializers)
ast.KindArrowFunction: func(node *ast.Node) {
    enterPropertyInit(node)
},

2. Redundant method.Kind checks (internal/plugins/typescript/rules/class_methods_use_this/class_methods_use_this.go:82-88, 92-107)

In getMethodName, when node.Kind == ast.KindMethodDeclaration, you're checking method.Kind again for ast.KindGetAccessor and ast.KindSetAccessor. This appears incorrect - if the node is already a KindMethodDeclaration, it can't also be a KindGetAccessor. These checks should likely be removed:

if node.Kind == ast.KindMethodDeclaration {
    method := node.AsMethodDeclaration()
    if method != nil && method.Name() != nil {
        name, nameType := utils.GetNameFromMember(ctx.SourceFile, method.Name())
        if nameType == utils.MemberNameTypePrivate {
            // Remove these incorrect checks:
            // if method.Kind == ast.KindGetAccessor { ... }
            // if method.Kind == ast.KindSetAccessor { ... }
            if method.AsteriskToken != nil {
                return "private generator method " + name
            }
            return "private method " + name
        }
        // Similar cleanup for non-private branch
    }
}

3. Options field naming inconsistency

The struct field is EnforceForClassFields (Go naming convention), but the JSON tag doesn't use camelCase:

EnforceForClassFields  bool     `json:"enforceForClassFields"`

While this works, check if other rules in the codebase use camelCase or PascalCase for JSON tags. Looking at no_extraneous_class.go, it uses camelCase in JSON tags (e.g., allowConstructorOnly), so your implementation is consistent. This is correct.

4. Missing documentation comments

Following Go best practices, exported types and variables should have documentation comments:

// ClassMethodsUseThisOptions configures the class-methods-use-this rule
type ClassMethodsUseThisOptions struct {
    // ExceptMethods is a list of method names that are exempt from the rule
    ExceptMethods []string `json:"exceptMethods"`
    // EnforceForClassFields determines whether to check class field initializers
    EnforceForClassFields bool `json:"enforceForClassFields"`
}

// ClassMethodsUseThisRule enforces that class methods utilize 'this' or 'super'
var ClassMethodsUseThisRule = rule.CreateRule(...)

Performance Considerations

1. Linear search in isExceptedMethod: For small exception lists this is fine, but if someone has many excepted methods, consider using a map[string]bool instead:

exceptedMethods := make(map[string]bool)
for _, method := range opts.ExceptMethods {
    exceptedMethods[method] = true
}

isExceptedMethod := func(methodName string) bool {
    return exceptedMethods[methodName]
}

This changes O(n) lookup to O(1). However, for typical usage with 1-5 excepted methods, the current approach is fine.

2. Repeated parent traversal: isInClass walks up the parent chain on every method. Since you're already inside a class context when processing members, this check might be redundant. Consider whether this is necessary - in the typical case, methods should always be in a class if they reach the enterMethod function.

Security Considerations

No security concerns identified. The implementation:

• Doesn't perform any unsafe operations
• Doesn't execute user-provided code
• Doesn't access the file system
• Properly validates options with type assertions

Test Coverage

The PR enables the test file class-methods-use-this-core.test.ts, which is excellent. Ensure:

• All tests pass in CI
• Edge cases are covered (nested classes, decorators, etc.)
• The rule correctly handles TypeScript-specific syntax

Additional Suggestions

1. Consider extracting method name retrieval logic: The method name extraction code in enterMethod (lines 181-201) is duplicated logic from getMethodName. Consider refactoring to reuse code.

2. Scope boundary documentation: Add comments explaining why certain node types create scope boundaries and others don't, especially around arrow functions vs function expressions.

3. Test the actual implementation: Run the tests to ensure the implementation matches expected behavior:

   pnpm test

Summary

This is a solid implementation with good structure and comprehensive handling of edge cases. The main issues are:

1. Remove incorrect method.Kind checks in getMethodName
2. Add documentation comments for exported types
3. Consider adding clarifying comments for arrow function handling

The implementation correctly handles the complex scoping rules for this in JavaScript/TypeScript, which is not trivial. Great work on tackling the nested function and property initializer cases!

Recommendation: Address the issues mentioned above, particularly the redundant method.Kind checks, before merging.