Skip to content
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: Update linearity checker to use new diagnostics #601

Merged
merged 8 commits into from
Oct 31, 2024

Conversation

mark-koch
Copy link
Collaborator

Closes #541

@mark-koch mark-koch requested a review from ss2165 October 29, 2024 10:24
@mark-koch mark-koch requested a review from a team as a code owner October 29, 2024 10:24
Comment on lines +6 to +9
| ^ Variable `q` with linear type `qubit` would be consumed
| multiple times when
| evaluating this
| comprehension
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a formatting bug. See #602

node.value, ty = self._synth_expr(node.value)
if ty.linear:
raise GuppyTypeError(f"Value with linear type `{ty}` is not used", node)
node.value, _ = self._synth_expr(node.value)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drive by: This error should be handled during linearity checking

Base automatically changed from diag/parent-format to feat/diagnostics October 29, 2024 10:38
@codecov-commenter
Copy link

codecov-commenter commented Oct 29, 2024

Codecov Report

Attention: Patch coverage is 98.73016% with 4 lines in your changes missing coverage. Please review.

Project coverage is 92.03%. Comparing base (e096c53) to head (9bbfe75).
Report is 1 commits behind head on feat/diagnostics.

Files with missing lines Patch % Lines
guppylang/checker/core.py 77.77% 2 Missing ⚠️
guppylang/checker/linearity_checker.py 98.56% 2 Missing ⚠️
Additional details and impacted files
@@                 Coverage Diff                  @@
##           feat/diagnostics     #601      +/-   ##
====================================================
+ Coverage             91.74%   92.03%   +0.29%     
====================================================
  Files                    61       62       +1     
  Lines                  6624     6832     +208     
====================================================
+ Hits                   6077     6288     +211     
+ Misses                  547      544       -3     

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

Copy link
Member

@ss2165 ss2165 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very nice, just some small queries

"..."
)
place: Place
kind: "UseKind"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: from __future__ import annotations?

if self.is_call_arg:
f = f"Function `{self.func_name}`" if self.func_name else "Function"
return (
f"{f} wants to take ownership of this argument, but you don't own "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a fan of "you" here, maybe be more specific about the calling scope?

#: Ownership of an owned value is transferred by returning it
RETURN = auto()

#: An owned value is renamed or stored in a tuple/list
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is tuple the same as struct here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately not since constructing a struct is a function call, so this counts as consumption

|
16 | @guppy(module)
17 | def foo(qs: list[tuple[bool, qubit]] @owned) -> list[int]:
18 | return [42 for b, q in qs if b if bar(q)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does python allow chained ifs? expected an and

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that works in Python

We don't support and and or in comprehensions yet since we compile them to dataflow...

| ^ Variable `q` with linear type `qubit` cannot be moved ...
|
13 | return [(q, q) for q in qs]
| - since it was already moved here
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

| ...
|
20 | use(q)
| - since it was already consumed here
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"consumed" meaning enters another scope, and "moved" meaning belonging to a new name within same scope?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly!

20 | return s.q, t.q
| ^^^ Field `s.q` with linear type `qubit` cannot be returned ...
|
19 | t = s
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i find it hard to read messages where the same line is repeated, once in the original and then once in the sub. Just a note, don't think there is anything to be done.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the rendering can definitely be improved further. See #608

@mark-koch mark-koch requested a review from ss2165 October 31, 2024 12:19
Copy link
Member

@ss2165 ss2165 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@mark-koch mark-koch merged commit bdc6568 into feat/diagnostics Oct 31, 2024
2 checks passed
@mark-koch mark-koch deleted the diag/linearity branch October 31, 2024 13:08
github-merge-queue bot pushed a commit that referenced this pull request Nov 11, 2024
This is the development branch for our new diagnostics infrastructure.
Tracked by #535.

*  #548
* #551
* #552 
* #553
* #586
* #588
* #587
* #589 
* #590
* #600
* #601
* #604 
* #605 
* #606
github-merge-queue bot pushed a commit that referenced this pull request Nov 12, 2024
🤖 I have created a release *beep* *boop*
---


## [0.13.0](v0.12.2...v0.13.0)
(2024-11-12)


### ⚠ BREAKING CHANGES

* `prelude` module renamed to `std`

### Features

* add `qubit` discard/measure methods
([#580](#580))
([242fa44](242fa44))
* Add `SizedIter` wrapper type
([#611](#611))
([2e9da6b](2e9da6b))
* conventional results post processing
([#593](#593))
([db96224](db96224))
* Improve compiler diagnostics
([#547](#547))
([90d465d](90d465d)),
closes [#551](#551)
[#553](#553)
[#586](#586)
[#588](#588)
[#587](#587)
[#590](#590)
[#600](#600)
[#601](#601)
[#606](#606)
* restrict result tag sizes to 256 bytes
([#596](#596))
([4e8e00f](4e8e00f)),
closes [#595](#595)


### Bug Fixes

* Mock guppy decorator during sphinx builds
([#622](#622))
([1cccc04](1cccc04))


### Documentation

* Add DEVELOPMENT.md
([#584](#584))
([1d29d39](1d29d39))
* Fix docs build ([#639](#639))
([bd6011c](bd6011c))


### Miscellaneous Chores

* Manually set last release commit
([#643](#643))
([b2d569b](b2d569b))


### Code Refactoring

* rename prelude to std
([#642](#642))
([1a68e8e](1a68e8e))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

---------

Co-authored-by: Agustín Borgna <agustin.borgna@quantinuum.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants