feat: Parse inout annotations in function signatures #316

mark-koch · 2024-07-24T11:19:50Z

Closes #309. See #282 for context.

codecov-commenter · 2024-07-29T15:39:10Z

Codecov Report

Attention: Patch coverage is 92.85714% with 5 lines in your changes missing coverage. Please review.

Project coverage is 92.63%. Comparing base (86c8b31) to head (ff545a7).

Files	Patch %	Lines
guppylang/tys/parsing.py	91.37%	5 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff               @@
##           feat/inout     #316      +/-   ##
==============================================
+ Coverage       92.62%   92.63%   +0.01%     
==============================================
  Files              45       45              
  Lines            5165     5188      +23     
==============================================
+ Hits             4784     4806      +22     
- Misses            381      382       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Closes #347 and closes #312

mark-koch · 2024-07-30T12:38:58Z

tests/error/misc_errors/return_flag_callable.err

+Guppy compilation failed. Error in file $FILE:14
+
+12:    @guppy.declare(module)
+13:    def foo(f: Callable[[], qubit @inout]) -> None: ...
+                  ^^^^^^^^^^^^^^^^^^^^^^^^^^
+GuppyError: `@` type annotations are not allowed in this position


The broad error location is not ideal, but improving this requires a bigger refactor

acl-cqc

Ok, so I've written some quite long comments suggesting a couple of big refactors. I think commoning up check_signature with _CallableTypeDef.check_instantiate should be done, unless it's much harder than it looks, and (if you do nothing else) then I'd do FlaggedArgs -> FlaggedArg.

But I'd also think seriously about breaking up arg_from_ast, and switching from tuple[type, flags] to a broader union (TypeArg | ConstArg | InoutTypeArg, say - maybe there are benefits in the approach taken by python's Annotated), even if you don't go that way.

acl-cqc · 2024-07-30T17:06:24Z

guppylang/tys/parsing.py



 def arg_from_ast(
    node: AstNode,
    globals: Globals,
    param_var_mapping: dict[str, Parameter] | None = None,
-) -> Argument:
+) -> tuple[Argument, InputFlags]:


arg! (argh!) Sorry but I got getting really confused here; what is an Argument? Is it a value Hugr static TypeArg? (It certainly looks like one) - but then we wouldn't want it to be inout or have InputFlags, would we? More comments would really help, maybe also think about renaming Argument->StaticArg, say.

I think arg_from_ast parses both TypeArgs (Hugr TypeArgs of kind TypeParam::Type), and ConstArgs (Hugr TypeArgs of other kinds). But, hang on, ConstArgs are not function parameters (that'd be like def foo(x: 6) !). And @inout is only appropriate for the top/outermost level argument to a def, or to Callable, yet we make life difficult for all the other check_instantiates in order to fit in Callable (e.g. list[int @inout]) - I see 5 calls to check_no_flags, plus a couple that have no args to have any flags.

I think passing the flag by subclassing could be an easier route, you might still have an equivalent of check_no_flags but I reckon it might just fall out of normal check_all_args or checking-against-TypeParam (that is: make a normal Hugr TypeParam::Type unable to accept an InoutTypeArg, but Callable has a special TypeParam::MaybeInOutType that can accept both InoutTypeArg and normal TypeArg).

Alternatively....this'll be even more controversial....can you special-case Callable?

Generally I think I'd favour breaking the lengthy arg_from_ast up into flagged_arg_from_ast or (maybe_)inout_arg_from_ast that looks for BinOp with @ and then (usually) falls back to a shorter arg_from_ast that does not support that case, and thus, returns a TypeArg (| ConstArg) only. (Or returns Argument without separate flags, that'd be a first/lesser step from what's in the PR now).

Which is to say that (my gut feeling is) you'd do better to push error checking downwards towards the parsing/leaves, rather than parsing whatever and then raising error messages (like, found function parameter of type 6). Not sure if that would help with the error locations. Then, say, maybe_inout_arg_from_ast would (if not BinOp) call type_arg_from_ast (that returns TypeArg, and does not support ConstArg), and then there is const_or_type_arg_from_ast (used for arguments to parameterized types), that kind of thing.

Note you've already gone this route with type_from_arg vs type_with_flags_from_arg, so it's not thaaaat radical....

Of course we should also think about likely future annotations. list[MyStruct @frozen] is plausible, true....

arg! (argh!) Sorry but I got getting really confused here; what is an Argument? Is it a value Hugr static TypeArg?

Sorry for the confusion, yes Argument is Guppy's version of static arguments to types. E.g. the type array[int, 5] has a TypeArg int and a ConstArg 5.

More comments would really help, maybe also think about renaming Argument->StaticArg, say.

I added #358 to consider the rename

And @inout is only appropriate for the top/outermost level argument to a def, or to Callable, yet we make life difficult for all the other check_instantiates in order to fit in Callable (e.g. list[int @inout]) [...] Alternatively....this'll be even more controversial....can you special-case Callable?

After reflecting a bit on this I think that special casing Callable is probaby the best solution in the near term. I initially wanted a general solution that can handle future flags like @frozen etc., but I agree it adds a lot of complexity and probably isn't worth it for now. I like your subclassing idea though, I'll probably go for that if we want more general flags in the future 👍

The neatest solution here (I think) is adding a new TypeParam. However it's definitely worth having a think about what the future flags might be - Frozen<T> might be a lot like T (i.e. acceptable anywhere a T is?), so probably needs special consideration, whereas inout is clearly a bit special (it's not really a property of the parameter but of the whole function signature).

