Enable test for ban-tslint-comment rule

[delino]

Summary

• Uncommented the test file for the ban-tslint-comment rule in rstest.config.mts to enable testing
• Implemented the ban-tslint-comment rule in Go
• Registered the rule in the configuration

Context

This PR is a follow-up to #149 which enabled the ban-ts-comment rule. This PR enables the ban-tslint-comment rule which helps enforce migration from deprecated TSLint to ESLint.

Changes

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

   • Uncommented the line enabling the ban-tslint-comment test file

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

   • Created new Go implementation for the ban-tslint-comment rule
   • Detects tslint:disable and tslint:enable comments in both single-line (//) and multi-line (/* */) formats
   • Provides automatic fixes that remove tslint comments
   • Handles various comment positions (standalone lines and inline comments)
   • Reports errors with messageId 'commentDetected' and appropriate description

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

   • Added import for the ban_tslint_comment package
   • Registered the rule as @typescript-eslint/ban-tslint-comment

Implementation Details

The rule implementation:

• Scans source text for tslint comment patterns
• Matches comments starting with tslint:disable or tslint:enable
• Creates fixes that intelligently remove comments:
  • For standalone comment lines: removes the entire line including newline
  • For inline comments (e.g., someCode(); // tslint:disable-line): removes just the comment
• Follows the pattern established by the ban-ts-comment rule

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 ban-tslint-comment rule:
https://typescript-eslint.io/rules/ban-tslint-comment/

TSLint was deprecated in favor of ESLint, and this rule helps enforce the migration by detecting and removing TSLint directive comments.

🤖 Generated with Claude Code

[kdy1]

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

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 ban-tslint-comment rule and ensure it passes.

Objective

Uncomment the test file './tests/typescript-eslint/rules/ban-tslint-comment.test.ts' in /home/runner/work/rslint/rslint/packages/rslint-test-tools/rstest.config.mts (line 36) and ensure all tests pass.

Documentation & Resources

• TypeScript-ESLint ban-tslint-comment rule: https://typescript-eslint.io/rules/ban-tslint-comment/
• TypeScript-ESLint rules overview: https://typescript-eslint.io/rules/
• TSLint deprecation context: Roadmap: TSLint -> ESLint palantir/tslint#4534
• Go AST visitor pattern guide: https://disaev.me/p/writing-useful-go-analysis-linter/
• Go analysis framework: https://pkg.go.dev/golang.org/x/tools/go/analysis

Implementation Steps

1. Locate the test file in rstest.config.mts (line 36)
2. Uncomment the line: './tests/typescript-eslint/rules/ban-tslint-comment.test.ts'
3. Run the tests to verify current state
4. If tests fail, examine or create the Go implementation at /home/runner/work/rslint/rslint/internal/plugins/typescript/rules/ban_tslint_comment/
5. Fix implementation bugs or implement missing functionality

Critical Requirement

If CI fails with test errors, you MUST implement the ban-tslint-comment rule from scratch:

• Study the TypeScript-ESLint reference at https://typescript-eslint.io/rules/ban-tslint-comment/
• Detect and report tslint:disable and tslint:enable comments
• The rule helps enforce migration from deprecated TSLint to ESLint
• Handle both single-line and multi-line comment formats
• Pattern match: // tslint:disable, // tslint:enable, /* tslint:disable */, etc.
• Use Go's AST comment traversal
• Follow existing code patterns in the rslint codebase

Success Criteria

• Test file is uncommented in rstest.config.mts
• All tests for ban-tslint-comment pass
• CI pipeline succeeds
• Implementation matches TypeScript-ESLint behavior

---

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

[claude]

Pull Request Review: ban-tslint-comment Rule Implementation

Thanks for implementing the ban-tslint-comment rule! This is a well-structured PR that follows the established patterns in the codebase. Below is my detailed review covering code quality, potential issues, performance, security, and test coverage.

---

✅ Strengths

1. Follows Established Patterns: The implementation closely mirrors the ban-ts-comment rule structure, which ensures consistency across the codebase.

2. Good Infrastructure Addition: The addition of ReportRangeWithFixes method to the RuleContext is a valuable enhancement that can be reused by other rules.

3. Autofix Support: Properly implements autofixes for removing tslint comments, which improves developer experience.

4. Comprehensive Test Coverage: The test file includes both valid and invalid cases with expected autofix outputs.

---

🐛 Potential Bugs and Issues

1. Critical: Incomplete Comment Pattern Matching

Location: internal/plugins/typescript/rules/ban_tslint_comment/ban_tslint_comment.go:14-17

The regex patterns only match tslint:disable and tslint:enable, but the tests expect detection of additional TSLint directive patterns:

• tslint:disable-line
• tslint:disable-next-line
• tslint:disable:rule1 rule2 (with specific rule names)

Current regex:

singleLineTslintRegex = regexp.MustCompile(`^//\s*tslint:(disable|enable)`)
multiLineTslintRegex = regexp.MustCompile(`^/\*\s*tslint:(disable|enable)`)

Should be:

singleLineTslintRegex = regexp.MustCompile(`^//\s*tslint:(disable|enable)`)
multiLineTslintRegex = regexp.MustCompile(`^/\*\s*tslint:(disable|enable)`)

These patterns will fail to match:

• // tslint:disable-line (test line 98)
• // tslint:disable-next-line (test lines 84, 111)

Recommendation: Update regex patterns to match all TSLint directive variants:

singleLineTslintRegex = regexp.MustCompile(`^//\s*tslint:(disable|enable)`)
multiLineTslintRegex = regexp.MustCompile(`^/\*\s*tslint:(disable|enable)`)

2. String Detection in Code

Location: ban_tslint_comment.go:36-86 (processComments function)

The current implementation scans raw text without considering:

• String literals that may contain // or /*
• RegExp literals that may contain comment-like patterns
• Template literals with comment-like content

Example false positives:

const example = "// tslint:disable"; // Would incorrectly be flagged
const regex = /\/\/ tslint:enable/; // Would incorrectly be flagged

The ban-ts-comment rule has the same limitation, so this is consistent with existing code, but it's worth documenting as a known limitation.

3. Unused Variables

Location: ban_tslint_comment.go:128-129

_ = line
_ = column

The line and column variables are calculated but not used. Either remove the calculation or use them in the error reporting if the framework supports it.

4. Unused Function

Location: ban_tslint_comment.go:199-201

func trimTrailingWhitespace(s string) string {
	return strings.TrimRight(s, " \t\r\n")
}

This function is defined but never called. Should be removed.

---

⚡ Performance Considerations

1. Single-Pass Text Scanning: ✅ Good

The implementation scans the source text once, which is efficient.

2. Regex Compilation: ✅ Good

Regular expressions are compiled once as package-level variables, avoiding repeated compilation overhead.

3. Line Start Calculation: ⚠️ Minor Concern

The calculateLineStarts() function is called but the result is only used for calculating line/column numbers that are then discarded. Consider removing this overhead:

// Remove this calculation if line/column aren't needed
lineStarts := calculateLineStarts(text)

---

🔒 Security Considerations

1. No Regular Expression DoS (ReDoS): ✅ Good

The regex patterns are simple and don't contain nested quantifiers or backtracking issues.

2. No Arbitrary Code Execution: ✅ Good

The implementation only reads and reports on source code, with no code execution or file system modifications beyond the autofix range.

3. Safe Text Range Handling: ✅ Good

Boundary checks are performed before accessing text positions.

---

🧪 Test Coverage

1. Test File Enabled: ✅ Good

The PR properly enables the test file in rstest.config.mts.

2. Test Case Coverage: ⚠️ Incomplete

The test file includes diverse cases, but will fail due to the regex issue mentioned above:

• ❌ Test at line 84-95: // tslint:disable-next-line won't match
• ❌ Test at line 97-109: // tslint:disable-line won't match
• ❌ Test at line 111-121: // tslint:disable-next-line:rule1... won't match
• ❌ Test at line 123-142: // tslint:disable-line on standalone line won't match

3. Missing Test Cases:

• Inline multi-line comments: someCode(); /* tslint:disable */ moreCode();
• Triple-slash comments: /// tslint:disable
• Comments with extra whitespace: // tslint:disable (multiple spaces)
• Case sensitivity: // TSLINT:DISABLE or // TsLint:disable

---

📝 Code Quality Suggestions

1. Add Package Documentation

// Package ban_tslint_comment implements the @typescript-eslint/ban-tslint-comment rule.
// This rule bans // tslint:<rule-flag> comments to enforce migration from deprecated TSLint.
package ban_tslint_comment

2. Add Function Documentation

Key functions like reportTslintComment, createFix, and processComments should have doc comments explaining their purpose and behavior.

3. Simplify Fix Logic

The createFix function has complex nested conditions. Consider extracting helper functions:

func shouldRemoveEntireLine(hasContentBefore, hasContentAfter bool) bool {
    return !hasContentBefore && !hasContentAfter
}

4. Error Message Consistency

The error message "tslint is deprecated and you should stop using it" could be more actionable:

Description: "TSLint is deprecated in favor of ESLint. Remove this directive comment."

---

🎯 Action Items

Must Fix Before Merge:

1. ✅ Fix regex patterns to match all TSLint directive types (disable-line, disable-next-line, etc.)
2. ✅ Remove unused code (trimTrailingWhitespace, unused line/column variables)
3. ✅ Verify tests pass with the corrected regex

Nice to Have:
4. Consider adding string literal detection to avoid false positives
5. Add package and function documentation
6. Consider additional test cases for edge cases
7. Improve error message clarity

---

📊 Overall Assessment

This is a solid implementation that follows the repository patterns well. The main issue is the incomplete regex pattern that will cause test failures. Once the regex is fixed to match all TSLint directive variants, this will be ready to merge.

The infrastructure addition (ReportRangeWithFixes) is valuable and well-implemented. The autofix logic is thorough and handles various comment positions correctly.

Recommendation: Request changes to fix the regex pattern, then approve once tests pass.

---

🤖 Review generated with Claude Code