-
Notifications
You must be signed in to change notification settings - Fork 32
feat!: Support static methods on types #1699
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
29a7895
5ed374d
68b8552
5c9a4be
78f1165
f1506a4
2ec633c
ee959de
d522f88
722472c
2bddc3c
2a5c42b
ee63214
7f9d6ee
70a7282
4744454
28be41f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,7 +28,7 @@ | |
| from contextlib import suppress | ||
| from dataclasses import replace | ||
| from types import ModuleType | ||
| from typing import TYPE_CHECKING, Any, NoReturn | ||
| from typing import TYPE_CHECKING, Any, NoReturn, cast | ||
|
|
||
| from guppylang_internals.ast_util import ( | ||
| AstNode, | ||
|
|
@@ -88,11 +88,11 @@ | |
| UnaryOperatorNotDefinedError, | ||
| WrongNumberOfArgsError, | ||
| ) | ||
| from guppylang_internals.definition.common import Definition | ||
| from guppylang_internals.definition.common import Definition, ParsedDef | ||
| from guppylang_internals.definition.parameter import ParamDef | ||
| from guppylang_internals.definition.ty import TypeDef | ||
| from guppylang_internals.definition.value import CallableDef, ValueDef | ||
| from guppylang_internals.engine import ENGINE | ||
| from guppylang_internals.engine import DEF_STORE, ENGINE | ||
| from guppylang_internals.error import ( | ||
| GuppyComptimeError, | ||
| GuppyError, | ||
|
|
@@ -580,7 +580,23 @@ def visit_Attribute(self, node: ast.Attribute) -> tuple[ast.expr, Type]: | |
| # Name can be a EnumDef only if it is in a attribute access, thus we | ||
| # manually need to call the helper instead of relying on the standard | ||
| # visit_Name (that is called through synthesize) | ||
|
|
||
| # visit node for case of staticmethods on a non-instantiated type | ||
| ty = get_type_opt(node.value) | ||
| 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] | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should already be |
||
| if ( | ||
| node.attr in DEF_STORE.type_members[ty_def.id] | ||
| and isinstance(ty_def, TypeDef) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| and (func := ENGINE.get_instance_func(ty_def, node.attr)) | ||
| and DEF_STORE.type_members[ty_def.id][node.attr].is_static | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we actually need this static check? Imo x = int.__add__(1, 2) |
||
| ): | ||
| return with_loc( | ||
| node, GlobalName(id=node.attr, def_id=func.id) | ||
| ), func.ty | ||
|
Comment on lines
+586
to
+598
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This could definitely be cleaner / wrapped up into a function.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am confused in particular about the Also, I guess Let us wait for a comment from Mark on this one. |
||
|
|
||
| if ty is None: | ||
| node.value, ty = self._check_name_id( | ||
| node.value.id, node.value, allow_enum=True | ||
|
|
@@ -642,16 +658,28 @@ def visit_Attribute(self, node: ast.Attribute) -> tuple[ast.expr, Type]: | |
|
|
||
| def _check_method( | ||
| self, ty: Type, node: ast.Attribute | ||
| ) -> tuple[PartialApply, FunctionType] | None: | ||
| ) -> tuple[ast.expr, FunctionType] | None: | ||
| """Helper method to check if an attribute access corresponds to a method call""" | ||
| if func := ENGINE.get_instance_func(ty, node.attr): | ||
| name = with_type( | ||
| func.ty, with_loc(node, GlobalName(id=func.name, def_id=func.id)) | ||
| ) | ||
| # Make a closure by partially applying the `self` argument | ||
| # TODO: Try to infer some type args based on `self` | ||
| result_ty = FunctionType(func.ty.inputs[1:], func.ty.output, func.ty.params) | ||
| return with_loc(node, PartialApply(func=name, args=[node.value])), result_ty | ||
| ty_id = DEF_STORE.type_member_parents[func.id] | ||
|
|
||
| if ( | ||
| impl_def := DEF_STORE.type_members[ty_id].get(node.attr) | ||
| ) and impl_def.is_static: | ||
|
Comment on lines
+669
to
+671
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this would be clearer if we introduced a new if member := ENGINE.get_type_member(ty, node.attr):
if member.is_static:
...
else:
... |
||
| # if this is a staticmethod do not partially apply `self` | ||
| return with_loc(node, GlobalName(id=node.attr, def_id=func.id)), func.ty | ||
| else: | ||
| # Make a closure by partially applying the `self` argument | ||
| # TODO: Try to infer some type args based on `self` | ||
| result_ty = FunctionType( | ||
| func.ty.inputs[1:], func.ty.output, func.ty.params | ||
| ) | ||
| return with_loc( | ||
| node, PartialApply(func=name, args=[node.value]) | ||
| ), result_ty | ||
| else: | ||
| return None | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -317,19 +317,22 @@ def check_signature( | |
| param = parse_parameter(param_node, i, globals, param_var_mapping) | ||
| param_var_mapping[param.name] = param | ||
|
|
||
| is_static = False | ||
| # Figure out if this is a method | ||
| self_defn: TypeDef | None = None | ||
| if def_id is not None and def_id in DEF_STORE.type_member_parents: | ||
| self_def_id = DEF_STORE.type_member_parents[def_id] | ||
| self_defn = cast("TypeDef", ENGINE.get_checked(self_def_id, mono_args=())) | ||
| assert isinstance(self_defn, TypeDef) | ||
| # Figure out if this is a staticmethod | ||
| is_static = DEF_STORE.type_members[self_def_id][func_def.name].is_static | ||
|
|
||
| inputs = [] | ||
| ctx = TypeParsingCtx(globals, param_var_mapping, allow_free_vars=True) | ||
| for i, inp in enumerate(func_def.args.args): | ||
| # Special handling for `self` arguments. Note that `__new__` is excluded here | ||
| # since it's not a method so doesn't take `self`. | ||
| if self_defn and i == 0 and func_def.name != "__new__": | ||
| # Special handling for `self` arguments. Note that `__new__` and staticmethods | ||
| # are excluded here since they do not take `self`. | ||
| if self_defn and i == 0 and func_def.name != "__new__" and not is_static: | ||
|
Comment on lines
+333
to
+335
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Once this PR lands, we should turn |
||
| input = parse_self_arg(inp, self_defn, ctx) | ||
| ctx = replace(ctx, self_ty=input.ty) | ||
| else: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,11 +22,15 @@ | |
| OpCompiler, | ||
| RawCustomFunctionDef, | ||
| ) | ||
| from guppylang_internals.definition.declaration import RawFunctionDecl | ||
| from guppylang_internals.definition.function import RawFunctionDef | ||
| from guppylang_internals.definition.overloaded import OverloadedFunctionDef | ||
| from guppylang_internals.definition.traced import RawTracedFunctionDef | ||
| from guppylang_internals.definition.ty import OpaqueTypeDef, TypeDef | ||
| from guppylang_internals.definition.wasm import RawWasmFunctionDef | ||
| from guppylang_internals.dummy_decorator import _dummy_custom_decorator, sphinx_running | ||
| from guppylang_internals.engine import DEF_STORE | ||
| from guppylang_internals.error import GuppyError, pretty_errors | ||
| from guppylang_internals.error import GuppyError, InternalGuppyError, pretty_errors | ||
| from guppylang_internals.std._internal.checker import WasmCallChecker | ||
| from guppylang_internals.std._internal.compiler.wasm import ( | ||
| WasmModuleCallCompiler, | ||
|
|
@@ -158,12 +162,47 @@ def extend_type(defn: TypeDef, return_class: bool = False) -> Callable[[type], t | |
| def dec(c: type) -> type: | ||
| for val in c.__dict__.values(): | ||
| if isinstance(val, GuppyDefinition): | ||
| DEF_STORE.register_type_member(defn.id, val.wrapped.name, val.id) | ||
| DEF_STORE.register_type_member( | ||
| defn.id, val.wrapped.name, val.id, is_static=determine_static(val) | ||
| ) | ||
| return c if return_class else GuppyDefinition(defn) # type: ignore[return-value] | ||
|
|
||
| return dec | ||
|
|
||
|
|
||
| def determine_static(defn: GuppyDefinition) -> bool: | ||
| """If defn corresponds to a function, check if it is static.""" | ||
| if not isinstance(defn, GuppyFunctionDefinition): | ||
| return False | ||
| match defn.wrapped: | ||
| case RawFunctionDef() | RawCustomFunctionDef() | RawFunctionDecl(): | ||
| return isinstance(defn.wrapped.python_func, staticmethod) | ||
| # 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 | ||
|
Comment on lines
+180
to
+186
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the issue with comptime functions, I'd assume everything should just work?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have not looked into this, but I get |
||
| case OverloadedFunctionDef(): | ||
| # check all the methods in the overload are also static | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or, maybe we should just look for |
||
| for func_id in defn.wrapped.func_ids: | ||
| func_def = DEF_STORE.raw_defs[func_id] | ||
| if not isinstance(func_def, RawFunctionDef): | ||
| return False | ||
|
Comment on lines
+191
to
+192
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It could also be a |
||
| if not isinstance(func_def.python_func, staticmethod): | ||
| # TODO guppy error handling | ||
| raise TypeError( | ||
| "one of the functions in this overload is not static" | ||
| ) | ||
|
Comment on lines
+194
to
+197
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
| return True | ||
| case _: | ||
| raise InternalGuppyError( | ||
| f"Cannot determine staticness of GuppyFunctionDefinition wrapping \ | ||
| {type(defn.wrapped)}" | ||
| ) | ||
|
|
||
|
|
||
| def custom_type( | ||
| hugr_ty: ht.Type | Callable[[Sequence[Argument], CompilerContext], ht.Type], | ||
| name: str = "", | ||
|
|
@@ -202,7 +241,9 @@ def dec(c: type[T]) -> type[T]: | |
| DEF_STORE.register_def(defn, get_calling_frame()) | ||
| for val in c.__dict__.values(): | ||
| if isinstance(val, GuppyDefinition): | ||
| DEF_STORE.register_type_member(defn.id, val.wrapped.name, val.id) | ||
| DEF_STORE.register_type_member( | ||
| defn.id, val.wrapped.name, val.id, is_static=determine_static(val) | ||
| ) | ||
| # We're pretending to return the class unchanged, but in fact we return | ||
| # a `GuppyDefinition` that handles the comptime logic | ||
| return GuppyDefinition(defn) # type: ignore[return-value] | ||
|
|
@@ -300,7 +341,10 @@ def dec(cls: builtins.type[T]) -> GuppyDefinition: | |
| for val in cls.__dict__.values(): | ||
| if isinstance(val, GuppyDefinition): | ||
| DEF_STORE.register_type_member( | ||
| ext_module.id, val.wrapped.name, val.id | ||
| ext_module.id, | ||
| val.wrapped.name, | ||
| val.id, | ||
| is_static=determine_static(val), | ||
| ) | ||
| wasm_def: RawWasmFunctionDef | ||
| if isinstance(val, GuppyFunctionDefinition) and isinstance( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -325,6 +325,9 @@ def compile_call( | |
|
|
||
|
|
||
| def parse_py_func(f: PyFunc, sources: SourceMap) -> tuple[ast.FunctionDef, str | None]: | ||
| # get the wrapped function if a staticmethod | ||
| if isinstance(f, staticmethod): | ||
| f = f.__func__ | ||
|
Comment on lines
+328
to
+330
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The fact that the saved
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mark-koch and I briefly discussed this but I would appreciate any more thoughts / discussion to make sure this is ok
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Currently there is no guarantees made by the
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could consider storing the wrapped function directly instead of unwrapping here. That might be cleaner? |
||
| source_lines, line_offset = inspect.getsourcelines(f) | ||
| source, func_ast, line_offset = parse_source(source_lines, line_offset) | ||
| file = inspect.getsourcefile(f) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -75,7 +75,6 @@ | |
| setattr, | ||
| slice, | ||
| sorted, | ||
| staticmethod, | ||
| sum, | ||
| super, | ||
| type, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.