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

Remove brackets from virtual filename Display impl #2

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

greenhat
Copy link

Discovered in 0xPolygonMiden/compiler#220

This PR removes the brackets enclosing in Display impl for FileName to be in sync with as_str implementation and to avoid generating invalid MASM module ids (brackets are not allowed).

@greenhat greenhat requested a review from bitwalker June 24, 2024 12:08
Copy link
Contributor

@bitwalker bitwalker left a comment

Choose a reason for hiding this comment

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

While I understand the underlying motivation for this PR, I do wonder if this is not actually the right place to address it. Specifically, one of the goals behind displaying names this way was to visually represent that a given name was "virtualized", i.e. it isn't a real filename - otherwise, it would not be possible to distinguish between a real file named foo, and a virtual file named foo when displayed. This is especially useful in debug output, or in REPLs, where being able to tell whether the source of a particular bit of code came from a real file path or a virtual one is of particular usefulness.

The fact that we are relying on file names of Rust compiler outputs to derive module names in Miden Assembly is basically a workaround in and of itself, that will no longer be needed after 0xPolygonMiden/miden-vm#1363 is merged. Once that is merged, we can switch to using fully-qualified identifiers without a module at all, e.g. exec.::"miden:tx-kernel/get-inputs". Note two things about that example:

  1. We are telling the assembler that the procedure path is absolute/fully-qualified using the :: prefix
  2. We are using "raw" identifier syntax for the procedure name, so the usual identifier restrictions are disabled (aside from disallowing non-printable characters, or control characters)

Now, currently this is only useful when compiling a Wasm module to MASM without linking to another Wasm-compiled library, since we're effectively removing the need to ever name the MASM module explicitly at all (or more precisely, we can name it whatever we want). However, one last change I plan to make will allow specifying absolute paths using Wasm Component Model syntax, i.e. ::miden:tx-kernel/get-inputs would be equivalent to a LibraryPath where the LibraryNamespace is User("miden"), with path components equivalent to tx-kernel::get-inputs. This will let us avoid the need for raw identifiers in most cases, and also set the stage for packaging, so that referencing items in other packages can use the universal form, rather than having to translate to some MASM-specific identifier. That will come soon, but until packages are a thing, we don't need that functionality just yet.

Sorry this turned into a long spiel, but wanted to explain why I don't think we should merge this, and instead solve the problem in midenc using a more targeted fix. This crate is used by other projects than just midenc, so I'd prefer to keep it as general as possible, unless an issue affects all downstream users more or less equally.

@greenhat
Copy link
Author

I agree, but I was quite surprised to see to_string vs. as_str returning different values. Would Debug impl be a better fit for "bracketed" virtual file name?

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.

2 participants