Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: no-unlocalized-strings rule to correctly handle as const assertions in property values with ignoreNames #92

Merged
merged 2 commits into from
Dec 5, 2024

Conversation

swernerx
Copy link
Contributor

@swernerx swernerx commented Dec 4, 2024

Description:

This PR fixes an issue in the no-unlocalized-strings ESLint rule where string literals assigned to object properties are incorrectly reported as unlocalized strings when using TypeScript's as const assertions, even if the property names are specified in the ignoreNames option.

Background:

When using the ignoreNames option with a regex pattern to ignore certain property names, the rule should not report string literals assigned to those properties. However, if the property value includes an as const assertion, the rule fails to recognize it due to the TSAsExpression node introduced by the TypeScript assertion.

Example:

Given the ESLint configuration:

{
  "rules": {
    "no-unlocalized-strings": [
      "error",
      { "ignoreNames": [{ "regex": { "pattern": "^[A-Z0-9_-]+$" } }] }
    ]
  }
}

The following code incorrectly reports errors for the string literals "circle" and "rectangle":

export const Shape = {
  CIRCLE: "circle" as const,
  RECTANGLE: "rectangle" as const,
};

Cause of the Issue:

The as const assertion wraps the string literal in a TSAsExpression node in the Abstract Syntax Tree (AST). The existing rule does not handle TSAsExpression nodes when processing property values, so it fails to apply the ignoreNames logic to the unwrapped string literals.

Solution:

The fix involves updating the rule to handle TSAsExpression nodes by unwrapping them to access the underlying string literals. Specifically:

  1. Implemented an Unwrapping Helper Function:

    Added a helper function unwrapTSAsExpression to recursively unwrap TSAsExpression nodes:

    function unwrapTSAsExpression(
      node: TSESTree.Expression,
    ): TSESTree.Literal | TSESTree.TemplateLiteral | TSESTree.Expression {
      while (node.type === TSESTree.AST_NODE_TYPES.TSAsExpression) {
        node = node.expression;
      }
      return node;
    }
  2. Updated the isStringLiteral Function:

    Modified isStringLiteral to handle TSAsExpression nodes:

    function isStringLiteral(node: TSESTree.Node | null | undefined): boolean {
      if (!node) return false;
    
      if (node.type === TSESTree.AST_NODE_TYPES.TSAsExpression) {
        return isStringLiteral(node.expression);
      }
    
      switch (node.type) {
        case TSESTree.AST_NODE_TYPES.Literal:
          return typeof node.value === 'string';
        case TSESTree.AST_NODE_TYPES.TemplateLiteral:
          return true;
        case TSESTree.AST_NODE_TYPES.JSXText:
          return true;
        default:
          return false;
      }
    }
  3. Modified the Visitor to Handle TSAsExpression Nodes:

    Updated the visitor function for Property nodes to unwrap TSAsExpression nodes when processing property values:

    'Property > :matches(Literal, TemplateLiteral, TSAsExpression)'(
      node: TSESTree.Literal | TSESTree.TemplateLiteral | TSESTree.TSAsExpression,
    ) {
      const parent = node.parent as TSESTree.Property;
    
      if (
        (isIdentifier(parent.key) && isIgnoredName(parent.key.name)) ||
        ((isLiteral(parent.key) || isTemplateLiteral(parent.key)) &&
          isIgnoredName(getText(parent.key)))
      ) {
        const unwrappedNode = unwrapTSAsExpression(node);
    
        if (isLiteral(unwrappedNode) || isTemplateLiteral(unwrappedNode)) {
          visited.add(unwrappedNode);
        }
      }
    },

Testing:

  • Manual Testing:
    • Verified that with the ignoreNames option configured, the linter no longer reports errors for string literals assigned to property names matching the regex pattern, even when using as const assertions.
  • Automated Testing:
    • Ran the existing test suite to ensure no regressions were introduced.
    • Added new test cases to cover scenarios involving as const assertions in property values.

Notes:

  • Minimal Changes: The fix is minimal and specifically targets the handling of TSAsExpression nodes in property values, avoiding changes that could affect other parts of the rule.
  • TypeScript Compatibility: The solution ensures compatibility with TypeScript features, making the rule more robust when analyzing TypeScript code.
  • No Regressions: Care was taken to ensure that existing functionality remains unaffected, and that the fix does not introduce any new issues.

Conclusion:

This PR improves the no-unlocalized-strings rule by correctly handling as const assertions in property values when using the ignoreNames option. It ensures that string literals assigned to ignored property names are not incorrectly reported as unlocalized strings.

@timofei-iatsenko
Copy link
Collaborator

LGTM, thanks for the contribution!

Copy link

codecov bot commented Dec 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.60%. Comparing base (68b9052) to head (9159f53).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #92      +/-   ##
==========================================
+ Coverage   96.53%   96.60%   +0.07%     
==========================================
  Files          10       10              
  Lines         375      383       +8     
  Branches      106      109       +3     
==========================================
+ Hits          362      370       +8     
  Misses         13       13              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@andrii-bodnar
Copy link
Contributor

@swernerx thanks for the contribution! Could you please rebase the current branch and resolve conflicts?

@swernerx swernerx force-pushed the fix/ts-as-const-ignore-names branch from 4120045 to 9159f53 Compare December 5, 2024 09:57
@swernerx
Copy link
Contributor Author

swernerx commented Dec 5, 2024

Rebase done!

@andrii-bodnar andrii-bodnar merged commit 4048c4d into lingui:main Dec 5, 2024
4 checks passed
@andrii-bodnar andrii-bodnar linked an issue Dec 5, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ignoreNames doesn't work strings that are typed with "as const"
3 participants