Repository navigation
Conversation
…gnments Class bodies silently dropped tuple unpacking and subscript assignments (e.g. `lower, upper = (2, 7)` or `table["value"] = 7` inside a class). Delegate to the same set_value() used by regular assignments, with class-body-first name scoping via ChainMap, so class bodies accept the same assignment targets as module-level code. Closes huggingface#2940
VANDRANKI
left a comment
There was a problem hiding this comment.
This is a community review. It does not clear the merge gate.
The fix itself works, but I found a regression: class attributes whose names match a built-in tool now raise. Details below. I ran everything on Windows with Python 3.13, PR head against main 96f33fa, through evaluate_python_code with BASE_PYTHON_TOOLS.
What works (ran):
- Tuple targets, subscript targets, chained targets and an attribute target on an outer class (
class B: passthenB.z = 4inside another class body) all work on the PR. On main the first three are silently dropped or fail later, and the last raisesThe variable B is not defined. - A method reading the unpacked attributes (
a, b = 1, 2thenself.a + self.b) returns 3 on the PR and fails with AttributeError on main. - A subscript on an outer dict inside the class body (
t['v'] = 5) now writes through to the outer dict, as in native Python. - An
Enumclass with plain members still works. - The PR's new test fails on main and passes on the PR.
tests/test_local_python_executor.py: 396 passed, 1 failed, 3 skipped on the PR, and the single failure (TestLocalPythonExecutorSecurity::test_vulnerability_for_all_dangerous_functions[os.system]) also fails on main. I did not investigate it.
Regression (ran). set_value for an ast.Name target raises Cannot assign to name '<x>': doing this would erase the existing tool! when the name is in static_tools. That check makes sense for module-level code, but class-body names are attributes of the class, not rebindings of the tool. The old class-body branch wrote class_dict[target.id] = value without that check. BASE_PYTHON_TOOLS includes min, max, sum, str, type, len, round, abs, map, filter, list, dict, set, int, float, sorted and range. Results:
| code | main | PR |
|---|---|---|
class Limits: with min = 0 and max = 10, then (Limits.min, Limits.max) |
(0, 10) |
InterpreterError, erase the existing tool |
class A: with sum = 3 |
3 |
InterpreterError |
class A: with str = 'x' |
'x' |
InterpreterError |
The model-written code this executor runs can reasonably have an attribute named min/max (ranges, limits, enum-like constants), so I think this should be fixed before merge. One option: only run the tool-name check for the module and function scope, and keep a plain class_dict[target.id] = value for ast.Name targets inside evaluate_class_def, while still delegating Tuple, Subscript and Attribute targets. I have not tried that change, so I do not know whether it interacts with the tuple case (for example min, max = 1, 2 inside a class body, which currently raises on the PR for the same reason).
Not covered by the PR, FYI: starred unpacking in a class body (a, *b = 1, 2, 3) raises Cannot unpack tuple of wrong size on the PR. That comes from set_value, which does not support starred targets at all, so it is consistent with module-level behavior. I did not test module level.
set_value's tool-rebinding guard is meant for module/function scope, where an assignment would erase the tool. Class-body writes land in class_dict, so a class attribute named e.g. min/max can never erase a module-level tool — skip the guard there (including for tuple-unpacking targets). Adds a regression test: class attributes may shadow built-in tool names, while the module-level rebinding guard still raises.
Class bodies silently dropped tuple unpacking and subscript assignments. For example:
The
ast.Assignbranch ofevaluate_class_defonly handledast.Nameandast.Attributetargets, soast.Tuple/ast.Subscripttargets fell through with no assignment and no error, while the same code at module level worked fine.The fix delegates to the same
set_value()helper regular assignments use, with class-body-first name scoping (ChainMap(class_dict, state)) so a target object liketableresolves from enclosing scopes while writes always land in the class dict. Verified the reproducer from the issue against native Python control, plus attribute targets, methods, and class-local dict targets. Addedtest_evaluate_class_def_with_assign_tuple_and_subscript_targets; fulltest_local_python_executor.pysuite passes (the only 2 failures are pre-existing scipy/sklearn import errors from missing optional deps in my env).Closes #2940