feat: Port ESLint core rule no-constructor-return

[delino]

Summary

This PR ports the ESLint core rule no-constructor-return to rslint, disallowing return statements with values in class constructors.

Follow-up task of PR #75.

Implementation Details

• ✅ Created new rule in internal/rules/no_constructor_return/
• ✅ Detects return statements with values inside class constructors
• ✅ Allows bare return; statements (without values) for flow control
• ✅ Properly handles nested functions - returns in nested functions are ignored
• ✅ Registered in global rule registry

Rule Behavior

The rule prevents return statements with values in constructors, which typically indicate mistakes or misunderstandings about how JavaScript constructors work.

Invalid Patterns

// Returning values in constructors
class C { constructor() { return '' } }
class C { constructor() { return 1 } }
class C { constructor() { return {} } }
class C { constructor() { return this } }

// Conditional returns with values
class C { constructor(a) { if (!a) { return '' } } }

// Multiple returns
class C { constructor(x) { if (x) return 'yes'; return 'no'; } }

Valid Patterns

// No return statement
class C { constructor() { this.value = 1 } }

// Bare return for flow control
class C { constructor(a) { if (!a) { return } else { a() } } }
class C { constructor() { return } }

// Nested functions can return values
class C { constructor() { function fn() { return true } } }
class C { constructor() { this.fn = () => { return true } } }

// Methods can return values
class C { method() { return '' } }
class C { get value() { return '' } }

Test Coverage

• ✅ Ported all test cases from ESLint's test suite
• ✅ 21 valid test cases covering various scenarios
• ✅ 12 invalid test cases with expected error detection
• ✅ All tests passing
• ✅ Tests include:
  • Regular functions and arrow functions (allowed)
  • Classes with and without constructors
  • Bare return statements (allowed)
  • Nested functions in constructors
  • TypeScript-specific cases (parameter properties)
  • Multiple return statements
  • Various return value types

Test Plan

• Rule implementation follows rslint patterns
• All test cases ported from ESLint
• Tests pass (all 33 tests passing)
• Manual testing with example code
• Integration with existing linter setup

References

• ESLint Rule: https://eslint.org/docs/latest/rules/no-constructor-return
• ESLint Source: https://github.com/eslint/eslint/blob/main/lib/rules/no-constructor-return.js
• ESLint Tests: https://github.com/eslint/eslint/blob/main/tests/lib/rules/no-constructor-return.js
• Related PR feat: Port ESLint core rule no-constant-condition #75: feat: Port ESLint core rule no-constant-condition #75

Files Changed

• internal/config/config.go - Added rule registration (2 lines)
• internal/rules/no_constructor_return/no_constructor_return.go - Complete rule implementation (~80 lines)
• internal/rules/no_constructor_return/no_constructor_return_test.go - Comprehensive test suite (~200 lines)

🤖 Generated with Claude Code

[kdy1]

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

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 ESLint core rule no-constructor-return to rslint (Go implementation).

Documentation & Resources

• Rule documentation: https://eslint.org/docs/latest/rules/no-constructor-return
• ESLint source code: https://github.com/eslint/eslint/blob/main/lib/rules/no-constructor-return.js
• ESLint test cases: https://github.com/eslint/eslint/blob/main/tests/lib/rules/no-constructor-return.js

Rule Description

Disallows return statements in constructors.

Implementation Requirements

1. Create directory internal/rules/no_constructor_return/
2. Implement rule in no_constructor_return.go
3. Port all ESLint test cases
4. Create no_constructor_return_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]

Code Review for PR #76: Port ESLint core rule no-constructor-return

Thank you for this well-structured PR! I've reviewed the implementation and overall it looks solid. Here's my detailed feedback:

✅ Strengths

1. Excellent Test Coverage: 33 test cases (21 valid, 12 invalid) provide comprehensive coverage of edge cases including nested functions, TypeScript features, and multiple return types.

2. Clean Implementation: The code is well-organized with clear separation of concerns:

   • findEnclosingConstructor handles the tree traversal logic
   • isReturnStatementInConstructor validates the return statement
   • Proper handling of function boundaries to avoid false positives

3. Correct Behavior: Properly distinguishes between:

   • Bare return; statements (allowed for flow control)
   • Return statements with values (correctly flagged)
   • Returns in nested functions (correctly ignored)

4. Proper Registration: Rule is correctly registered in config.go and follows the established patterns.

5. Documentation: The PR description is thorough with clear examples of valid and invalid patterns.

🔍 Code Quality Observations

Minor: Consistency with Codebase Patterns

Looking at other rules like getter_return and constructor_super, I notice they sometimes use helper utilities. Consider:

1. In no_constructor_return.go:31-38: The switch statement stopping at function boundaries is correct, but you might want to verify if there are any other function-like constructs that should be included. For example:
   • ast.KindGetAccessor / ast.KindSetAccessor (if applicable)
   • Any other method-like constructs