Special-casing Callable might be fine in the short-term, tho, and maybe even in the longer term if it turns out we don't need similar specials for any of the others...

Special-casing Callable might be fine in the short-term, tho, and maybe even in the longer term if it turns out we don't need similar specials for any of the others...

Agreed, I think I'll do that for now 👍

Another benefit of special casing Callable is that we can now properly handle the list syntax Callable[[Arg1, Arg2], Return] without the flattening hack

acl-cqc · 2024-07-30T17:12:10Z

guppylang/definition/ty.py


 if TYPE_CHECKING:
    from guppylang.checker.core import Globals


+FlaggedArgs: TypeAlias = Sequence[tuple[Argument, InputFlags]]


Most uses have gone from Sequence[Argument] to FlaggedArgs, but you do have a couple of tuple[Argument, InputFlags] elsewhere. Maybe define FlaggedArg = tuple[...] and use that (so Sequence[FlaggedArg] in many places)....albeit, elsewhere I suggest a bigger redifinition of FlaggedArg ;)

This no longer applied after ff545a7

acl-cqc · 2024-07-30T17:13:59Z

guppylang/prelude/builtins.py

@@ -55,6 +55,16 @@ def py(*_args: Any) -> Any:
    raise GuppyError("`py` can only by used in a Guppy context")


+class _Inout:
+    """Dummy class to support `@inout` annotations."""


This is to allow def foo(x: int @inout), since Python (in its infinite wisdom cough splutter boo) disallows def foo(x: @inout int). Obviously, it's fine to do matrix multiplication inside a type annotation but not invoke a decorator....

However, have you thought about the "pythonic" alternative, which I believe would be def foo(x: Annotated[int, "inout"]?@inout is a bastardization of python syntax anyway so what would users think? I admit Annotated is rather long - what about def foo(x: Inout[int]) ? (No way, I hear you say - and I see the downside - maybe that lengthy Annotated isn't sooo bad, then....)

Not seriously recommending you change, but how different / how much simpler might that make things, would it impact the design at all?

I created #359 for discussion.

Not seriously recommending you change, but how different / how much simpler might that make things, would it impact the design at all?

I think this is mostly a syntax concern. The design wouldn't be affected much imo

acl-cqc · 2024-07-30T17:21:19Z

guppylang/checker/func_checker.py

@@ -148,8 +148,13 @@ def check_signature(func_def: ast.FunctionDef, globals: Globals) -> FunctionType
    for inp in func_def.args.args:
        if inp.annotation is None:
            raise GuppyError("Argument type must be annotated", inp)
-        ty = type_from_ast(inp.annotation, globals, param_var_mapping)
-        inputs.append(FuncInput(ty, InputFlags.NoFlags))
+        ty, flags = type_with_flags_from_ast(inp.annotation, globals, param_var_mapping)


This does seem a bit like, we have two places that define the set(s) of flags legal in the signature of a function - _CallableTypeDef also makes similar checks. If you parsed type_with_flags_from_ast for the return type too, you could call some single checking function (maybe it takes a list[FlaggedArg] and a single FlaggedArg and then _CallableTypeDef first separates out the last element of the list) that issues appropriate errors and returns some appropriate e.g. (list[TypeArg],TypeArg)

acl-cqc · 2024-07-30T17:37:56Z

guppylang/tys/parsing.py

+    param_var_mapping: dict[str, Parameter] | None = None,
+) -> tuple[Type, InputFlags]:
+    """Turns an AST expression into a Guppy type possibly annotated with @flags."""
+    # Parse an argument and check that it's valid for a `TypeParam`


Yes, it's amusing to think that we could have got this far with x: 6 @inout ;-). But I'm reassured that you are making the right check!

acl-cqc

Thank you for changes @mark-koch, looks good to me! :). Improvement in error location is nice too.

