gemini-code-assist[bot] commented on code in PR #37248: URL: https://github.com/apache/beam/pull/37248#discussion_r2677754906
########## sdks/python/apache_beam/typehints/opcodes.py: ########## @@ -151,6 +151,9 @@ def get_iter(state, unused_arg): def symmetric_binary_op(state, arg, is_true_div=None): # TODO(robertwb): This may not be entirely correct... + # BINARY_SUBSCR was rolled into BINARY_OP in 3.14. + if arg == 26: + return binary_subscr(state, arg) Review Comment:  Using the magic number `26` here is not ideal for maintainability. It would be better to define a constant by introspecting `opcode._nb_ops` to find the index of `'NB_SUBSCR'`, similar to how `_div_binop_args` is handled. This would make the code more robust against future changes in Python's opcodes. For example, you could define a constant at the top of the file and use it here: ```python # At top of file if sys.version_info >= (3, 11): # ... try: _NB_SUBSCR_OP = [op[0] for op in opcode._nb_ops].index('NB_SUBSCR') except (AttributeError, ValueError): _NB_SUBSCR_OP = -1 else: # ... _NB_SUBSCR_OP = -1 # In symmetric_binary_op if arg == _NB_SUBSCR_OP: return binary_subscr(state, arg) ``` ########## sdks/python/apache_beam/typehints/trivial_inference_test.py: ########## @@ -105,13 +105,13 @@ def reverse(ab): typehints.Tuple[int, int], reverse, [typehints.List[int]]) def testGetItemSlice(self): - self.assertReturnType( - typehints.List[int], lambda v: v[::-1], [typehints.List[int]]) - self.assertReturnType( - typehints.Tuple[int], lambda v: v[::-1], [typehints.Tuple[int]]) - self.assertReturnType(str, lambda v: v[::-1], [str]) - self.assertReturnType(typehints.Any, lambda v: v[::-1], [typehints.Any]) - self.assertReturnType(typehints.Any, lambda v: v[::-1], [object]) + # self.assertReturnType( + # typehints.List[int], lambda v: v[::-1], [typehints.List[int]]) + # self.assertReturnType( + # typehints.Tuple[int], lambda v: v[::-1], [typehints.Tuple[int]]) + # self.assertReturnType(str, lambda v: v[::-1], [str]) + # self.assertReturnType(typehints.Any, lambda v: v[::-1], [typehints.Any]) + # self.assertReturnType(typehints.Any, lambda v: v[::-1], [object]) Review Comment:  These tests for slicing have been commented out. This is not a good practice as it may hide a regression or an incomplete feature. The tests should be updated to work with the new Python 3.14 opcode behavior for slicing. If this is intended to be addressed in a follow-up, please add a `TODO` with a link to a tracking issue. ########## sdks/python/apache_beam/typehints/trivial_inference.py: ########## @@ -512,6 +520,11 @@ def infer_return_type_func(f, input_types, debug=False, depth=0): # stack[-has_kwargs]: Map of keyword args. # stack[-1 - has_kwargs]: Iterable of positional args. # stack[-2 - has_kwargs]: Function to call. + if arg is None: + # CALL_FUNCTION_EX does not take an arg in 3.14, instead the + # signaling for kwargs is done via a PUSH_NULL instruction + # right before CALL_FUNCTION_EX. + arg = ~last_op_push_null & 1 Review Comment:  The logic to determine `has_kwargs` appears to be inverted from what the comment describes. The comment states that `PUSH_NULL` signals the presence of keyword arguments, which means if `last_op_push_null` is `1`, `has_kwargs` should be `1`. However, the expression `~last_op_push_null & 1` evaluates to `0` when `last_op_push_null` is `1`, and `1` when it's `0`. If the comment is correct, the logic should be `arg = last_op_push_null`. It's also worth noting that the Python 3.14 release notes state that `PUSH_NULL` is used to signal keyword arguments for the `CALL` instruction, not `CALL_FUNCTION_EX`. This might also need to be reviewed. ```suggestion arg = last_op_push_null ``` -- 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]
