Skip to content

Conversation

strub
Copy link
Member

@strub strub commented Jan 20, 2025

No description provided.

@strub strub self-assigned this Jan 20, 2025
@strub strub changed the title Reword stlib clones. Rework stdlib clones. Jan 20, 2025
@strub strub force-pushed the hidden-theory-items branch from fdd7813 to fa18670 Compare January 20, 2025 12:33
@strub strub force-pushed the poly-better-clones branch from eeb3470 to 9f03146 Compare January 20, 2025 12:36
@strub strub requested a review from fdupress January 20, 2025 12:37
@strub strub force-pushed the poly-better-clones branch from 9f03146 to 0792c02 Compare January 20, 2025 14:06
@strub strub force-pushed the hidden-theory-items branch 2 times, most recently from 7066af3 to 6dfe829 Compare February 6, 2025 17:26
@vbgl
Copy link
Contributor

vbgl commented Feb 19, 2025

I can confirm that in this branch, it is possible to clone and use the PolyReduce.PolyReduce. Any chance to get it merged in the main branch?

Note that what I expected to be able to write was:

clone import PolyReduce.PolyReduce as P with
type coeff <= int,
theory Coeff <= Ring.IntID,
op n <- 256.

But that failed with

unknown symbol: Top.Ring.IntID.t

Workaround was to define my own IntID theory using

clone include Ring.IDomain with
  type t <= int,

when Ring.ec reads:

clone include IDomain with
  type t <- int,

@fdupress
Copy link
Member

I didn't review this because it was still draft, but happy to do so after it's rebased.

As a side note, given what we now know about how cloning works (and doesn't), perhaps it's time to emit a warning when the user clones a theory that does not contain modules and uses <- to instantiate things? (It would certainly help review this for thoroughness of the replacement.)

We could also (later) have a [final] option on clones, that deactivates the warning but prevents further instantiation—with a more meaningful error message than that currently emitted.

@strub strub force-pushed the hidden-theory-items branch 3 times, most recently from 67dc10a to 3673b82 Compare August 23, 2025 09:43
@strub strub force-pushed the poly-better-clones branch from 0792c02 to fb974b7 Compare August 23, 2025 11:57
@strub strub marked this pull request as ready for review August 23, 2025 11:57
@strub
Copy link
Member Author

strub commented Aug 23, 2025

I can confirm that in this branch, it is possible to clone and use the PolyReduce.PolyReduce. Any chance to get it merged in the main branch?

Note that what I expected to be able to write was:

clone import PolyReduce.PolyReduce as P with
type coeff <= int,
theory Coeff <= Ring.IntID,
op n <- 256.

But that failed with

unknown symbol: Top.Ring.IntID.t

Workaround was to define my own IntID theory using

clone include Ring.IDomain with
  type t <= int,

when Ring.ec reads:

clone include IDomain with
  type t <- int,

IMO, that's a mistake a IntID to remove the t type definition.

Base automatically changed from hidden-theory-items to main August 23, 2025 12:23
@strub strub force-pushed the poly-better-clones branch from fb974b7 to 81c35a2 Compare August 23, 2025 12:25
@strub strub force-pushed the poly-better-clones branch from 81c35a2 to 0a6170b Compare September 3, 2025 06:51
@strub
Copy link
Member Author

strub commented Sep 6, 2025

Merging. Not perfect but will continue working in a different PR.

@strub strub merged commit 87c0a63 into main Sep 6, 2025
15 checks passed
@strub strub deleted the poly-better-clones branch September 6, 2025 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants