Fix false negative for disallowed-name on non-constant module-level names - #11241
Conversation
… names `NameChecker.visit_assignname` short-circuited on `_meets_exception_for_non_consts()` before reaching `_check_name()`. That guard exists to avoid an `invalid-name` false positive on module-level names whose assigned value is not a `nodes.Const`, but skipping the call entirely also skipped the `disallowed-name` check, so a name explicitly listed in `bad-names` went unreported. Pass the guard through as `disallowed_check_only` instead, the parameter `_check_name()` already provides for this situation, so `disallowed-name` is still emitted while `invalid-name` stays suppressed. `disallowed_check_only` now also skips registering the name for multiple naming style detection. That registration happens before the existing `disallowed_check_only` guard and reports `invalid-name` from `leave_module()`, which would otherwise have reintroduced the false positive for users configuring `name-group` with named-group patterns. Closes pylint-dev#10679 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #11241 +/- ##
=======================================
Coverage 96.35% 96.35%
=======================================
Files 178 178
Lines 19932 19940 +8
=======================================
+ Hits 19205 19213 +8
Misses 727 727
🚀 New features to boost your workflow:
|
This comment has been minimized.
This comment has been minimized.
Pin the current behavior of the name-group census for module-level names: a name redefining an import in an except ImportError handler still takes part in it, while a name bound to a non-constant inside a module-level loop does not.
Pierre-Sassoulas
left a comment
There was a problem hiding this comment.
Thank you for working on pylint. This change the behavior of un unrelated check, I added a regression test and pushed it on your branch, because explaining what to do is harder. (pull your branch before working on it please :) )
|
|
||
| for _ in range(3): | ||
| tata = {}.keys() # [disallowed-name] | ||
| some_other_name = {}.keys() |
There was a problem hiding this comment.
It matches variable-rgx, so it stays silent with or without your change mabe I didn't understand the intent of the test ?
| @@ -0,0 +1,5 @@ | |||
| Fix a false negative for ``disallowed-name`` on module-level names assigned a | |||
| value that is not a constant. The control flow that suppresses ``invalid-name`` | |||
There was a problem hiding this comment.
Module-level names whose value cannot be inferred still emit nothing.
| @@ -0,0 +1,5 @@ | |||
| Fix a false negative for ``disallowed-name`` on module-level names assigned a | |||
| value that is not a constant. The control flow that suppresses ``invalid-name`` | |||
| for these names no longer skips the ``disallowed-name`` check as well. | |||
There was a problem hiding this comment.
I'd state the user-visible change instead of describing it ("foo = {}.keys() at module level is now reported").
The previous revision gated _is_multi_naming_match on disallowed_check_only, which conflated two different reasons for suppressing invalid-name. A name redefining an import should still take part in the multiple naming style census; only a name that previously never reached _check_name at all should stay out of it. Split them: skip_name_group_census is now set solely by _meets_exception_for_non_consts, leaving redefines_import to suppress the message while remaining in the census, as before. Both functional tests added in b20bde9 now pass.
|
Thanks, and the test was much clearer than an explanation would have been. Pulled and fixed. You were right about what broke. I had gated
My guard treated both the same and dropped the import case out of the census, which is exactly what your Split them with a separate Both of your tests pass now, and the loop one passes for the right reason rather than by accident. Full Worth flagging since it is a real question rather than a rhetorical one: the census behaviour for the non-const case is now pinned by your loop test, but it is still a behaviour change relative to |
|
🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉 This comment was generated for commit 52d9764 |
Closes #10679.
visit_assignnamewrapped the_check_name()calls for module-scope names inif not self._meets_exception_for_non_consts(...). That guard exists to suppress aninvalid-namefalse positive, but because it short-circuits before_check_name()is entered, it suppressesdisallowed-namealong with it.So a module-level name that is explicitly listed in
bad-namesis silently not reported whenever its value is a non-Constexpression:On
mainonly line 3 is reported. This is the case marked by the TODO left intests/functional/d/disallowed_name.pyby #10677:The change
_check_name()already takesdisallowed_check_only, which does exactly this job and is already used for theredefines_importcase. Both guards now pass that flag instead of skipping the call, soinvalid-namestays suppressed anddisallowed-nameis still emitted.The part worth pushing back on
There is a third change, and it is arguably beyond the minimal fix.
Once
_check_name()is entered for these names, they also reach_is_multi_naming_match(), which records them for the multi-naming-style report. That would make a name start influencing theC0103naming-group heuristic purely because it is on thebad-nameslist, which is not what the guard was suppressing it for. I gated that call onnot disallowed_check_onlyand added a functional test for it.If you would rather keep this PR to the two call sites and treat the multi-naming interaction separately, say so and I will drop that hunk and its test.
Tests
tests/functional/d/disallowed_name.pyline 9 changes from the TODO comment to an actual# [disallowed-name]expectation, with the.txtregenerated via--update-functional-output. A newdisallowed_name_multi_naming_stylefunctional test covers the third change.Reverting only
checker.pyand keeping the tests fails them. Fulltests/test_functional.py: 875 passed on a clean checkout, 876 with this branch, with an identical set of 12 pre-existing failures in both runs (wrong_import_order,typing_broken_noreturn,typevar_naming_style_defaultand others, all Python 3.14 / astroid 4.3.0 issues unrelated to this change). Changelog fragment added underdoc/whatsnew/fragments/.I used an AI assistant while working on this. The reproduction, the baseline comparison and the decision to flag the multi-naming hunk separately are mine.