Current code:

switch current.Kind {
case ast.KindFunctionDeclaration,
    ast.KindFunctionExpression,
    ast.KindArrowFunction,
    ast.KindMethodDeclaration:
    return nil
}

Suggestion: Double-check the AST types to ensure all function boundaries are covered. Based on the test cases, this appears correct, but it's worth verifying against the typescript-go shim.

Potential Edge Case

2. Constructor in expressions: The implementation should handle constructors correctly, but consider testing these edge cases:
   • Constructor in class expressions: const C = class { constructor() { return 1 } }
   • Constructors in nested classes (your tests cover this implicitly)

Looking at your test suite, you don't have a test for class expressions. Consider adding:

// Valid: nested class with its own constructor
{Code: `class Outer { method() { class Inner { constructor() { return 1 } } } }`},

// Invalid: class expression with return value
{Code: `const C = class { constructor() { return 1 } }`, Errors: [...]},

🎯 Performance Considerations

The implementation is efficient:

• ✅ Only listens to KindReturnStatement nodes (minimal overhead)
• ✅ Uses parent traversal only when needed
• ✅ Early returns prevent unnecessary computation
• ✅ No memory leaks or recursive issues

🔒 Security Concerns

No security issues identified. The rule is read-only and doesn't execute or modify code.

📊 Test Coverage Recommendations

Your test coverage is excellent. The only additions I'd suggest:

1. Class expressions (as mentioned above)
2. Edge case: Constructor with nested arrow function that returns

   // Valid: nested arrow in constructor
   {Code: `class C { constructor() { const fn = () => 1; return } }`},

🐛 Potential Bugs

No bugs found! The implementation correctly:

• ✅ Handles null checks appropriately
• ✅ Stops at function boundaries
• ✅ Only flags returns with values (not bare returns)
• ✅ Works with TypeScript constructs (parameter properties)

💡 Best Practices Suggestions

1. Consider adding a comment in no_constructor_return.go:24-26:

   // Check if this is a constructor method
   // Note: Both ast.KindConstructor and some ast.KindMethodDeclaration
   // nodes can represent constructors depending on the AST structure
   if current.Kind == ast.KindConstructor {
       return current
   }

2. Error message clarity: Your error message "Unexpected return statement in constructor." matches ESLint, which is good for familiarity. Consider if rslint has any conventions about whether to suggest fixes in messages.

📝 Documentation

The PR description is excellent. However, consider:

• Adding inline code comments explaining the function boundary logic
• If rslint has a docs folder for rules, ensure this rule is documented there

✅ Approval Recommendation

This PR is approved with minor suggestions. The implementation is:

• ✅ Correct and bug-free
• ✅ Well-tested
• ✅ Following codebase patterns
• ✅ Safe and efficient

The suggestions above are optional improvements that could be addressed in this PR or as follow-ups.

🔧 Action Items (Optional)

• Add test case for class expressions
• Consider adding inline comments for function boundary logic
• Verify all function-like AST node types are covered in the switch statement

Great work on this implementation! 🎉

[claude]

PR Review: no-constructor-return Rule Implementation

Thank you for this well-structured PR! The implementation of the no-constructor-return rule is solid and follows ESLint's behavior correctly. Here's my comprehensive review:

Overall Assessment

✅ LGTM with minor suggestions - This is a high-quality implementation that:

• Correctly ports the ESLint no-constructor-return rule
• Has excellent test coverage (33 tests: 21 valid, 12 invalid)
• Follows the repository's coding patterns and conventions
• Is properly registered in the global rule registry

---

Code Quality & Best Practices

Strengths ✨

1. Clean Implementation (no_constructor_return.go:1-81)

   • Simple, focused logic that does one thing well
   • Good separation of concerns with helper functions
   • Clear function naming and organization

2. Proper AST Traversal (no_constructor_return.go:16-44)

   • Correctly walks up the AST to find enclosing constructor
   • Properly stops at function boundaries to avoid false positives
   • Handles nested functions correctly (functions inside constructors can return values)

3. Test Coverage (no_constructor_return_test.go)

   • Comprehensive test suite covering all edge cases
   • Tests both TypeScript and JavaScript patterns
   • Includes nested function scenarios, parameter properties, etc.

Suggestions for Improvement 💡

1. Code Documentation (Minor)

The rule lacks package-level documentation. Consider adding:

// Package no_constructor_return implements the no-constructor-return ESLint rule.
// This rule disallows returning values from constructors in JavaScript/TypeScript classes.
//
// In JavaScript, returning a value from a constructor can lead to unexpected behavior:
// - Returning primitive values is ignored
// - Returning objects replaces the newly created instance
// This typically indicates a mistake or misunderstanding of constructor semantics.
package no_constructor_return

