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

Minor refactoring of the Library struct #1430

Closed
bobbinth opened this issue Aug 7, 2024 · 2 comments
Closed

Minor refactoring of the Library struct #1430

bobbinth opened this issue Aug 7, 2024 · 2 comments
Assignees
Labels
assembly Related to Miden assembly
Milestone

Comments

@bobbinth
Copy link
Contributor

bobbinth commented Aug 7, 2024

Current Library struct looks like so:

pub struct Library {
    digest: RpoDigest,
    exports: BTreeMap<QualifiedProcedureName, Export>,
    mast_forest: MastForest,
}

enum Export {
    Local(MastNodeId),
    External(RpoDigest),
}

There are a couple of things we may want to change here:

  1. We are missing some metadata from library definition. One example of this is version but there are also other potential things we may want to track about procedures and the library itself. Regarding version, we should decide if we actually need it, and if not, remove the Version struct from the codebase.
  2. We can also consider removing Export enum because information about whether a given procedure is local or re-exported can be inferred from the underlying mast_forest.
@bobbinth bobbinth added the assembly Related to Miden assembly label Aug 7, 2024
@bobbinth bobbinth added this to the v0.11.0 milestone Aug 7, 2024
@bitwalker
Copy link
Contributor

IMO version belongs to a hypothetical Package struct, the "version" of a Library as I see it, is the content hash returned by Library::digest.

I see Library as basically a thin wrapper around the raw MastForest, providing the core information needed to understand its contents. All the higher-level user-facing metadata should belong to Package, which would be named, have a semantic version, and contain optional metadata like debugging metadata, WIT definitions (or whatever we end up with there), and other elements that aren't really needed once the underlying Library is loaded into the Host.

So, externally, most users are working with Package; while internally in miden-processor and miden-core, we're mostly concerned with Library or MastForest. To the extent that Package is present at all, it might be in the miden-processor, so that Host can be "package-aware".

As for removing Export, I think it's worth checking that we can rely on looking at the underlying MastForest, however IIRC, part of the problem is that when two exports refer to the same MAST node, we're only taking the non-External node, which means when you go to look up the MAST node, for all intents and purposes it will look like that node is a definition and not a re-export. We do this because we can't store multiple nodes under the same digest in the forest. I haven't looked into this in detail though, to see if we can address this differently. Export was essentially the method I landed on to ensure that we didn't accidentally replace "real" definitions with an External node that would then never be resolvable (since node corresponding to the actual procedure definition was replaced).

@bobbinth
Copy link
Contributor Author

bobbinth commented Nov 4, 2024

Closed by #1465 (the second point was addressed there and we decided to have metadata at the package level).

@bobbinth bobbinth closed this as completed Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assembly Related to Miden assembly
Projects
None yet
Development

No branches or pull requests

2 participants