fix: align no-constant-binary-expression with ESLint implementation#553
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This is an excellent and thorough pull request that significantly improves the no-constant-binary-expression rule by aligning it with ESLint's implementation. The refactoring of the core logic is substantial, making it more robust and context-aware, particularly with the introduction of type-checker-based scope analysis to handle built-ins correctly. The addition of comprehensive unit and integration tests is a fantastic step towards ensuring correctness and preventing regressions. I've found one potential issue with how delete expressions are handled, which I've detailed in a specific comment. Overall, this is a very high-quality contribution.
internal/rules/no_constant_binary_expression/no_constant_binary_expression.go
Show resolved
Hide resolved
Rewrite the rule to fix false positives on short-circuit patterns like
`!module || !module[pluginName]` and fully align with ESLint's original
`no-constant-binary-expression` rule behavior.
Key changes:
- Add `inBooleanPosition` parameter to `isConstant` for context-aware checking
- Fix `isStaticBoolean(!expr)` to recursively check operand constancy
- Fix `isAlwaysNew` to only flag built-in constructors via TypeChecker scope analysis
- Fix `hasConstantNullishness` with `nonNullish` parameter and correct `??` recursion
- Restructure main rule logic: `===`/`!==` uses single-side alwaysNew, `==`/`!=` uses bothAlwaysNew
- Add missing AST node handlers: typeof, delete, void, class, template, sequence, assignment expressions
- Add scope analysis via `isGlobalBuiltin` with TypeChecker + IsSymbolFromDefaultLibrary
- Fix `getBooleanValue` to not strip quotes (node.Text() already returns parsed content)
- Fix `hasConstantNullishness` to return true for all binary operators including comparisons
- Fix rule definition to use `rule.Rule{}` instead of `rule.CreateRule()` (core ESLint rule, no plugin prefix)
- Add comprehensive Go tests (47 valid + 85 invalid) and JS integration tests (87 cases with snapshots)
5655c8d to
661ffa7
Compare
Summary
Fix false positives in
no-constant-binary-expressionrule (closes #552) and fully align the implementation with ESLint's original rule behavior.Root cause:
isConstant()unconditionally returnedtruefor!expr, causing normal short-circuit patterns like!module || !module[pluginName]to be falsely flagged.Changes:
isConstantwithinBooleanPositionparameter for context-aware checkingisStaticBoolean(!expr)to recursively check operand constancy instead of returningtrueisAlwaysNewto only flag built-in constructors (via TypeChecker scope analysis), not allnew X()hasConstantNullishnesswithnonNullishparameter; correct??recursive logic (only check right side non-nullish); cover all binary operators including comparison operators===/!==uses single-sidealwaysNew,==/!=usesbothAlwaysNewtypeof,delete,void, class expressions, template expressions, sequence/comma expressions, assignment expressions, BigInt literals, spread elements, logical assignmentsisGlobalBuiltin()using TypeChecker +IsSymbolFromDefaultLibrary, with fallback for function declaration merginggetBooleanValueto not strip quotes/backticks (node.Text()already returns parsed content without delimiters)rule.Rule{}instead ofrule.CreateRule()(core ESLint rule should not have@typescript-eslint/prefix)Related Links
no-constant-binary-expressionreports valid short-circuit checks #552Checklist