Location: internal/rules/no_constructor_return/no_constructor_return.go:1

2. Function Documentation (Minor)

While the function names are descriptive, adding comments would improve maintainability:

// findEnclosingConstructor walks up the AST from the given node to find the nearest
// enclosing constructor method. Returns nil if the node is not inside a constructor
// or if a function boundary is encountered (nested functions are not considered
// part of the constructor scope for this rule).
func findEnclosingConstructor(node *ast.Node) *ast.Node {

Location: internal/rules/no_constructor_return/no_constructor_return.go:16

---

Potential Bugs & Issues

✅ No Critical Issues Found

The implementation correctly handles all the tricky cases:

1. Bare returns are allowed ✅

   class C { constructor() { return } }  // OK - early return for control flow

2. Nested functions work correctly ✅

   class C { 
     constructor() { 
       function fn() { return true }  // OK - nested function can return
     } 
   }

3. Stops at function boundaries ✅

   • The switch statement in findEnclosingConstructor properly checks for all function types
   • Includes arrow functions, method declarations, etc.

Nitpick: Function Boundary Check

The function boundary check is thorough but could be more explicit. Consider documenting why each function type stops the search:

// Stop at function boundaries - returns in nested functions don't apply to outer constructor
switch current.Kind {
case ast.KindFunctionDeclaration,    // function foo() {}
     ast.KindFunctionExpression,       // const fn = function() {}
     ast.KindArrowFunction,            // const fn = () => {}
     ast.KindMethodDeclaration:        // method() {}
    // We've hit a nested function boundary, stop searching
    return nil
}

Location: internal/rules/no_constructor_return/no_constructor_return.go:31-38

---

Performance Considerations

✅ Efficient Implementation

1. Minimal AST traversal: Only walks up when checking return statements
2. Early returns: Exits quickly when conditions aren't met
3. No unnecessary allocations: Simple boolean checks

Potential Optimization (Optional)

Currently, the rule checks every return statement and walks up the tree. For files with many return statements, you could consider:

• Tracking when entering/exiting constructors to avoid repeated tree walks
• However, given the simplicity and speed of the current approach, this optimization is likely not necessary

---

Security Concerns

✅ No Security Issues

The rule is purely static analysis with no:

• External I/O
• Dynamic code execution
• User input processing beyond AST nodes

---

Test Coverage

✅ Excellent Coverage

Valid Cases (21 tests):

• Regular functions ✅
• Classes without constructors ✅
• Bare return statements ✅
• Nested functions ✅
• Methods and getters ✅
• TypeScript parameter properties ✅

Invalid Cases (12 tests):

• Basic return with value ✅
• Conditional returns ✅
• Multiple return types (string, number, object, null, undefined, boolean, array) ✅
• Function calls ✅
• Returning this ✅
• TypeScript with type annotations ✅

Additional Test Suggestions (Optional)

Consider adding edge cases for:

1. Async constructors (should still catch returns):

   class C { async constructor() { return 1 } }  // Should error

2. Generator constructors (if supported):

   class C { *constructor() { return 1 } }  // Should error

3. Class expressions:

   const C = class { constructor() { return 1 } }  // Should error

However, the current test suite is already comprehensive and covers the primary use cases from the ESLint rule.

---

Alignment with Repository Standards

✅ Follows Conventions

1. File Organization: Matches pattern of other rules (e.g., no_const_assign, no_constant_condition)
2. Test Structure: Uses rule_tester.RunRuleTester as documented in docs/RULE_TESTING_GUIDE.md
3. Registration: Properly added to internal/config/config.go
4. Naming: Follows kebab-case for rule names, PascalCase for Go identifiers
5. Dictionary Update: Added kumiko to spell checker dictionary ✅

---

Comparison with ESLint Source

I verified against the ESLint implementation:

✅ Accurate Port

• Correctly identifies constructors ✅
• Allows bare return statements ✅
• Ignores nested functions ✅
• Checks for return expression presence ✅

Differences (By Design)

The Go implementation uses AST node traversal instead of ESLint's scope analysis, which is appropriate for the RSLint architecture.

---

Final Recommendations

Required Actions

None - The PR is ready to merge as-is.

Suggested Improvements (Optional)

1. Add package-level documentation
2. Add function-level comments for exported functions
3. Consider the additional test cases mentioned above

Commits

• ✅ Good commit messages
• ✅ Includes proper attribution
• ✅ Spell check fix in separate commit

---

Summary

This is a well-crafted PR that successfully ports the ESLint no-constructor-return rule to RSLint. The implementation is:

• ✅ Functionally correct
• ✅ Well-tested
• ✅ Following repository conventions
• ✅ Properly documented in the PR description
• ✅ Ready for merge

Great work! 🎉

---

Reviewed by: Claude Code
Review Date: 2025-11-01