I wonder about also being able to parse Annotated[<type>, inout] just so that there is an allowed "pythonic" syntax too (we claim guppy is pythonic after all! And then we can see what people do with it), but I suspect that's more effort than it's worth...

acl-cqc · 2024-08-01T08:07:31Z

guppylang/tys/builtin.py

-                "`@` type annotations are not allowed in this position", loc
-            )
-        return FunctionType([FuncInput(ty, flags) for ty, flags in inputs], output_ty)
+        # Callable types are constructed using special login in the type parser


Suggested change

# Callable types are constructed using special login in the type parser

# Callable types are constructed using special logic in the type parser

You could consider moving some of that code into CallableTypeDef itself, just so you could have a comment "Callable types are constructed using instantiate_func_type()" or something like that, but this is fine

acl-cqc · 2024-08-01T08:09:52Z

guppylang/tys/parsing.py

-            return arg_from_ast(stmt.value, globals, param_var_mapping)
-        except (SyntaxError, ValueError):
-            raise GuppyError("Invalid Guppy type", node) from None
+        node = _parse_delayed_annotation(node.value, node)


Yes, breaking that out is a good move :)

acl-cqc · 2024-08-01T08:10:43Z

guppylang/tys/parsing.py

+    globals: Globals,
+    param_var_mapping: dict[str, Parameter] | None,
+) -> FunctionType:
+    """Helper function to parse a `Callable[[<arguments>], <return types>]` type."""


Suggested change

"""Helper function to parse a `Callable[[<arguments>], <return types>]` type."""

"""Helper function to parse a `Callable[[<arguments>], <return type>]` type."""

Or are multiple return types allowed??

No it should only be a single return type (that could be a tuple of multiple types) 👍

acl-cqc · 2024-08-01T08:11:24Z

guppylang/tys/parsing.py

+    """Helper function to parse a `Callable[[<arguments>], <return types>]` type."""
+    err = (
+        "Function types should be specified via "
+        "`Callable[[<arguments>], <return types>]`"


Ditto

Suggested change

"`Callable[[<arguments>], <return types>]`"

"`Callable[[<arguments>], <return type>]`"

I wonder about also noting, this is standard python hyping!

acl-cqc · 2024-08-01T08:13:28Z

guppylang/tys/parsing.py

+    """Turns an AST expression into a Guppy type with some optional @flags."""
+    # Check for `type @flag` annotations
+    if isinstance(node, ast.BinOp) and isinstance(node.op, ast.MatMult):
+        ty, flags = type_with_flags_from_ast(node.left, globals, param_var_mapping)


Note this means x @inout @inout is also allowed and has the same effect as only once

Indeed, I think that should be fine though

This reverts commit b8348a7.

Closes #309. See #282 for context.

@guppy

This is the dev branch for the inout argument feature (tracked by #282). The idea is to allow explicit `@inout` annotations on function arguments that "give back" the passed value after the function returns: ```python @guppy def foo(q: qubit @inout) -> None: ... @guppy def bar(q1: qubit @inout, q2: qubit @inout) -> bool: ... @guppy def main() -> None: q1, q2 = qubit(), qubit() foo(q1) # Desugars to `q1 = foo(q1)` x = bar(q1, q2) # Desugars to `q1, q2, x = bar(q1, q2)` y = bar(q1, q1) # Error: Linearity violation, q1 used twice ``` To enable this, we need to enforce that `@inout` arguments are not moved in the body of the function (apart from passing them in another `@inout` position). This means that the argument will always be bound to the same name and never aliased which allows us to desugar `@inout` functions like ```python @guppy def bar(q1: qubit, q2: qubit) -> bool: [body] return [expr] ``` into ```python @guppy def bar(q1: qubit, q2: qubit) -> tuple[qubit, qubit, bool]: [body] return q1, q2, [expr] # Linearity checker needs to ensure that q1, q2 are unused ``` Note that we only allow `@inout` annotations on linear types, since they would be useless for classical ones (unless we also implement an ownership system for classical values). Supporting them would make the checking logic more complicated without providing any meaningful benefit. Tracked PRs: * #315 * #316 * #349 * #344 * #321 * #331 * #350 * #339 * #340 * #351

🤖 I have created a release *beep* *boop* --- ## [0.10.0](v0.9.0...v0.10.0) (2024-09-11) ### ⚠ BREAKING CHANGES * Bumped the `hugr` dependency to `0.8.0` * `GuppyModule.load` no longer loads the content of modules but instead just brings the name of the module into scope. Use `GuppyModule.load_all` to get the old behaviour. * Removed `guppylang.hugr_builder.hugr.Hugr`, compiling a module returns a `hugr.Package` instead. ### Features * Add `__version__` field to guppylang ([#473](#473)) ([b996c62](b996c62)) * Add angle type ([#449](#449)) ([12e41e0](12e41e0)) * Add array literals ([#446](#446)) ([a255c02](a255c02)) * Add equality test for booleans ([#394](#394)) ([dd702ce](dd702ce)), closes [#363](#363) * Add pi constant ([#451](#451)) ([9d35a78](9d35a78)) * Add qualified imports and make them the default ([#443](#443)) ([553ec51](553ec51)) * Allow calling of methods ([#440](#440)) ([5a59da3](5a59da3)) * Allow imports of function definitions and aliased imports ([#432](#432)) ([e23b666](e23b666)) * Array indexing ([#415](#415)) ([2199b48](2199b48)), closes [#421](#421) [#422](#422) [#447](#447) * Inout arguments ([#311](#311)) ([060649b](060649b)), closes [#315](#315) [#316](#316) [#349](#349) [#344](#344) [#321](#321) [#331](#331) [#350](#350) [#340](#340) [#351](#351) * range() with single-argument ([#452](#452)) ([d05f369](d05f369)) * Skip checking of redefined functions ([#457](#457)) ([7f9ad32](7f9ad32)) * Support `nat`/`int` ↔ `bool` cast operations ([#459](#459)) ([3b778c3](3b778c3)) * Use `hugr-cli` for validation ([#455](#455)) ([1d0667b](1d0667b)) * Use cell name instead of file for notebook errors ([#382](#382)) ([d542601](d542601)) * Use the hugr builder ([536abf9](536abf9)) ### Bug Fixes * Fix and update demo notebook ([#376](#376)) ([23b2a15](23b2a15)) * Fix linearity checking bug ([#441](#441)) ([0b8ea21](0b8ea21)) * Fix struct definitions in notebooks ([#374](#374)) ([b009465](b009465)) ### Documentation * Update readme, `cargo build` instead of `--extra validation` ([#471](#471)) ([c2a4c86](c2a4c86)) ### Miscellaneous Chores * Update hugr to `0.8.0` ([#454](#454)) ([b02e0d0](b02e0d0)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

feat: Store inout flags in FunctionType

2bd7fdf

mark-koch requested a review from a team as a code owner July 24, 2024 11:19

mark-koch requested review from acl-cqc and removed request for a team July 24, 2024 11:19

mark-koch force-pushed the inout/parse branch from dd758f3 to 3c69d61 Compare July 24, 2024 11:22

mark-koch mentioned this pull request Jul 24, 2024

feat: Inout arguments #311

Merged

feat: Parse inout annotations in function signatures

b5a590a

mark-koch force-pushed the inout/parse branch from 3c69d61 to b5a590a Compare July 24, 2024 13:54

mark-koch added 2 commits July 25, 2024 09:32

Pack input types and flags into dataclass

c455a54

Merge branch 'inout/flags' into inout/parse

68ece53

Base automatically changed from inout/flags to feat/inout July 25, 2024 09:19

Merge branch 'feat/inout' into inout/parse

11f2e25

feat: Move flag parsing into arg_from_ast (#349)

bf070ab

Closes #347 and closes #312

mark-koch commented Jul 30, 2024

View reviewed changes

acl-cqc reviewed Jul 30, 2024

View reviewed changes

mark-koch mentioned this pull request Jul 31, 2024

Consider renaming Argument to StaticArg #358

Open

mark-koch added 2 commits July 31, 2024 12:22

Undo parsing changes

77a4dc8

Special case parsing of Callable

ff545a7

mark-koch force-pushed the inout/parse branch from c5e3232 to ff545a7 Compare July 31, 2024 16:25

mark-koch requested a review from acl-cqc July 31, 2024 16:44

acl-cqc approved these changes Aug 1, 2024

View reviewed changes

mark-koch merged commit b8348a7 into feat/inout Aug 9, 2024
2 checks passed

mark-koch deleted the inout/parse branch August 9, 2024 10:42

mark-koch added a commit that referenced this pull request Aug 9, 2024

Revert "feat: Parse inout annotations in function signatures (#316)"

891197c

This reverts commit b8348a7.

mark-koch restored the inout/parse branch August 9, 2024 10:43

mark-koch added a commit that referenced this pull request Aug 9, 2024

feat: Parse inout annotations in function signatures (#316)

5a72043

Closes #309. See #282 for context.

mark-koch added a commit that referenced this pull request Aug 9, 2024

feat: Parse inout annotations in function signatures (#316)

4316ac9

Closes #309. See #282 for context.

mark-koch mentioned this pull request Aug 9, 2024

Parse inout annotation in function signatures #309

Closed

hugrbot mentioned this pull request Aug 22, 2024

chore(main): release 0.10.0 #377

Merged

hugrbot mentioned this pull request Oct 21, 2024

chore(main): release 0.13.0 #518

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Parse inout annotations in function signatures #316

feat: Parse inout annotations in function signatures #316

mark-koch commented Jul 24, 2024

codecov-commenter commented Jul 29, 2024 •

edited

Loading

mark-koch Jul 30, 2024

acl-cqc left a comment

acl-cqc Jul 30, 2024 •

edited

Loading

acl-cqc Jul 30, 2024

mark-koch Jul 31, 2024 •

edited

Loading

acl-cqc Jul 31, 2024

mark-koch Jul 31, 2024

mark-koch Jul 31, 2024

acl-cqc Jul 30, 2024

mark-koch Jul 31, 2024

acl-cqc Jul 30, 2024

mark-koch Jul 31, 2024

acl-cqc Jul 30, 2024

acl-cqc Jul 30, 2024 •

edited

Loading

acl-cqc left a comment

acl-cqc Aug 1, 2024

acl-cqc Aug 1, 2024

acl-cqc Aug 1, 2024

acl-cqc Aug 1, 2024

mark-koch Aug 9, 2024

acl-cqc Aug 1, 2024

acl-cqc Aug 1, 2024

mark-koch Aug 9, 2024

	# Callable types are constructed using special login in the type parser
	# Callable types are constructed using special logic in the type parser

	"""Helper function to parse a `Callable[[<arguments>], <return types>]` type."""
	"""Helper function to parse a `Callable[[<arguments>], <return type>]` type."""

	"`Callable[[<arguments>], <return types>]`"
	"`Callable[[<arguments>], <return type>]`"

feat: Parse inout annotations in function signatures #316

feat: Parse inout annotations in function signatures #316

Conversation

mark-koch commented Jul 24, 2024

codecov-commenter commented Jul 29, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

acl-cqc left a comment

Choose a reason for hiding this comment

acl-cqc Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mark-koch Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acl-cqc Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

acl-cqc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 29, 2024 •

edited

Loading

acl-cqc Jul 30, 2024 •

edited

Loading

mark-koch Jul 31, 2024 •

edited

Loading

acl-cqc Jul 30, 2024 •

edited

Loading