Skip to content

JS: Promote js/regex/duplicate-in-character-class to quality #19711

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

Merged
merged 6 commits into from
Jun 11, 2025

Conversation

Napalys
Copy link
Contributor

@Napalys Napalys commented Jun 10, 2025

The following pull request aims to promote js/regex/duplicate-in-character-class to quality-suite.

Copy link
Contributor

github-actions bot commented Jun 10, 2025

QHelp previews:

javascript/ql/src/RegExp/DuplicateCharacterInCharacterClass.qhelp

Duplicate character in character class

Character classes in regular expressions (denoted by square brackets []) represent sets of characters where the pattern matches any single character from that set. Since character classes are sets, specifying the same character multiple times is redundant and often indicates a programming error.

Common mistakes include:

  • Using square brackets [] instead of parentheses () for grouping alternatives
  • Misunderstanding that special regex characters like |, *, +, (), and - work differently when appearing inside a character class
  • Accidentally duplicating characters or escape sequences that represent the same character

Recommendation

Examine each duplicate character to determine the intended behavior:

  • If you see | inside square brackets (e.g., [a|b|c]): This is usually a mistake. The author likely intended alternation. Replace the character class with a group: (a|b|c)
  • If trying to match alternative strings, use parentheses () for grouping instead of square brackets
  • If the duplicate was truly accidental, remove the redundant characters
  • If trying to use special regex operators inside square brackets, note that most operators (like |) are treated as literal characters
    Note that simply removing | characters from character classes is rarely the correct fix. Instead, analyze the pattern to understand what the author intended to match.

Example

Example 1: Confusing character classes with groups

The pattern [password|pwd] does not match "password" or "pwd" as intended. Instead, it matches any single character from the set {p, a, s, w, o, r, d, |}. Note that | has no special meaning inside character classes.

if (/[password|pwd] =/.test(input))
	console.log("Found password!");

To fix this problem, the regular expression should be rewritten to /(password|pwd) =/.

Example 2: CSS unit matching

The pattern r?e[m|x] appears to be trying to match "rem" or "rex", but actually matches "re" followed by any of the characters {m, |, x}. The correct pattern should be r?e(m|x) or r?e[mx].

Similarly, v[h|w|min|max] should be v(h|w|min|max) to properly match "vh", "vw", "vmin", or "vmax".

References

@Napalys Napalys force-pushed the js/quality/promote_duplicate_char_class branch from 84cc0c3 to d68f5eb Compare June 10, 2025 10:10
@Napalys Napalys marked this pull request as ready for review June 10, 2025 10:43
@Copilot Copilot AI review requested due to automatic review settings June 10, 2025 10:43
@Napalys Napalys requested a review from a team as a code owner June 10, 2025 10:43
@Napalys Napalys added the no-change-note-required This PR does not need a change note label Jun 10, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Promote the js/regex/duplicate-in-character-class query to the quality suite by adjusting metadata, expanding tests, and enhancing documentation.

  • Update query tags from reliability to quality and include correctness & regular-expressions
  • Add new regex patterns to tst.js and dynamic replacement tests in tst_replace.js, plus expected-alert entries
  • Enhance .qhelp with a clearer overview, recommendations, examples, and references; include the rule in the quality-suite integration tests

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
javascript/ql/test/query-tests/RegExp/DuplicateCharacterInCharacterClass/tst_replace.js Added dynamic‐pattern replacement test functions and new test cases
javascript/ql/test/query-tests/RegExp/DuplicateCharacterInCharacterClass/tst.js Added a variety of new regex literals to trigger duplicate alerts
javascript/ql/src/RegExp/DuplicateCharacterInCharacterClass.ql Changed @tags from reliability to quality suite and added tags
javascript/ql/src/RegExp/DuplicateCharacterInCharacterClass.qhelp Expanded overview, recommendations, examples, and reference links
javascript/ql/integration-tests/query-suite/javascript-code-quality.qls.expected Added the duplicate-character-class rule to the code-quality suite
Comments suppressed due to low confidence (2)

javascript/ql/test/query-tests/RegExp/DuplicateCharacterInCharacterClass/tst_replace.js:1

  • [nitpick] Function names reg, reg1, reg2, reg3 are ambiguous; consider using more descriptive test function names to clarify their purpose.
function reg(){

javascript/ql/src/RegExp/DuplicateCharacterInCharacterClass.ql:9

  • [nitpick] The reliability tag is on a separate line without an @tags prefix; consolidate tags into a single @tags quality, reliability, correctness, regular-expressions annotation for consistency.
*       reliability

@Napalys Napalys requested a review from asgerf June 10, 2025 11:57
@Napalys Napalys merged commit 6811cad into github:main Jun 11, 2025
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation JS no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants