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

Move Noop, MakeTuple, UnpackTuple, Tag to prelude extension #816

Closed
ss2165 opened this issue Jan 22, 2024 · 2 comments · Fixed by #1475
Closed

Move Noop, MakeTuple, UnpackTuple, Tag to prelude extension #816

ss2165 opened this issue Jan 22, 2024 · 2 comments · Fixed by #1475
Assignees
Labels
breaking-change Changes that break semver

Comments

@ss2165
Copy link
Member

ss2165 commented Jan 22, 2024

Requires #787

@ss2165 ss2165 self-assigned this Jan 22, 2024
@aborgna-q aborgna-q added the breaking-change Changes that break semver label Mar 14, 2024
@aborgna-q aborgna-q added this to the v0.3.0 milestone Mar 20, 2024
@ss2165 ss2165 removed their assignment Apr 10, 2024
@ss2165 ss2165 removed this from the v0.3.0 milestone Apr 12, 2024
@acl-cqc
Copy link
Contributor

acl-cqc commented Apr 15, 2024

I think this can be done without #787 by defining custom binary compute_signature functions. This just moves code around (i.e. the code that currently compute signature of these ops gets moved into an extension), with I suspect a substantial increase in boilerplate. OTOH it makes the API more uniform.

As a side benefit it also brings fields such as

pub ty: Type,
and
pub tys: TypeRow,
into scope for this check:
// Check TypeArgs are valid, and if we can, fit the declared TypeParams
match resolved {
CustomOp::Extension(exten) => exten
.def()
.validate_args(exten.args(), self.extension_registry, var_decls)
.map_err(|cause| ValidationError::SignatureError { node, cause })?,
....which "feels right", but (for these ops) doesn't catch anything that we wouldn't anyway in
self.validate_port_kind(&port_kind, var_decls)
.map_err(|cause| ValidationError::SignatureError { node, cause })?;
.

@aborgna-q aborgna-q closed this as not planned Won't fix, can't repro, duplicate, stale May 1, 2024
@acl-cqc
Copy link
Contributor

acl-cqc commented May 22, 2024

We've done #787, so we can do this now without any "custom binary" code for compute_signature. I think we should.

Lift similarly (although Lift has limited lifespan, should disappear following #919)

CallIndirect similarly (this should go in the Tierkreis extension, not prelude).

@acl-cqc acl-cqc reopened this May 22, 2024
@ss2165 ss2165 self-assigned this Aug 22, 2024
@ss2165 ss2165 added this to the hugr-rs 0.12 / hugr-py 0.8 milestone Aug 28, 2024
aborgna-q pushed a commit that referenced this issue Aug 29, 2024
…1475)

Much of the noise is adding PRELUDE to extension inference tests. Add a
convenience `with_prelude()` method to Signature.

`Tag` left as a core operation as it is paramaterized by a variable
number of type rows.

Also drive-by get rid of custom signature use for print.

Closes #816
Closes #1373

BREAKING CHANGE: Move `Lift`, `MakeTuple`, `UnpackTuple` and `Lift` from
core operations to prelude. Rename `ops::leaf` module to `ops::sum`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Changes that break semver
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants