-
Notifications
You must be signed in to change notification settings - Fork 3
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
feat: Turn int and float into core types #225
Conversation
8c04951
to
d22d4bb
Compare
d22d4bb
to
179b115
Compare
guppylang/tys/builtin.py
Outdated
@@ -79,6 +86,23 @@ def check_instantiate( | |||
return NoneType() | |||
|
|||
|
|||
@dataclass(frozen=True) | |||
class _NumericTypeDef(TypeDef): | |||
"""Type definition associated with the builtin `None` 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.
associated with the Numeric
type!
guppylang/prelude/builtins.py
Outdated
|
||
builtins = GuppyModule("builtins", import_builtins=False) | ||
|
||
T = guppy.type_var(builtins, "T") | ||
L = guppy.type_var(builtins, "L", linear=True) | ||
|
||
|
||
# Define the nat type so scripts can import it | ||
nat = nat_type_def |
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.
I'd expect this to implement some numeric dunder methods, like __add__
, __eq__
, __gt__
?
guppylang/prelude/_internal.py
Outdated
@@ -145,16 +122,12 @@ def synthesize(self, args: list[ast.expr]) -> tuple[ast.expr, Type]: | |||
|
|||
for i in range(len(args)): | |||
args[i], ty = ExprSynthesizer(self.ctx).synthesize(args[i]) | |||
if isinstance(ty, OpaqueType) and ty.defn == self.ctx.globals["int"]: | |||
if isinstance(ty, NumericType) and ty.kind == NumericType.Kind.Int: |
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.
Shouldn't nat
be valid too? Maybe bool?
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.
arguably anything that implements __float__
...
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.
Yeah, we need a better way to figure out which types are automatically coreced (also applies to function tuples etc.). These CustomCallChecker
s are quite hacky...
I'm tempted to just leave it as is until we have a proper solution?
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.
arguably anything that implements float...
This only works as long as float is at the top of the numeric hierarchy. For example, if we add complex
in the future, it might also have a __float__
method but shouldn't be coreced automtically.
Also we'd need coercsion from nat
to int
etc...
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.
I think the better interim solution is that we coerce NumericType
instead of just int
, then come up with something better down the line
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.
I'm surprised not to see any special handling for nat
in the type checker, and that it doesn't have any methods implemented by default. Are we ever checking that nat
s >= 0?
My plan was to leave all of this to another PR so this one doesn't become too big. But I could also add it here if you'd prefer to look at everything? |
I think the better way to split it up would be NumericType in this PR, then the one that adds the functionality also adds |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #225 +/- ##
=======================================
Coverage ? 90.69%
=======================================
Files ? 44
Lines ? 4730
Branches ? 0
=======================================
Hits ? 4290
Misses ? 440
Partials ? 0 ☔ View full report in Codecov by Sentry. |
guppylang/prelude/_internal.py
Outdated
return func | ||
|
||
def synthesize(self, args: list[ast.expr]) -> tuple[ast.expr, Type]: | ||
args, _, inst = synthesize_call(self.func.ty, args, self.node, self.ctx) |
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.
I don't really understand what this call is doing?
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.
It checks args
against the function inputs and potentially infers some type args. The call to self._get_func().synthesize_call()
below delegates to the corresponding integer function.
What's the motivation for this change? |
Mainly consistency with Python. Adding I don't feel super strongly though, so we could reconsider if numeric bools are controversial? |
If we are considering guppy as a strongly typed language, I think |
Co-authored-by: Craig Roy <croyzor@users.noreply.github.com>
guppylang/tys/builtin.py
Outdated
@@ -145,6 +175,10 @@ def bool_type() -> OpaqueType: | |||
return OpaqueType([], bool_type_def) | |||
|
|||
|
|||
def int_type() -> NumericType: | |||
return NumericType(NumericType.Kind.Int) |
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.
Should this not contain some reference to int_type_def
?
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.
Is this used? I see a couple of places it'd work in this PR but it's just being inlined
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.
Should this not contain some reference to int_type_def?
No, only the int_type_def
has a reference to a NumericType
. The inverse mapping from type to type def is implemeted here:
guppylang/guppylang/checker/core.py
Lines 82 to 112 in cf8a529
def get_instance_func(self, ty: Type | TypeDef, name: str) -> CallableDef | None: | |
"""Looks up an instance function with a given name for a type. | |
Returns `None` if the name doesn't exist or isn't a function. | |
""" | |
type_defn: TypeDef | |
match ty: | |
case TypeDef() as type_defn: | |
pass | |
case BoundTypeVar() | ExistentialTypeVar() | SumType(): | |
return None | |
case NumericType(kind): | |
match kind: | |
case NumericType.Kind.Int: | |
type_defn = int_type_def | |
case NumericType.Kind.Float: | |
type_defn = float_type_def | |
case kind: | |
return assert_never(kind) | |
case FunctionType(): | |
type_defn = callable_type_def | |
case OpaqueType() as ty: | |
type_defn = ty.defn | |
case StructType() as ty: | |
type_defn = ty.defn | |
case TupleType(): | |
type_defn = tuple_type_def | |
case NoneType(): | |
type_defn = none_type_def | |
case _: | |
return assert_never(ty) |
Is this used? I see a couple of places it'd work in this PR but it's just being inlined
Good point, it's indeed unused. I'll remove it 👍
args=num_params * [tys.TypeArg(tys.BoundedNatArg(n=INT_WIDTH))], | ||
parent=UNDEFINED, | ||
) | ||
ops.CustomOp(extension=ext, op_name=op_name, args=args, parent=UNDEFINED) |
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.
Nice 👍
🤖 I have created a release *beep* *boop* --- ## [0.6.0](v0.5.2...v0.6.0) (2024-07-02) ### Features * Add array type ([#258](#258)) ([041c621](041c621)) * Add nat type ([#254](#254)) ([a461a9d](a461a9d)) * Add result function ([#271](#271)) ([792fb87](792fb87)), closes [#270](#270) * Allow constant nats as type args ([#255](#255)) ([d706735](d706735)) * Generate constructor methods for structs ([#262](#262)) ([f68d0af](f68d0af)), closes [#261](#261) * Return already-compiled hugrs from `GuppyModule.compile` ([#247](#247)) ([9d01eae](9d01eae)) * Turn int and float into core types ([#225](#225)) ([99217dc](99217dc)) ### Bug Fixes * Add missing test file ([#266](#266)) ([75231fe](75231fe)) * Loading custom polymorphic function defs as values ([#260](#260)) ([d15b2f5](d15b2f5)), closes [#259](#259) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
This PR adds a new
NumericType
to the type hierarchy that now representsint
andfloat
.Closes #250