gemini-code-assist[bot] commented on code in PR #39210:
URL: https://github.com/apache/beam/pull/39210#discussion_r3544416464
##########
sdks/python/apache_beam/typehints/trivial_inference.py:
##########
@@ -145,20 +158,42 @@ def copy(self):
def const_type(self, i):
return Const(self.co.co_consts[i])
- def get_closure(self, i):
- num_cellvars = len(self.co.co_cellvars)
- if i < num_cellvars:
- return self.vars[i]
- else:
- return self.f.__closure__[i - num_cellvars].cell_contents
-
def closure_type(self, i):
- """Returns a TypeConstraint or Const."""
- val = self.get_closure(i)
- if isinstance(val, typehints.TypeConstraint):
- return val
+ """Returns the type of the cell or free variable with the given index.
+
+ The index is the raw oparg of a LOAD_CLOSURE or LOAD_DEREF instruction.
+ For Python < 3.11 it indexes co_cellvars + co_freevars. From Python 3.11
+ on it indexes the frame's "fast locals" (localsplus) storage, in which
+ the cell of a captured parameter shares the slot of the parameter
+ itself, so the slot layout is co_varnames, followed by the cell
+ variables that are not parameters, followed by the free variables.
+ """
+ if sys.version_info >= (3, 11):
+ names = self.co.co_varnames + tuple(
+ c for c in self.co.co_cellvars
+ if c not in self.co.co_varnames) + self.co.co_freevars
else:
+ names = self.co.co_cellvars + self.co.co_freevars
+ name = names[i]
Review Comment:
If `i` is out of bounds for `names` (e.g., due to unexpected bytecode or
version-specific layout), accessing `names[i]` will raise an `IndexError`.
Adding a guard prevents potential crashes during type inference.
```python
if i >= len(names):
return typehints.Any
name = names[i]
```
##########
sdks/python/apache_beam/typehints/trivial_inference.py:
##########
@@ -145,20 +158,42 @@ def copy(self):
def const_type(self, i):
return Const(self.co.co_consts[i])
- def get_closure(self, i):
- num_cellvars = len(self.co.co_cellvars)
- if i < num_cellvars:
- return self.vars[i]
- else:
- return self.f.__closure__[i - num_cellvars].cell_contents
-
def closure_type(self, i):
- """Returns a TypeConstraint or Const."""
- val = self.get_closure(i)
- if isinstance(val, typehints.TypeConstraint):
- return val
+ """Returns the type of the cell or free variable with the given index.
+
+ The index is the raw oparg of a LOAD_CLOSURE or LOAD_DEREF instruction.
+ For Python < 3.11 it indexes co_cellvars + co_freevars. From Python 3.11
+ on it indexes the frame's "fast locals" (localsplus) storage, in which
+ the cell of a captured parameter shares the slot of the parameter
+ itself, so the slot layout is co_varnames, followed by the cell
+ variables that are not parameters, followed by the free variables.
+ """
+ if sys.version_info >= (3, 11):
+ names = self.co.co_varnames + tuple(
+ c for c in self.co.co_cellvars
+ if c not in self.co.co_varnames) + self.co.co_freevars
else:
+ names = self.co.co_cellvars + self.co.co_freevars
+ name = names[i]
+ if name in self.co.co_freevars:
+ # A free variable: its cell belongs to the function's closure.
+ val = self.f.__closure__[self.co.co_freevars.index(name)].cell_contents
Review Comment:
If `self.f` is a function object without a closure (or if it is
mocked/constructed dynamically), `self.f.__closure__` can be `None`. Accessing
it directly will raise a `TypeError`. We should add a defensive check to handle
this safely.
```python
if self.f.__closure__ is None or len(self.f.__closure__) <=
self.co.co_freevars.index(name):
return typehints.Any
val = self.f.__closure__[self.co.co_freevars.index(name)].cell_contents
```
##########
sdks/python/apache_beam/typehints/trivial_inference.py:
##########
@@ -145,20 +158,42 @@ def copy(self):
def const_type(self, i):
return Const(self.co.co_consts[i])
- def get_closure(self, i):
- num_cellvars = len(self.co.co_cellvars)
- if i < num_cellvars:
- return self.vars[i]
- else:
- return self.f.__closure__[i - num_cellvars].cell_contents
-
def closure_type(self, i):
- """Returns a TypeConstraint or Const."""
- val = self.get_closure(i)
- if isinstance(val, typehints.TypeConstraint):
- return val
+ """Returns the type of the cell or free variable with the given index.
+
+ The index is the raw oparg of a LOAD_CLOSURE or LOAD_DEREF instruction.
+ For Python < 3.11 it indexes co_cellvars + co_freevars. From Python 3.11
+ on it indexes the frame's "fast locals" (localsplus) storage, in which
+ the cell of a captured parameter shares the slot of the parameter
+ itself, so the slot layout is co_varnames, followed by the cell
+ variables that are not parameters, followed by the free variables.
+ """
+ if sys.version_info >= (3, 11):
+ names = self.co.co_varnames + tuple(
+ c for c in self.co.co_cellvars
+ if c not in self.co.co_varnames) + self.co.co_freevars
else:
+ names = self.co.co_cellvars + self.co.co_freevars
+ name = names[i]
+ if name in self.co.co_freevars:
+ # A free variable: its cell belongs to the function's closure.
+ val = self.f.__closure__[self.co.co_freevars.index(name)].cell_contents
+ if isinstance(val, _TypeInCell):
+ # A synthetic cell produced while emulating MAKE_FUNCTION: it holds
+ # the inferred type of the captured variable, not an actual runtime
+ # value.
+ return val.value
return Const(val)
+ try:
+ # A cell variable of the current frame. The frame state tracks the
+ # inferred *type* of each local variable rather than its value, so the
+ # tracked type can be returned directly.
+ return self.vars[self.co.co_varnames.index(name)]
+ except ValueError:
Review Comment:
If `self.vars` is shorter than `self.co.co_varnames` (e.g., if some local
variables are not tracked), `self.vars[self.co.co_varnames.index(name)]` can
raise an `IndexError`. We should catch `IndexError` along with `ValueError` to
ensure robustness.
```suggestion
except (ValueError, IndexError):
```
##########
sdks/python/apache_beam/typehints/trivial_inference.py:
##########
@@ -468,13 +503,15 @@ def infer_return_type_func(f, input_types, debug=False,
depth=0):
print('(' + dis.cmp_op[arg] + ')', end=' ')
elif op in dis.hasfree:
if free is None:
- free = co.co_cellvars + co.co_freevars
- # From 3.11 on the arg is no longer offset by len(co_varnames)
- # so we adjust it back
- print_arg = arg
- if (sys.version_info.major, sys.version_info.minor) >= (3, 11):
- print_arg = arg - len(co.co_varnames)
- print('(' + free[print_arg] + ')', end=' ')
+ # From 3.11 on the arg indexes the localsplus storage, in which
+ # the cell of a captured parameter shares the parameter's slot.
+ if (sys.version_info.major, sys.version_info.minor) >= (3, 11):
Review Comment:
For consistency with the rest of the file (such as line 171), we should use
the more idiomatic `sys.version_info >= (3, 11)` check instead of
`(sys.version_info.major, sys.version_info.minor) >= (3, 11)`.
```suggestion
if sys.version_info >= (3, 11):
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
