-
Notifications
You must be signed in to change notification settings - Fork 1
Add polymath-ros linter which adds a check for executor threads #10
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
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
57ea9ec
add linter rule to cpp to enforce always specifying executor threads
9087dd1
move into separate ROS linter
6a24335
add polymath-ros linter to precommit config and test_runner
682a330
bump version to 2.2.0
daedc13
clean up the whitespace functions
7bfa659
cleanup claude commentary
9b34e81
allow the user to pass 0 threads (hardware_concurrency)
7a0a7e2
move to ros subdirectory
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| # SPDX-FileCopyrightText: 2026 Polymath Robotics, Inc. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
| import argparse | ||
|
|
||
| from polymath_code_standard.checker import CheckerGroup, Result, check_group | ||
| from polymath_code_standard.checkers.ros.executor_lint import check_executor_threads | ||
|
|
||
|
|
||
| @check_group | ||
| class RosGroup(CheckerGroup): | ||
| name = 'ros' | ||
|
|
||
| def run(self, args: argparse.Namespace) -> list[Result]: | ||
| return [check_executor_threads(args.files)] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,193 @@ | ||
| # SPDX-FileCopyrightText: 2026 Polymath Robotics, Inc. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
| """Require an explicit thread count for multi-threaded rclcpp executors. | ||
|
|
||
| A default-constructed MultiThreadedExecutor / EventsCBGExecutor spawns | ||
| ``hardware_concurrency()`` threads (one per core). | ||
| Their constructors take ``number_of_threads`` as the 2nd | ||
| positional arg, so a bounded count needs ``Type(rclcpp::ExecutorOptions(), N)``; | ||
| This linter forces ROS users to deliberately choose a thread count. | ||
| This includes passing '0' as an option (which defaults to std::hardware_concurrency) | ||
| so that the max threads are created deliberately | ||
| and not from default construction. | ||
|
|
||
| Detection is based on regex + balanced-paren parsing. | ||
| To suppress, use a trailing ``// NOLINT``. | ||
| """ | ||
|
|
||
| import re | ||
| from pathlib import Path | ||
|
|
||
| from polymath_code_standard.checker import Result | ||
|
|
||
| # Executors whose 2nd positional ctor arg is number_of_threads. | ||
| THREADED_EXECUTORS = ('MultiThreadedExecutor', 'EventsCBGExecutor') | ||
|
|
||
| _TYPE_RE = re.compile(r'(?:[A-Za-z_]\w*::)*(' + '|'.join(THREADED_EXECUTORS) + r')\b') | ||
|
|
||
|
|
||
| def _blank(span: str) -> str: | ||
| """Spaces of equal length to ``span``, with newlines left in place.""" | ||
| return ''.join('\n' if c == '\n' else ' ' for c in span) | ||
|
|
||
|
|
||
| def _string_end(text: str, start: int) -> int: | ||
| """Index just past the string/char literal whose opening quote is at ``start``.""" | ||
| quote = text[start] | ||
| i = start + 1 | ||
| while i < len(text): | ||
| if text[i] == '\\': # escape consumes the next char, including a quote | ||
| i += 2 | ||
| elif text[i] == quote: | ||
| return i + 1 | ||
| else: | ||
| i += 1 | ||
| return len(text) | ||
|
|
||
|
|
||
| def _blank_comments_and_strings(text: str) -> str: | ||
| """Blank comments and string/char literals to spaces. The result keeps the | ||
| same length and newline positions as ``text``, so offsets and line numbers | ||
| computed on it map straight back to the original.""" | ||
| out = [] | ||
| i, n = 0, len(text) | ||
| while i < n: | ||
| pair = text[i : i + 2] | ||
| if pair == '//': | ||
| end = text.find('\n', i) | ||
| end = n if end == -1 else end | ||
| elif pair == '/*': | ||
| end = text.find('*/', i + 2) | ||
| end = n if end == -1 else end + 2 | ||
| elif text[i] in '"\'': | ||
| end = _string_end(text, i) | ||
| else: | ||
| out.append(text[i]) | ||
| i += 1 | ||
| continue | ||
| out.append(_blank(text[i:end])) | ||
| i = end | ||
| return ''.join(out) | ||
|
|
||
|
|
||
| def _skip_ws(text: str, i: int) -> int: | ||
| while i < len(text) and text[i].isspace(): | ||
| i += 1 | ||
| return i | ||
|
|
||
|
|
||
| def _parse_arg_list(text: str, open_idx: int) -> list[str] | None: | ||
| """Top-level comma-split of the bracketed list starting at ``open_idx``. | ||
|
|
||
| Depth is tracked uniformly across (), [] and {} so nested calls like | ||
| ``ExecutorOptions()`` count as part of one argument. Returns None if the | ||
| list is unbalanced (parse gave up — caller should not flag).""" | ||
| depth = 0 | ||
| args: list[str] = [] | ||
| buf: list[str] = [] | ||
| for c in text[open_idx:]: | ||
| if c in '([{': | ||
| depth += 1 | ||
| if depth == 1: | ||
| continue | ||
| buf.append(c) | ||
| elif c in ')]}': | ||
| depth -= 1 | ||
| if depth == 0: | ||
| tail = ''.join(buf).strip() | ||
| if args or tail: | ||
| args.append(tail) | ||
| return args | ||
| buf.append(c) | ||
| elif c == ',' and depth == 1: | ||
| args.append(''.join(buf).strip()) | ||
| buf = [] | ||
| else: | ||
| buf.append(c) | ||
| return None | ||
|
|
||
|
|
||
| def find_violations(text: str) -> list[tuple[int, str]]: | ||
| """Return (1-based line, type name) for each unbounded executor construction.""" | ||
| code = _blank_comments_and_strings(text) | ||
| violations = [] | ||
| for m in _TYPE_RE.finditer(code): | ||
| type_name = m.group(1) | ||
|
|
||
| # Skip type aliases — ``typedef X Y;`` / ``using Y = X;`` are not ctors. | ||
| stmt_start = ( | ||
| max( | ||
| code.rfind(';', 0, m.start()), | ||
| code.rfind('{', 0, m.start()), | ||
| code.rfind('}', 0, m.start()), | ||
| ) | ||
| + 1 | ||
| ) | ||
| if re.search(r'\b(?:typedef|using)\b', code[stmt_start : m.start()]): | ||
| continue | ||
|
|
||
| j = _skip_ws(code, m.end()) | ||
| if j >= len(code): | ||
| continue | ||
| ch = code[j] | ||
| args: list[str] | None = None | ||
| default_ctor = False | ||
|
|
||
| if ch in '({': # Type(...) / Type{...} temporary or direct init | ||
| args = _parse_arg_list(code, j) | ||
| elif ch == '_' or ch.isalpha(): # Type varname ... | ||
| k = j | ||
| while k < len(code) and (code[k].isalnum() or code[k] == '_'): | ||
| k += 1 | ||
| k = _skip_ws(code, k) | ||
| if k < len(code) and code[k] in '({': | ||
| args = _parse_arg_list(code, k) | ||
| elif k < len(code) and code[k] in ';,)=': | ||
| default_ctor = True # Type var; (no initializer) | ||
| else: | ||
| continue | ||
| elif ch == '>': # make_shared<...Type>( ... ) | ||
| k = _skip_ws(code, j + 1) | ||
| if k < len(code) and code[k] == '(': | ||
| args = _parse_arg_list(code, k) | ||
| else: | ||
| continue | ||
| else: | ||
| continue # reference / pointer / unrelated token | ||
|
|
||
| if default_ctor: | ||
| bounded = False | ||
| elif args is None: | ||
| continue # unbalanced — don't risk a false positive | ||
| else: | ||
| bounded = len(args) >= 2 and args[1] != '' | ||
| if not default_ctor and bounded: | ||
| continue | ||
|
|
||
| line = code.count('\n', 0, m.start()) + 1 | ||
| violations.append((line, type_name)) | ||
| return violations | ||
|
|
||
|
|
||
| def check_executor_threads(cpp_files: list[str]) -> Result: | ||
| name = 'executor-threads' | ||
| if not cpp_files: | ||
| return Result(name=name, passed=True, skipped=True) | ||
| findings = [] | ||
| for path in cpp_files: | ||
| try: | ||
| text = Path(path).read_text(encoding='utf-8', errors='replace') | ||
| except OSError: | ||
| continue | ||
| src_lines = text.splitlines() | ||
| for lineno, type_name in find_violations(text): | ||
| if 0 <= lineno - 1 < len(src_lines) and 'NOLINT' in src_lines[lineno - 1]: | ||
| continue | ||
| findings.append( | ||
| f'{path}:{lineno}: {type_name} constructed without an explicit thread count; ' | ||
| f'pass number_of_threads, e.g. {type_name}(rclcpp::ExecutorOptions(), N) ' | ||
| f'(use // NOLINT to suppress) [executor-threads]' | ||
| ) | ||
| if findings: | ||
| return Result(name=name, passed=False, output='\n'.join(findings)) | ||
| return Result(name=name, passed=True) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,69 @@ | ||
| # SPDX-FileCopyrightText: 2026 Polymath Robotics, Inc. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
| """Unit tests for the executor thread-count linter.""" | ||
|
|
||
| import pytest | ||
|
|
||
| from polymath_code_standard.checkers.ros.executor_lint import check_executor_threads, find_violations | ||
|
|
||
| # Constructions that MUST be flagged (no explicit, non-zero thread count). | ||
| FLAGGED = [ | ||
| 'rclcpp::executors::MultiThreadedExecutor executor;', | ||
| 'rclcpp::executors::EventsCBGExecutor exec;', | ||
| 'MultiThreadedExecutor executor;', | ||
| 'auto e = rclcpp::executors::MultiThreadedExecutor();', | ||
| 'rclcpp::executors::MultiThreadedExecutor executor(rclcpp::ExecutorOptions());', | ||
| 'auto e = std::make_shared<rclcpp::executors::MultiThreadedExecutor>();', | ||
| 'auto e = new rclcpp::executors::EventsCBGExecutor();', | ||
| 'rclcpp::executors::MultiThreadedExecutor executor{};', | ||
| ] | ||
|
|
||
| # Constructions and references that MUST NOT be flagged. | ||
| CLEAN = [ | ||
| 'rclcpp::executors::MultiThreadedExecutor executor(rclcpp::ExecutorOptions(), 4);', | ||
| 'MultiThreadedExecutor executor(rclcpp::ExecutorOptions(), 0);', | ||
| 'rclcpp::executors::MultiThreadedExecutor executor(rclcpp::ExecutorOptions(), node->getNumThreads());', | ||
| 'auto e = std::make_shared<rclcpp::executors::EventsCBGExecutor>(rclcpp::ExecutorOptions(), 2);', | ||
| 'rclcpp::executors::SingleThreadedExecutor executor;', | ||
| 'void f(rclcpp::executors::MultiThreadedExecutor & exec);', | ||
| 'using Exec = rclcpp::executors::MultiThreadedExecutor;', | ||
| 'typedef rclcpp::executors::MultiThreadedExecutor Exec;', | ||
| '// rclcpp::executors::MultiThreadedExecutor executor;', | ||
| 'const char * s = "MultiThreadedExecutor executor;";', | ||
| 'rclcpp::executors::MultiThreadedExecutor executor; // NOLINT(custom)', | ||
| ] | ||
|
|
||
|
|
||
| @pytest.mark.parametrize('src', FLAGGED) | ||
| def test_flagged(src): | ||
| assert find_violations(src), f'expected a violation for: {src}' | ||
|
|
||
|
|
||
| @pytest.mark.parametrize('src', CLEAN) | ||
| def test_clean(src): | ||
| # NOLINT suppression is applied at the file level, so route through check_*. | ||
| assert not find_violations(src) or 'NOLINT' in src, f'unexpected violation for: {src}' | ||
|
|
||
|
|
||
| def test_nolint_suppresses(tmp_path): | ||
| f = tmp_path / 'a.cpp' | ||
| f.write_text('rclcpp::executors::MultiThreadedExecutor executor; // NOLINT\n') | ||
| assert check_executor_threads([str(f)]).passed | ||
|
|
||
|
|
||
| def test_check_reports_path_and_line(tmp_path): | ||
| f = tmp_path / 'b.cpp' | ||
| f.write_text('int main() {\n rclcpp::executors::MultiThreadedExecutor exec;\n}\n') | ||
| result = check_executor_threads([str(f)]) | ||
| assert not result.passed | ||
| assert f'{f}:2:' in result.output | ||
|
|
||
|
|
||
| def test_multiline_ctor_with_threads_is_clean(tmp_path): | ||
| f = tmp_path / 'c.cpp' | ||
| f.write_text('rclcpp::executors::MultiThreadedExecutor exec(\n rclcpp::ExecutorOptions(),\n 4);\n') | ||
| assert check_executor_threads([str(f)]).passed | ||
|
|
||
|
|
||
| def test_empty_file_list_skipped(): | ||
| assert check_executor_threads([]).skipped |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.