Re: [PR] fix: prevent silent NULL-handle creation in Object.__init__ [tvm-ffi]
junrushao closed pull request #449: fix: prevent silent NULL-handle creation in Object.__init__ URL: https://github.com/apache/tvm-ffi/pull/449 -- 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]
Re: [PR] fix: prevent silent NULL-handle creation in Object.__init__ [tvm-ffi]
junrushao commented on PR #449: URL: https://github.com/apache/tvm-ffi/pull/449#issuecomment-3947083993 Closing - will open a new PR once unblocked. -- 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]
Re: [PR] fix: prevent silent NULL-handle creation in Object.__init__ [tvm-ffi]
junrushao commented on PR #449: URL: https://github.com/apache/tvm-ffi/pull/449#issuecomment-3903001142 blocked by cuteDSL usage of this private API -- 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]
Re: [PR] fix: prevent silent NULL-handle creation in Object.__init__ [tvm-ffi]
tqchen commented on code in PR #449: URL: https://github.com/apache/tvm-ffi/pull/449#discussion_r2808222012 ## python/tvm_ffi/cython/object.pxi: ## @@ -116,6 +116,12 @@ cdef class Object: # case of error before chandle is set self.chandle = NULL +def __init__(self): Review Comment: there might be a cases that depends on class construction and handle move, so we need to cross check downstream for this change -- 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]
Re: [PR] fix: prevent silent NULL-handle creation in Object.__init__ [tvm-ffi]
junrushao commented on code in PR #449: URL: https://github.com/apache/tvm-ffi/pull/449#discussion_r2808218988 ## python/tvm_ffi/access_path.py: ## @@ -105,7 +105,6 @@ def _path_equal(self, _1: AccessPath, /) -> bool: ... def __init__(self) -> None: """Disallow direct construction; use `AccessPath.root()` instead.""" -super().__init__() raise ValueError( Review Comment: **[high | API Consistency | Consensus]** `AccessPath.__init__` raises `ValueError` while the base `Object.__init__` now raises `TypeError`. Three different exception types are used for "cannot construct": - `Object.__init__` → `TypeError` - `__init__invalid` (registry) → `RuntimeError` - `AccessPath.__init__` → `ValueError` `TypeError` is the most Pythonic choice for "this class cannot be instantiated this way" (matches abstract classes and `__new__` guards). Consider unifying: ```python def __init__(self) -> None: """Disallow direct construction; use `AccessPath.root()` instead.""" raise TypeError( "AccessPath cannot be initialized directly. " "Use AccessPath.root() to create a path to the root object" ) ``` _Source: Consensus (Claude + Codex)_ ## python/tvm_ffi/cython/object.pxi: ## @@ -116,6 +116,12 @@ cdef class Object: # case of error before chandle is set self.chandle = NULL +def __init__(self): Review Comment: **[low | Maintainability | Codex]** The error message hardcodes "FFI constructors" and "@register_object" as the solutions. If new mechanisms for object creation are added, this message won't mention them. Minor suggestion — consider making it slightly more generic, e.g. adding "factory methods" as an option. _Source: Codex_ ## tests/python/test_object.py: ## @@ -136,6 +136,33 @@ def test_opaque_type_error() -> None: ) +def test_object_direct_init_disabled() -> None: +"""Object() and unregistered subclasses must not silently create NULL-handle objects.""" +# Base Object class +with pytest.raises(TypeError, match="Cannot directly create Object instance"): +tvm_ffi.Object() + +# Unregistered subclass without __init__ +class UnregisteredObj(tvm_ffi.Object): +pass + +with pytest.raises(TypeError, match="Cannot directly create UnregisteredObj instance"): +UnregisteredObj() + +# Registered class without __c_ffi_init__ should still raise +with pytest.raises(RuntimeError, match="__init__ method of this class is not implemented"): Review Comment: **[medium | Correctness | Claude]** The match string `"__init__ method of this class is not implemented"` is a fragile substring — the actual message in `registry.py:387` is `"The __init__ method of this class is not implemented."` (starts with "The"). While `re.search` makes this work, a reader may think the full message starts with `"__init__"`. **[medium | Test Clarity | Codex]** It's also unclear from the test whether the `RuntimeError` comes from the missing `__c_ffi_init__` path or from the new `__init__` guard. A brief comment clarifying the error path would help. Suggested fix: ```python # Registered class without __c_ffi_init__: the registry replaces __init__ # with __init__invalid, which raises RuntimeError (not the base TypeError) with pytest.raises(RuntimeError, match="The __init__ method of this class is not implemented"): tvm_ffi.testing.TestObjectBase() ``` _Source: Claude (match string) + Codex (clarity)_ ## tests/python/test_object.py: ## @@ -136,6 +136,33 @@ def test_opaque_type_error() -> None: ) +def test_object_direct_init_disabled() -> None: Review Comment: **[medium | Test Coverage | Claude]** Missing test for `AccessPath.__init__` after removing `super().__init__()`. The removal changes exception ordering — previously the base `Object.__init__` (no-op) ran first, then `ValueError` fired. Now only `ValueError` fires directly. If someone accidentally removes the `raise ValueError`, the base `TypeError` would kick in with different semantics. Suggested addition: ```python def test_access_path_direct_init_raises(): """AccessPath() must raise ValueError directing users to root().""" from tvm_ffi.access_path import AccessPath with pytest.raises(ValueError, match="AccessPath can.t be initialized directly"): AccessPath() ``` _Source: Claude_ ## tests/python/test_object.py: ## @@ -136,6 +136,33 @@ def test_opaque_type_error() -> None: ) +def test_object_direct_init_disabled() -> None: +"""Object() and unregistered subclasses must not silently create NULL-handle objects.""" +# Base Object class +with pytest.raises(TypeError, match="Cannot directly create Object instance"): +tvm_ffi.Object() + +# Unregis
Re: [PR] fix: prevent silent NULL-handle creation in Object.__init__ [tvm-ffi]
gemini-code-assist[bot] commented on PR #449: URL: https://github.com/apache/tvm-ffi/pull/449#issuecomment-3902920581 ## Summary of Changes Hello @junrushao, I'm Gemini Code Assist[^1]! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the robustness of object instantiation by explicitly preventing the creation of `Object` instances with `NULL` handles through direct calls or unregistered subclasses. By raising a `TypeError` early, it provides clearer feedback to developers about incorrect usage patterns, thereby improving debugging and code reliability. ### Highlights * **Error Prevention**: Introduced a `TypeError` in `Object.__init__` to prevent silent creation of objects with `NULL` handles when `Object()` is called directly or an unregistered subclass without its own `__init__` is instantiated. * **Code Refinement**: Removed a redundant `super().__init__()` call from `AccessPath.__init__` as it is no longer necessary with the new `Object.__init__` behavior. * **Test Coverage**: Added a new test case, `test_object_direct_init_disabled`, to verify that direct instantiation of `Object` or unregistered subclasses now correctly raises a `TypeError`. 🧠 **New Feature in Public Preview:** You can now enable **Memory** to help **Gemini Code Assist** learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. **Click [here](https://codeassist.google/code-review/login) to enable Memory in your admin console.** Changelog * **python/tvm_ffi/access_path.py** * Removed `super().__init__()` call from `AccessPath.__init__`. * **python/tvm_ffi/cython/object.pxi** * Added an `__init__` method to the `Object` class that raises a `TypeError`. * **tests/python/test_object.py** * Added `test_object_direct_init_disabled` to validate the new `Object` initialization behavior. Using Gemini Code Assist The full guide for Gemini Code Assist can be found on our [documentation page](https://developers.google.com/gemini-code-assist/docs/review-github-code), here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either `/gemini ` or `@gemini-code-assist `. Below is a summary of the supported commands on the current page. Feature | Command | Description --- | --- | --- Code Review | `/gemini review` | Performs a code review for the current pull request in its current state. Pull Request Summary | `/gemini summary` | Provides a summary of the current pull request in its current state. Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in pull request comments and review comments. Help | `/gemini help` | Displays a list of available commands. Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a `.gemini/` folder in the base of the repository. Detailed instructions can be found [here](https://developers.google.com/gemini-code-assist/docs/customize-gemini-behavior-github). Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with :thumbsup: and :thumbsdown: on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up [here](https://google.qualtrics.com/jfe/form/SV_2cyuGuTWsEw84yG). You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the [Gemini Code Assist IDE Extension](https://cloud.google.com/products/gemini/code-assist). [^1]: Review the [Privacy Notices](https://policies.google.com/privacy), [Generative AI Prohibited Use Policy](https://policies.google.com/terms/generative-ai/use-policy), [Terms of Service](https://policies.google.com/terms), and learn how to configure Gemini Code Assist in GitHub [here](https://developers.google.com/gemini-code-assist/docs/customize-gemini-behavior-github). Gemini can make mistakes, so double check it and [use code with caution](https://support.google.com/legal/answer/13505487). -- 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]
