feat!: Support static methods on types#1699
Conversation
maximilianruesch
left a comment
There was a problem hiding this comment.
Thank you for the PR, this looks really interesting. I have a bunch of suggestions. I will also request @mark-koch to provide his comments on this PR!
| @dataclass(frozen=True) | ||
| class ImplDefinition: | ||
| id: DefId | ||
| is_static: bool |
There was a problem hiding this comment.
For this it is useful to inherit from NamedTuple (and remove the decorator), since you do not need the full blown features of a dataclass. It gives you lower runtime overhead and virtually the same attribute access.
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class ImplDefinition: |
There was a problem hiding this comment.
Let us call this TypeMember or TypeMemberDef. Impl... was the previous wording in this file, which was confusing.
| if isinstance(val, GuppyDefinition) and hasattr( | ||
| val.wrapped, "python_func" | ||
| ): |
There was a problem hiding this comment.
Two things:
Instead of silently failing to register if the wrapped definition has no python_func, let us fail in case it is a GuppyDefinition but we cannot determine whether the function is static or not. If the user can run into this case, it should be a proper error with a diagnostic (and an error test), if not, it can be a InternalGuppyError. This will help us debug / discover new cases where we introduce members that are GuppyDefinition, but have no function attached (for whatever reason).
We discourage hasattr checks, since they are dependent on "implementation-ish" details (characterising a class by which attribute is present), and are not hardened against refactoring changes. Furthermore, from here it is completely invisible which classes may have python_func. Instead, you should use isinstance checks to check all classes that you want to cover explicitly.
There was a problem hiding this comment.
Yes the hasattr is not ideal, the problem I was facing here was that I think I originally had isinstance(..., GuppyFunctionDefinition) but mypy was not able to determine that the wrapped Definition actually contained a python_func in that case. Is there a way to ensure a GuppyFunctionDefinition contains a function?
There was a problem hiding this comment.
GuppyFunctionDefinition is a subclass of GuppyDefinition, and is not the wrapped member. What you are searching for is RawFunctionDef and other similar classes.
| if isinstance(val, GuppyDefinition) and hasattr( | ||
| val.wrapped, "python_func" | ||
| ): |
There was a problem hiding this comment.
| # We can only access variants of the enum from the enum class | ||
| # and staticmethods |
There was a problem hiding this comment.
For one, this comment should be on the line with the name in variants OR is a static member clause, and for another it is worded confusingly: Right now it sounds like we can access variants from [either the enum class or static methods], which does not make sense.
Instead, let us say something like
# We can only access enum variants or static methods from the enum classThere was a problem hiding this comment.
We should really have a CI check for uv.lock synchronisation.
| @guppy | ||
| def main() -> None: | ||
| t = Test.default(1.0) | ||
| # can call Test.default[int] from Test[float] instance |
There was a problem hiding this comment.
A bit counterintuitive, but reasonable in this case. Are there cases where this is not allowed?
There was a problem hiding this comment.
This does seem odd but I think it makes sense, as I would expect the behaviour of staticmethods to be to ignore the instance.
There was a problem hiding this comment.
We should extend the comment by saying "as static methods ignore instance types".
There was a problem hiding this comment.
Can we test static guppy.declares? Perhaps with library linking?
On that note, you should check whether link name inference still works with static methods (in the link_name tests).
| # get the wrapped function if a staticmethod | ||
| if isinstance(f, staticmethod): | ||
| f = f.__func__ |
There was a problem hiding this comment.
The fact that the saved self.python_func is wrapped may be breaking as well (depending on if the wrapper copies or forwards key attributes such as __module__.
There was a problem hiding this comment.
@mark-koch and I briefly discussed this but I would appreciate any more thoughts / discussion to make sure this is ok
There was a problem hiding this comment.
Currently there is no guarantees made by the python_func about its type (because we do not have a proper type for it). However we do access things like __module__ on it, which is defined with regular functions. I guess if you add the link_name tests (and any other paths that may trigger attribute accesses) we should be fine.
There was a problem hiding this comment.
We could consider storing the wrapped function directly instead of unwrapping here. That might be cleaner?
| if node.value.id in self.ctx.globals: | ||
| defn = cast("ParsedDef", self.ctx.globals[node.value.id]) | ||
| if not isinstance(defn, PythonObject): | ||
| ty_def = ENGINE.parsed[defn.id] | ||
| if ( | ||
| node.attr in DEF_STORE.type_members[ty_def.id] | ||
| and isinstance(ty_def, TypeDef) | ||
| and (func := ENGINE.get_instance_func(ty_def, node.attr)) | ||
| and DEF_STORE.type_members[ty_def.id][node.attr].is_static | ||
| ): | ||
| return with_loc( | ||
| node, GlobalName(id=node.attr, def_id=func.id) | ||
| ), func.ty |
There was a problem hiding this comment.
Can this fail when the thing that we are accessing on is not in globals? Can this even happen? I am confused what this does, and what all the if clauses mean. Perhaps @mark-koch has thoughts on this.
There was a problem hiding this comment.
This could definitely be cleaner / wrapped up into a function.
Also the cast / isinstances were also a result of my fight with mypy due to my lack of experience with the compiler types.
Essentially I was going for, "if this is a type that has has a type_member of this name and it's static, then return the staticmethod"
There was a problem hiding this comment.
I am confused in particular about the if not isinstance(defn, PythonObject):. What does it do?
Also, I guess ENGINE.get_instance_func(...) is not necessarily the right name anymore.
Let us wait for a comment from Mark on this one.
Co-authored-by: Copilot <copilot@github.com>
| defn = cast("ParsedDef", self.ctx.globals[node.value.id]) | ||
| if not isinstance(defn, PythonObject): |
There was a problem hiding this comment.
| defn = cast("ParsedDef", self.ctx.globals[node.value.id]) | |
| if not isinstance(defn, PythonObject): | |
| defn_or_python_object = self.ctx.globals[node.value.id] | |
| if isinstance(defn_or_python_object, TypeDef): |
| node.attr in DEF_STORE.type_members[ty_def.id] | ||
| and isinstance(ty_def, TypeDef) | ||
| and (func := ENGINE.get_instance_func(ty_def, node.attr)) | ||
| and DEF_STORE.type_members[ty_def.id][node.attr].is_static |
There was a problem hiding this comment.
Do we actually need this static check? Imo type.func would also be fine if func is not static, similar to Python:
x = int.__add__(1, 2)| if node.value.id in self.ctx.globals: | ||
| defn = cast("ParsedDef", self.ctx.globals[node.value.id]) | ||
| if not isinstance(defn, PythonObject): | ||
| ty_def = ENGINE.parsed[defn.id] |
There was a problem hiding this comment.
It should already be ParsedDef?
| ty_def = ENGINE.parsed[defn.id] | ||
| if ( | ||
| node.attr in DEF_STORE.type_members[ty_def.id] | ||
| and isinstance(ty_def, TypeDef) |
There was a problem hiding this comment.
isinstance(ty_def, TypeDef) could be removed if you go with the refactor above
| if ( | ||
| impl_def := DEF_STORE.type_members[ty_id].get(node.attr) | ||
| ) and impl_def.is_static: |
There was a problem hiding this comment.
I think this would be clearer if we introduced a new ENGINE.get_type_member function that returns the TypeMember:
if member := ENGINE.get_type_member(ty, node.attr):
if member.is_static:
...
else:
...| # comptime methods not yet supported | ||
| case RawTracedFunctionDef(): | ||
| if isinstance(defn.wrapped.python_func, staticmethod): | ||
| # TODO guppy error handling | ||
| raise TypeError("static comptime func") | ||
| else: | ||
| return False |
There was a problem hiding this comment.
What's the issue with comptime functions, I'd assume everything should just work?
There was a problem hiding this comment.
I have not looked into this, but I get 'staticmethod' object has no attribute '__globals__' in mock_builtins
| if not isinstance(func_def, RawFunctionDef): | ||
| return False |
There was a problem hiding this comment.
It could also be a RawCustomFunctionDef, RawFunctionDecl, RawTracedFunctionDef, or even another OverloadedFunctionDef
| # TODO guppy error handling | ||
| raise TypeError( | ||
| "one of the functions in this overload is not static" | ||
| ) |
There was a problem hiding this comment.
I think a vanilla Python error is ok here since it raised while decorating, not during compilation.
However, the message could be improved a bit, for example mentioning which overload is static and which isn't. Also could you add an error test for this case?
| else: | ||
| return False | ||
| case OverloadedFunctionDef(): | ||
| # check all the methods in the overload are also static |
There was a problem hiding this comment.
I think the current implementation doesn't do that. You just check if any overload is static and then raise an error? I would expect something like
is_static = [determine_static(overload_id) for overload_id in defn.wrapped.func_ids]
if all(is_static):
return True
elif not any(is_static):
return False:
else:
raise TypeError("Some static, some not")Note that this would refquire refactoring determine_static to take ids instead of GuppyDefinitions
There was a problem hiding this comment.
Or, maybe we should just look for @staticmethod on the @guppy.overloaded function instead?
| # get the wrapped function if a staticmethod | ||
| if isinstance(f, staticmethod): | ||
| f = f.__func__ |
There was a problem hiding this comment.
We could consider storing the wrapped function directly instead of unwrapping here. That might be cleaner?
Adds staticmethods to guppy, supporting structs and enums.
Main changes include, registering if members are staticmethods, and then checking in
ExprSynthesizer.visit_Attributeif there is a staticmethod when visiting a Name.Whether or not members are static could probably mostly be inferred from the type instead (except for the case where the first argument to the staticmethod is also of type
Self).BREAKING CHANGE:
DEF_STORE.type_membersnow has values that also contain information about whether or not members are static.Closes #694