tvalentyn commented on code in PR #31170:
URL: https://github.com/apache/beam/pull/31170#discussion_r1589470721
##########
sdks/python/apache_beam/typehints/trivial_inference.py:
##########
@@ -143,6 +143,8 @@ def copy(self):
return FrameState(self.f, self.vars, self.stack, self.kw_names)
def const_type(self, i):
+ print(self.co.co_consts)
Review Comment:
leftover / `if debug` ?
##########
sdks/python/apache_beam/typehints/trivial_inference.py:
##########
@@ -432,6 +433,8 @@ def infer_return_type_func(f, input_types, debug=False,
depth=0):
elif op in dis.haslocal:
print('(' + co.co_varnames[arg] + ')', end=' ')
elif op in dis.hascompare:
+ if (sys.version_info.major, sys.version_info.minor) >= (3, 12):
+ arg = arg >> 4
Review Comment:
for my knowledge, where does this come from? Perhaps there is a link you
could add in a comment.
##########
sdks/python/apache_beam/typehints/intrinsic_one_ops.py:
##########
@@ -0,0 +1,98 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+"""Defines the actions intrinsic bytecodes have on the frame.
+
+Each function here corresponds to a bytecode documented in
+https://docs.python.org/2/library/dis.html or
Review Comment:
Py2 docs shouldn't be relevant anymore.
##########
sdks/python/apache_beam/typehints/intrinsic_one_ops.py:
##########
@@ -0,0 +1,98 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+"""Defines the actions intrinsic bytecodes have on the frame.
+
+Each function here corresponds to a bytecode documented in
+https://docs.python.org/2/library/dis.html or
+https://docs.python.org/3/library/dis.html. The first argument is a (mutable)
+FrameState object, the second the integer opcode argument.
+
+Bytecodes with more complicated behavior (e.g. modifying the program counter)
+are handled inline rather than here.
+
+For internal use only; no backwards-compatibility guarantees.
+"""
+# pytype: skip-file
+
+from . import opcodes
+
+
+def intrinsic_1_invalid(state, arg):
+ pass
+
+
+def intrinsic_print(state, arg):
+ pass
+
+
+def intrinsic_import_star(state, arg):
+ pass
+
+
+def intrinsic_stopiteration_error(state, arg):
+ pass
+
+
+def intrinsic_async_gen_wrap(state, arg):
+ pass
+
+
+def intrinsic_unary_positive(state, arg):
+ opcodes.unary_positive(state, arg)
+ pass
+
+
+def intrinsic_list_to_tuple(state, arg):
+ opcodes.list_to_tuple(state, arg)
+ pass
+
+
+def intrinsic_typevar(state, arg):
+ pass
+
+
+def intrinsic_paramspec(state, arg):
+ pass
+
+
+def intrinsic_typevartuple(state, arg):
+ pass
+
+
+def intrinsic_subscript_generic(state, arg):
+ pass
+
+
+def intrinsic_typealias(state, arg):
+ pass
+
+
+int_one_ops = [
Review Comment:
Could you add a comment what defines the order of ops in this sequence? How
can we make sure that this list stays up to date with future Python versions
potentially adding more intrinsic ops? I worry that the order might become
skewed but tests we won't notice the skew.
Is it possible to add a unit test that runs on Python 3.12+ which would
fail if INT_ONE_OPS doesn't match the list of intrinsic ops known to `dis`? If
that's not easy, maybe a unit test that would just fail on Python 3.13 with an
instruction to: revisit intrinsic_one_ops.INT_ONE_OPS, make necessary changes,
and change the test to fail starting next Python minor version. Then, it will
make use manually look at this list once in a while.
##########
sdks/python/apache_beam/typehints/intrinsic_one_ops.py:
##########
@@ -0,0 +1,98 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+"""Defines the actions intrinsic bytecodes have on the frame.
+
+Each function here corresponds to a bytecode documented in
+https://docs.python.org/2/library/dis.html or
+https://docs.python.org/3/library/dis.html. The first argument is a (mutable)
+FrameState object, the second the integer opcode argument.
+
+Bytecodes with more complicated behavior (e.g. modifying the program counter)
+are handled inline rather than here.
+
+For internal use only; no backwards-compatibility guarantees.
+"""
+# pytype: skip-file
+
+from . import opcodes
+
+
+def intrinsic_1_invalid(state, arg):
+ pass
+
+
+def intrinsic_print(state, arg):
+ pass
+
+
+def intrinsic_import_star(state, arg):
+ pass
+
+
+def intrinsic_stopiteration_error(state, arg):
+ pass
+
+
+def intrinsic_async_gen_wrap(state, arg):
+ pass
+
+
+def intrinsic_unary_positive(state, arg):
+ opcodes.unary_positive(state, arg)
+ pass
+
+
+def intrinsic_list_to_tuple(state, arg):
+ opcodes.list_to_tuple(state, arg)
+ pass
+
+
+def intrinsic_typevar(state, arg):
+ pass
+
+
+def intrinsic_paramspec(state, arg):
+ pass
+
+
+def intrinsic_typevartuple(state, arg):
+ pass
+
+
+def intrinsic_subscript_generic(state, arg):
+ pass
+
+
+def intrinsic_typealias(state, arg):
+ pass
+
+
+int_one_ops = [
Review Comment:
```suggestion
INT_ONE_OPS = tuple([
```
(to make it immutable and and to show it's a constant.
##########
sdks/python/apache_beam/typehints/intrinsic_one_ops.py:
##########
@@ -0,0 +1,98 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+"""Defines the actions intrinsic bytecodes have on the frame.
+
+Each function here corresponds to a bytecode documented in
+https://docs.python.org/2/library/dis.html or
+https://docs.python.org/3/library/dis.html. The first argument is a (mutable)
Review Comment:
```suggestion
https://docs.python.org/3/library/dis.html . The first argument is a
(mutable)
```
(to make it clickable)
##########
sdks/python/apache_beam/typehints/opcodes.py:
##########
@@ -347,6 +388,8 @@ def load_attr(state, arg):
Will replace with Any for builtin methods, but these don't have bytecode in
CPython so that's okay.
"""
+ if (sys.version_info.major, sys.version_info.minor) >= (3, 12):
+ arg = arg >> 1
Review Comment:
for my knowledge, where does this come from? Perhaps there is a link you
could add in a comment.
##########
sdks/python/apache_beam/typehints/intrinsic_one_ops.py:
##########
@@ -0,0 +1,98 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+"""Defines the actions intrinsic bytecodes have on the frame.
+
+Each function here corresponds to a bytecode documented in
+https://docs.python.org/2/library/dis.html or
+https://docs.python.org/3/library/dis.html. The first argument is a (mutable)
+FrameState object, the second the integer opcode argument.
+
+Bytecodes with more complicated behavior (e.g. modifying the program counter)
+are handled inline rather than here.
+
+For internal use only; no backwards-compatibility guarantees.
+"""
+# pytype: skip-file
+
+from . import opcodes
+
+
+def intrinsic_1_invalid(state, arg):
+ pass
+
+
+def intrinsic_print(state, arg):
+ pass
+
+
+def intrinsic_import_star(state, arg):
+ pass
+
+
+def intrinsic_stopiteration_error(state, arg):
+ pass
+
+
+def intrinsic_async_gen_wrap(state, arg):
+ pass
+
+
+def intrinsic_unary_positive(state, arg):
+ opcodes.unary_positive(state, arg)
+ pass
+
+
+def intrinsic_list_to_tuple(state, arg):
+ opcodes.list_to_tuple(state, arg)
+ pass
+
+
+def intrinsic_typevar(state, arg):
+ pass
+
+
+def intrinsic_paramspec(state, arg):
+ pass
+
+
+def intrinsic_typevartuple(state, arg):
+ pass
+
+
+def intrinsic_subscript_generic(state, arg):
+ pass
+
+
+def intrinsic_typealias(state, arg):
+ pass
+
+
+int_one_ops = [
+ intrinsic_1_invalid,
+ intrinsic_print,
+ intrinsic_import_star,
+ intrinsic_stopiteration_error,
+ intrinsic_async_gen_wrap,
+ intrinsic_unary_positive,
+ intrinsic_list_to_tuple,
+ intrinsic_typevar,
+ intrinsic_paramspec,
+ intrinsic_typevartuple,
+ intrinsic_subscript_generic,
+ intrinsic_typealias
+]
Review Comment:
```suggestion
])
```
##########
sdks/python/apache_beam/typehints/opcodes.py:
##########
@@ -188,6 +204,31 @@ def store_subscr(unused_state, unused_args):
pass
+def binary_slice(state, args):
+ _ = state.stack.pop()
+ _ = state.stack.pop()
+ base = Const.unwrap(state.stack.pop())
+ if base is str:
+ out = base
+ elif isinstance(base, typehints.IndexableTypeConstraint):
+ out = base
+ else:
+ out = element_type(base)
Review Comment:
For my understanding, what is the interpretation of
`out = element_type(base)`?
does this say: take a container, and return the type of individual element
that lives in the container? Wouldn't a slice always return a container as
opposed to individual elements?
##########
sdks/python/apache_beam/typehints/trivial_inference.py:
##########
@@ -641,6 +654,18 @@ def infer_return_type_func(f, input_types, debug=False,
depth=0):
# No-op introduced in 3.11. Without handling this some
# instructions have functionally > 2 byte size.
pass
+ elif opname == 'RETURN_CONST':
+ # Introduced in 3.12. Handles returning constants directly
+ # instead of having a LOAD_CONST before a RETURN_VALUE.
+ returns.add(state.const_type(arg))
+ state = None
+ elif opname == 'CALL_INTRINSIC_1':
+ int_op = intrinsic_one_ops.int_one_ops[arg]
+ if debug:
+ print("Executing intrinsic one op", int_op.__name__.upper())
+ int_op(state, arg)
+ elif opname == 'CALL_INTRINSIC_2':
Review Comment:
Should we add a comment sth like: all currently available binary intrinsic
ops are not relevant for type-checking, and as i mentioned above, also have
some safeguard to revisit this logic if we can detect that the list of
intrinsic ops has changed.
--
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]
