ddsidl: fix dead Python-keyword check in getAllowedName - #515
Merged
sschleemilch merged 3 commits intoMay 26, 2026
Merged
Conversation
`keyword.iskeyword(name.lower)` was missing its call parentheses, so the bound-method object `name.lower` was passed to `iskeyword` instead of the lowered string. `iskeyword` always returned False, leaving the Python-keyword arm of the check effectively dead — VSS signal names that are Python keywords but NOT also C/IDL keywords (e.g. 'class', 'def', 'import', 'lambda') were emitted unprefixed and produced syntactically invalid IDL. Compute `name.lower()` once into a local, pass it to `iskeyword` explicitly, and add a regression test in tests/test_ddsidl_naming.py covering both the bug case (Python-only keywords) and the existing behaviour (C/IDL keywords, normal names). Signed-off-by: Matt Jones <47545907+SoundMatt@users.noreply.github.com>
SoundMatt
force-pushed
the
fix/ddsidl-keyword-iskeyword-missing-call
branch
from
May 5, 2026 17:19
d3b58ff to
d6ac765
Compare
Collaborator
|
Thanks for the PR. I think it looks good but will leave it open for a while to give others a chance to review/comment if they would like to. |
| def getAllowedName(name): | ||
| if name.lower() in c_keywords or name.lower() in idl_keywords or keyword.iskeyword(name.lower): | ||
| lower = name.lower() | ||
| # `keyword.iskeyword(name.lower)` (without call parens) passed a |
Collaborator
There was a problem hiding this comment.
remove the verbose explanation of the past
Collaborator
|
MoM:
|
Collaborator
|
@SoundMatt - will you take a look at the comments from Sebastian |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Matt Jones <47545907+SoundMatt@users.noreply.github.com>
Contributor
Author
|
Addressed — verbose comments removed. |
…view Signed-off-by: Matt Jones <47545907+SoundMatt@users.noreply.github.com>
Collaborator
|
@sschleemilch - do you want to take second look |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Problem
getAllowedNameinsrc/vss_tools/exporters/ddsidl.pyis meant toprefix reserved-word identifiers (C, IDL, or Python) with a leading
underscore so the generated IDL is valid. The Python branch of the
check was dead:
name.lower(without()) is the bound-method object, not thelowered string.
keyword.iskeyword(<method object>)always returnsFalse, so VSS signal names that are Python keywords but NOT also
C/IDL keywords slipped through silently. Examples that bypass the
check:
class,def,import,lambda,try,with,yield,async,await.The downstream effect is that the IDL exporter emits these names
unprefixed, producing syntactically invalid IDL.
Fix
Extract
name.lower()to a local once and pass it explicitly tokeyword.iskeyword:Behaviour-preserving for all inputs the current code handles
correctly; corrects the dead Python-keyword arm.
Test
Adds
tests/test_ddsidl_naming.pywith a regression test thatasserts
getAllowedName("class") == "_class"(and similar fordef,import,lambda). With the original bug, that test fails.Also covers normal names, IDL keywords, and C keywords to lock in
current behaviour.
Notes
getAllowedNameand the new test file are touched. No othercall sites or behaviour changed.
name.lower()call (three times in the original) isnow called once, which is incidentally cleaner but the primary
motivation is correctness.