Skip to content

[lint] add operator-spacing checker to detect mixed spacing for '<' and '> - #19919

Closed
sarahgh8 wants to merge 1 commit into
apache:mainfrom
sarahgh8:fix-spacing
Closed

[lint] add operator-spacing checker to detect mixed spacing for '<' and '>#19919
sarahgh8 wants to merge 1 commit into
apache:mainfrom
sarahgh8:fix-spacing

Conversation

@sarahgh8

@sarahgh8 sarahgh8 commented Jul 1, 2026

Copy link
Copy Markdown

This adds a conservative lint script (tests/lint/check_operator_spacing.py) and unit tests (tests/lint/test_check_operator_spacing.py) that detect mixed-side spacing for single-character '<' and '>' comparisons (e.g., Or(x<lower, x> upper)). The checker uses heuristics to avoid templates, includes, static_cast, and stream/shift operators.

Closes issue #18017

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a conservative operator-spacing checker (check_operator_spacing.py) and its corresponding unit tests to flag inconsistent spacing around < and > operators in C++ files. The review feedback highlights a critical bug in the template-closing heuristic that causes it to skip valid comparison violations, making some tests fail. Additionally, the feedback suggests defensive checks for deleted or renamed files, stripping comments and string literals to prevent false positives, and expanding the skipped keywords to include other standard C++ casts.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +97 to +106
def looks_like_template_close(line: str, match: re.Match) -> bool:
# heuristics: if left char is alnum and the token after the op is an identifier start,
# it's probably "vector<int> v" or similar. Check a few characters after op.
left = match.group("left_char")
start = match.start("op")
after = line[start + len(match.group("op")) :]
if left and ALNUM_UNDERSCORE.match(left):
if after and (ALNUM_UNDERSCORE.match(after[0]) or (after[0].isspace() and len(after) > 1 and ALNUM_UNDERSCORE.match(after[1]))):
return True
return False

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The current implementation of looks_like_template_close has a critical bug: it returns True for any comparison between two identifiers with mixed spacing (e.g., x< lower or x> upper), causing the checker to skip them entirely. This makes the newly added tests test_disallow_mixed_right_space and test_composite_expression_matches_issue fail.

To fix this, we should:

  1. Only apply template-close heuristics when the operator is > (since < is a template open, and any valid template open like vector<int> is already skipped by the spacing check if it has no spaces).
  2. Search backwards for a matching < and verify if the preceding word is a likely template name (e.g., starts with an uppercase letter or is a known container like vector, shared_ptr, etc.).
def looks_like_template_close(line: str, match: re.Match) -> bool:
    if match.group("sym") != ">":
        return False
    known_templates = {
        "vector", "shared_ptr", "unique_ptr", "map", "set", "unordered_map", "unordered_set",
        "pair", "tuple", "list", "array", "function", "weak_ptr", "make_shared", "make_unique"
    }
    start = match.start("op")
    left_text = line[:start]
    lt_idx = left_text.rfind("<")
    if lt_idx == -1:
        return False
    idx = lt_idx - 1
    while idx >= 0 and (left_text[idx].isalnum() or left_text[idx] == "_"):
        idx -= 1
    word = left_text[idx + 1:lt_idx]
    if not word:
        return False
    return word[0].isupper() or word in known_templates or word.endswith("_t")

Comment on lines +108 to +110
def check_file(path: str) -> List[Tuple[int, int, str]]:
violations: List[Tuple[int, int, str]] = []
text = Path(path).read_text(encoding="utf-8", errors="ignore")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

If a file is deleted or renamed in a PR, git ls-files might still return it, leading to a FileNotFoundError when Path(path).read_text() is called. We should add a defensive check to ensure the path is a valid file before reading it.

def check_file(path: str) -> List[Tuple[int, int, str]]:
    violations: List[Tuple[int, int, str]] = []
    p = Path(path)
    if not p.is_file():
        return violations
    text = p.read_text(encoding="utf-8", errors="ignore")

Comment on lines +111 to +117
for lineno, line in enumerate(text.splitlines(), start=1):
if is_preprocessor_line(line):
continue
if contains_shift_or_stream(line):
continue
if contains_skip_keyword(line):
continue

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

To prevent false positives, we should strip single-line comments and string literals from the line before checking for operator spacing. This ensures that mixed spacing inside comments or string literals (e.g., "x <y") is not flagged as a violation.

    for lineno, line in enumerate(text.splitlines(), start=1):
        if is_preprocessor_line(line):
            continue
        if contains_shift_or_stream(line):
            continue
        if contains_skip_keyword(line):
            continue

        # Strip single-line comments
        if "//" in line:
            line = line.split("//", 1)[0]

        # Strip string literals to avoid false positives inside strings
        line = re.sub(r'"[^"]*"', lambda m: " " * len(m.group(0)), line)

Comment on lines +53 to +62
SKIP_KEYWORDS = [
"static_cast",
"std::",
"::",
"template",
"TVM_FFI",
"TVM_FFI_ICHECK",
"TVM_FFI_THROW",
"->",
]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

We should expand SKIP_KEYWORDS to include other standard C++ casts like dynamic_cast, reinterpret_cast, and const_cast to avoid false positives on lines containing them.

SKIP_KEYWORDS = [
    "static_cast",
    "dynamic_cast",
    "reinterpret_cast",
    "const_cast",
    "std::",
    "::",
    "template",
    "TVM_FFI",
    "TVM_FFI_ICHECK",
    "TVM_FFI_THROW",
    "->",
]

@tqchen

tqchen commented Jul 1, 2026

Copy link
Copy Markdown
Member

we prefer default settings instead of cusrom ones

@tqchen tqchen closed this Jul 1, 2026
@sarahgh8
sarahgh8 deleted the fix-spacing branch July 1, 2026 15:12
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.

[CI Problem] lint check has a bug

2 participants