fix(nolint): skip unopened NOLINTEND blocks that specify a category#423
fix(nolint): skip unopened NOLINTEND blocks that specify a category#423aaronliu0130 merged 2 commits intodevelopfrom
Conversation
these are often for external tools like clang-tidy instead
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe pull request updates cpplint's NOLINT END handling: END directives that include an external-tool category (e.g., Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cpplint_unittest.py (1)
776-793: Add a direct unopenedNOLINTEND(category)regression case.This currently validates the behavior indirectly through
NOLINTBEGIN(...)+NOLINTEND(...). The PR objective is specifically unopenedNOLINTEND(category), so a standalone case would better lock the contract.Proposed test addition
assert ( error_collector.Results() == "Use int16_t/int64_t/etc, rather than the C type long [runtime/int] [4]" ) + # unopened NOLINTEND(category) for external tools should be ignored + error_collector = ErrorCollector(self.assertTrue) + cpplint.ProcessFileData( + "test.cc", + "cc", + [ + "// Copyright 2026 Space Dot", + "// NOLINTEND(clang-analyzer-core.uninitialized.UndefReturn)", + "long d = 1;", + "", + ], + error_collector, + ) + assert ( + error_collector.Results() + == "Use int16_t/int64_t/etc, rather than the C type long [runtime/int] [4]" + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpplint_unittest.py` around lines 776 - 793, Add a new unit test case in cpplint_unittest.py that directly exercises an unopened NOLINTEND(category): create an ErrorCollector (ErrorCollector(self.assertTrue)) and call cpplint.ProcessFileData with a file content list that contains a standalone "// NOLINTEND(clang-analyzer-core.uninitialized.UndefReturn)" (no matching NOLINTBEGIN) plus a line that would normally trigger the lint (e.g., "long c = 418;"), then assert the expected lint message is still emitted (using error_collector.Results()). Place the test alongside the existing tests that call cpplint.ProcessFileData so it uses the same helpers (ErrorCollector, cpplint.ProcessFileData) and verifies behavior for unopened NOLINTEND(category).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cpplint.py`:
- Around line 1159-1164: The current skip in the NOLINTEND handling is too
broad; instead of skipping when matched.group(2) is any non-empty category, only
skip when the category explicitly identifies an external tool (e.g.,
clang/clang-tidy families). Update the condition around
_error_suppressions.HasOpenBlock() and matched.group(2) to test the category for
external-tool identifiers (for example, check matched.group(2) startswith
"clang-" or matches "clang-tidy"/"clang_analyzer" etc.) and only suppress the
readability/nolint error() when that external-tool check passes; otherwise call
error(filename, linenum, "readability/nolint", 5, "Not in a NOLINT block") as
before.
---
Nitpick comments:
In `@cpplint_unittest.py`:
- Around line 776-793: Add a new unit test case in cpplint_unittest.py that
directly exercises an unopened NOLINTEND(category): create an ErrorCollector
(ErrorCollector(self.assertTrue)) and call cpplint.ProcessFileData with a file
content list that contains a standalone "//
NOLINTEND(clang-analyzer-core.uninitialized.UndefReturn)" (no matching
NOLINTBEGIN) plus a line that would normally trigger the lint (e.g., "long c =
418;"), then assert the expected lint message is still emitted (using
error_collector.Results()). Place the test alongside the existing tests that
call cpplint.ProcessFileData so it uses the same helpers (ErrorCollector,
cpplint.ProcessFileData) and verifies behavior for unopened NOLINTEND(category).
cpplint.py
Outdated
| if not _error_suppressions.HasOpenBlock(): | ||
| error(filename, linenum, "readability/nolint", 5, "Not in a NOLINT block") | ||
| if matched.group(2) not in (None, "(*)"): | ||
| # Has category; probably a clang-tidy rule. Safer to ignore. | ||
| pass | ||
| else: | ||
| error(filename, linenum, "readability/nolint", 5, "Not in a NOLINT block") |
There was a problem hiding this comment.
Restrict this skip path to external-tool categories only.
The new check (matched.group(2) not in (None, "(*)")) skips the block-mismatch error for all categorized NOLINTEND(...) when no block is open. That can hide malformed cpplint/legacy directives (not just clang-* external ones), which is broader than the PR objective.
🔧 Proposed fix
- if not _error_suppressions.HasOpenBlock():
- if matched.group(2) not in (None, "(*)"):
- # Has category; probably a clang-tidy rule. Safer to ignore.
- pass
- else:
- error(filename, linenum, "readability/nolint", 5, "Not in a NOLINT block")
+ categories = matched.group(2)
+ parsed_categories = (
+ {c.strip() for c in categories[1:-1].split(",")}
+ if categories and categories.startswith("(") and categories.endswith(")")
+ else set()
+ )
+ has_only_other_tool_categories = bool(parsed_categories) and all(
+ any(category.startswith(prefix) for prefix in _OTHER_NOLINT_CATEGORY_PREFIXES)
+ for category in parsed_categories
+ )
+
+ if not _error_suppressions.HasOpenBlock() and not has_only_other_tool_categories:
+ error(filename, linenum, "readability/nolint", 5, "Not in a NOLINT block")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if not _error_suppressions.HasOpenBlock(): | |
| error(filename, linenum, "readability/nolint", 5, "Not in a NOLINT block") | |
| if matched.group(2) not in (None, "(*)"): | |
| # Has category; probably a clang-tidy rule. Safer to ignore. | |
| pass | |
| else: | |
| error(filename, linenum, "readability/nolint", 5, "Not in a NOLINT block") | |
| categories = matched.group(2) | |
| parsed_categories = ( | |
| {c.strip() for c in categories[1:-1].split(",")} | |
| if categories and categories.startswith("(") and categories.endswith(")") | |
| else set() | |
| ) | |
| has_only_other_tool_categories = bool(parsed_categories) and all( | |
| any(category.startswith(prefix) for prefix in _OTHER_NOLINT_CATEGORY_PREFIXES) | |
| for category in parsed_categories | |
| ) | |
| if not _error_suppressions.HasOpenBlock() and not has_only_other_tool_categories: | |
| error(filename, linenum, "readability/nolint", 5, "Not in a NOLINT block") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpplint.py` around lines 1159 - 1164, The current skip in the NOLINTEND
handling is too broad; instead of skipping when matched.group(2) is any
non-empty category, only skip when the category explicitly identifies an
external tool (e.g., clang/clang-tidy families). Update the condition around
_error_suppressions.HasOpenBlock() and matched.group(2) to test the category for
external-tool identifiers (for example, check matched.group(2) startswith
"clang-" or matches "clang-tidy"/"clang_analyzer" etc.) and only suppress the
readability/nolint error() when that external-tool check passes; otherwise call
error(filename, linenum, "readability/nolint", 5, "Not in a NOLINT block") as
before.
There was a problem hiding this comment.
I find this too complicated/less performant, and believe false positives would be niche.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
That's the exact concern I have. It's too uncommon a scenario/of too limited utility to use that much performance for.
I'm willing to see what others think about this.
There was a problem hiding this comment.
@aaronliu0130 Understood. Let’s keep the simpler, fast path.
To make the intent explicit (at zero runtime cost) and guard against regressions, I suggest:
- Add a short comment where END is handled to note we intentionally ignore unmatched NOLINTEND(category) because many such categories are for external tools (e.g., clang-tidy/analyzer).
- Add two small tests:
- Unopened NOLINTEND(clang-analyzer-...) does not emit readability/nolint.
- Unopened NOLINTEND(readability/...) still surfaces an error (currently “NOLINT categories not supported in block END: …”).
Example comment you could drop in:
Has category; many are external-tool directives (e.g., clang-tidy). Intentionally ignore unmatched END to avoid false positives (see #298).
If you’d like, I can add the tests in this PR.
seems to be slightly more performant due to branch prediction (former is more likely)
aaronliu0130
temporarily deployed
to
testpypi
GitHub Actions
Inactive
cclauss
left a comment
cclauss
left a comment
There was a problem hiding this comment.
I do not love else: pass but I can live with it.
2 participants
these are often for external tools like clang-tidy instead
Should fix #298
Summary by CodeRabbit
Bug Fixes
Tests