gemini-code-assist[bot] commented on code in PR #309: URL: https://github.com/apache/tvm-ffi/pull/309#discussion_r2587498447
########## python/tvm_ffi/utils/kwargs_wrapper.py: ########## @@ -0,0 +1,360 @@ +# 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. +"""Utilities for creating high-performance keyword argument wrapper functions. + +This module provides tools for wrapping positional-only callables with +keyword argument support using code generation techniques. +""" + +from __future__ import annotations + +import functools +import inspect +from typing import Any, Callable + +# Sentinel object for missing arguments +MISSING = object() + + +def _validate_argument_names(names: list[str], arg_type: str) -> None: + """Validate that argument names are valid Python identifiers and unique. + + Parameters + ---------- + names + List of argument names to validate. + arg_type + Description of the argument type (e.g., "Argument", "Keyword-only argument"). + + """ + # Check for duplicate names + if len(set(names)) != len(names): + raise ValueError(f"Duplicate {arg_type.lower()} names found in: {names}") + + # Validate each name is a valid identifier + for name in names: + if not isinstance(name, str): + raise TypeError( + f"{arg_type} name must be a string, got {type(name).__name__}: {name!r}" + ) + if not name.isidentifier(): + raise ValueError( + f"Invalid {arg_type.lower()} name: {name!r} is not a valid Python identifier" + ) Review Comment:  The `isidentifier()` method returns `True` for Python keywords (e.g., 'if', 'for'), which are not valid as parameter names. This will cause a `SyntaxError` when `exec()` is called on the generated function string. You should also check if the name is a keyword using `keyword.iskeyword()`. This will require importing the `keyword` module at the top of the file. ```suggestion if not name.isidentifier() or keyword.iskeyword(name): raise ValueError( f"Invalid {arg_type.lower()} name: {name!r} is not a valid Python identifier" ) ``` ########## tests/python/utils/test_kwargs_wrapper.py: ########## @@ -0,0 +1,321 @@ +# 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. +from __future__ import annotations + +import inspect +from typing import Any + +import pytest +from tvm_ffi.utils.kwargs_wrapper import make_kwargs_wrapper, make_kwargs_wrapper_from_signature + + +def test_basic_wrapper() -> None: + """Test basic wrapper functionality with various argument combinations.""" + + def target(*args: Any) -> int: + return sum(args) + + # No defaults - all required + wrapper = make_kwargs_wrapper(target, ["a", "b", "c"]) + assert wrapper(1, 2, 3) == 6 + assert wrapper(a=1, b=2, c=3) == 6 + assert wrapper(1, b=2, c=3) == 6 + + # Single default argument + wrapper = make_kwargs_wrapper(target, ["a", "b", "c"], args_defaults=(10,)) + assert wrapper(1, 2) == 13 # c=10 + assert wrapper(1, 2, 3) == 6 # c=3 explicit + assert wrapper(1, 2, c=5) == 8 # c=5 via keyword + + # Multiple defaults (right-aligned) + wrapper = make_kwargs_wrapper(target, ["a", "b", "c"], args_defaults=(20, 30)) + assert wrapper(1) == 51 # b=20, c=30 + assert wrapper(1, 2) == 33 # b=2, c=30 + assert wrapper(1, 2, 3) == 6 # all explicit + + # All defaults + wrapper = make_kwargs_wrapper(target, ["a", "b", "c"], args_defaults=(1, 2, 3)) + assert wrapper() == 6 + assert wrapper(10) == 15 + assert wrapper(10, 20, 30) == 60 + + # Bound methods + class Calculator: + def __init__(self, base: int) -> None: + self.base = base + + def add(self, a: int, b: int) -> int: + return self.base + a + b + + calc = Calculator(100) + wrapper = make_kwargs_wrapper(calc.add, ["a", "b"], args_defaults=(5,)) + assert wrapper(1) == 106 + + +def test_keyword_only_arguments() -> None: + """Test wrapper with keyword-only arguments.""" + + def target(*args: Any) -> int: + return sum(args) + + # Optional keyword-only arguments (with defaults) + wrapper = make_kwargs_wrapper( + target, + ["a", "b"], + args_defaults=(), + kwargsonly_names=["c", "d"], + kwargsonly_defaults={"c": 100, "d": 200}, + ) + assert wrapper(1, 2) == 303 # c=100, d=200 + assert wrapper(1, 2, c=10) == 213 # d=200 + assert wrapper(1, 2, c=10, d=20) == 33 + + wrapper = make_kwargs_wrapper( + target, ["a", "b"], args_defaults=(), kwargsonly_names=["c", "d"], kwargsonly_defaults={} + ) + assert wrapper(1, 2, c=10, d=20) == 33 # c and d are required + + wrapper = make_kwargs_wrapper( + target, + ["a", "b"], + args_defaults=(), + kwargsonly_names=["c", "d"], + kwargsonly_defaults={"d": 100}, + ) + assert wrapper(1, 2, c=10) == 113 # c required, d=100 + assert wrapper(1, 2, c=10, d=20) == 33 # both explicit + + wrapper = make_kwargs_wrapper( + target, + ["a", "b", "c"], + args_defaults=(10,), + kwargsonly_names=["d", "e"], + kwargsonly_defaults={"d": 20, "e": 30}, + ) + assert wrapper(1, 2) == 63 # c=10, d=20, e=30 + assert wrapper(1, 2, 5, d=15) == 53 # c=5 explicit, e=30 + + +def test_validation_errors() -> None: + """Test input validation and error handling.""" + target = lambda *args: sum(args) + + # Duplicate positional argument names + with pytest.raises(ValueError, match="Duplicate argument names found"): + make_kwargs_wrapper(target, ["a", "b", "a"]) + + # Duplicate keyword-only argument names + with pytest.raises(ValueError, match="Duplicate keyword-only argument names found"): + make_kwargs_wrapper(target, ["a"], kwargsonly_names=["b", "c", "b"]) + + # Invalid argument name types + with pytest.raises(TypeError, match="Argument name must be a string"): + make_kwargs_wrapper(target, ["a", 123]) # type: ignore[list-item] + + # Invalid Python identifiers + with pytest.raises(ValueError, match="not a valid Python identifier"): + make_kwargs_wrapper(target, ["a", "b-c"]) + + # args_defaults not a tuple + with pytest.raises(TypeError, match="args_defaults must be a tuple"): + make_kwargs_wrapper(target, ["a", "b"], args_defaults=[10]) # type: ignore[arg-type] + + # args_defaults too long + with pytest.raises(ValueError, match=r"args_defaults has .* values but only"): + make_kwargs_wrapper(target, ["a"], args_defaults=(1, 2, 3)) + + # Overlap between positional and keyword-only + with pytest.raises(ValueError, match="cannot be both positional and keyword-only"): + make_kwargs_wrapper(target, ["a", "b"], kwargsonly_names=["b"]) + + # kwargsonly_defaults key not in kwargsonly_names + with pytest.raises(ValueError, match="not in kwargsonly_names"): + make_kwargs_wrapper( + target, ["a", "b"], kwargsonly_names=["c"], kwargsonly_defaults={"d": 10} + ) + + # Internal name conflict + with pytest.raises(ValueError, match="conflict with internal names"): + make_kwargs_wrapper(target, ["__i_target_func", "b"]) Review Comment:  To ensure that Python keywords are not accepted as argument names, it would be beneficial to add a test case for this scenario. This will serve as a regression test for the validation logic you'll be adding. ```suggestion make_kwargs_wrapper(target, ["__i_target_func", "b"]) # Python keyword as argument name with pytest.raises(ValueError, match="not a valid Python identifier"): make_kwargs_wrapper(target, ["a", "for"]) ``` -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
