Fix duplicate procedure name issue. #1256
Conversation
The sentence describing the column $k_0$ had its order reversed, and the table headers were incorrectly named as $x_i$ and $y_i$ instead of $a_i$ and $b_i$ as described in the introduction.
Introduce `std::utils` assembly module
… info This commit changes the `import_info` fields of ProgramAst and ModuleAst to remove the `Option` wrapper, since it does not appear to serve a specific purpose, and both complicates access to the imports, and requires redundant code in a few places to access common information from the imports. It also required fallible checks in a few places where empty module import info would be suitable as a fallback. After this change, one can access the ModuleImports struct directly on both ProgramAst and ModuleAst via the `import_info` function, which returns a reference to the field. Redundant functions in both structs were removed if they are already provided by ModuleImports.
docs(mdbook): minor change in the bitwise chiplet
Expand capabilities of the `debug` instruction
add .editorconfig
Minor doc fixes
Minor doc fix LogUp
…dler Add `emit` instruction and host event handler
Chore: pre-commit sync
* ContextId type * propagate MemoryContextId * fix tests * MemoryContextId -> ContextId * move definition * Remove derive-more dependency * fix repl * fmt * docstring * import newline * Remove `Sub` from `ContextId` * blank line * section name change * blank line * use `ContextId::root()` * fmt * memory context -> execution context
Fumuran
force-pushed
the
andrew-duplicate-proc-fix
branch
from
08a7395 to
9de5447
Compare
| @@ -3,7 +3,7 @@ use super::{ | |||
| NamedProcedure, Procedure, ProcedureCache, ProcedureId, ProcedureName, RpoDigest, ToString, | |||
| Vec, | |||
| }; | |||
| use crate::ast::{ModuleAst, ProgramAst}; | |||
| use crate::ast::{event, Level, ModuleAst, ProgramAst}; | |||
There was a problem hiding this comment.
nit: it is odd to import the tracing::event macro from the ast module. Maybe import it directly?
| @@ -428,7 +428,7 @@ impl ModuleContext { | |||
| if self.compiled_procs.iter().any(|p| p.name() == name) | |||
| || self.proc_stack.iter().any(|p| &p.name == name) | |||
| { | |||
| return Err(AssemblyError::duplicate_proc_name(name, &self.path)); | |||
| event!(Level::WARN, "duplicate proc name '{}' in module {}", name, &self.path); | |||
There was a problem hiding this comment.
What would happen if we have two notes with the same procedure name, but different implementations? If I understood correctly, this would favor the first implementation, and use the same code for both notes. If that is the case, I think we need some different approach, perhaps compile both, and check that the MASTs are the same?
There was a problem hiding this comment.
You're right. I thought that compilation of the notes will be performed in different ModuleContexts, but it's not, and that's the core issue. I dived into implementation of the compilation in the VM, but forgot to check how actually notes are compiled.
I think we will need to make some changes in both miden-base and miden-vm. Currently we are compiling each note (which contains ProgramAst and not ModuleAst, which is important) not only in one AssemblyContext, but also in one ModuleContext which is being created within the AssemblyContext creation. So, IMHO, to make it work we need to create a new ModuleContext for each note, which is currently impossible: we create a new ModuleContext of we create an AssemblyContext for program or if we start to compile a new module.
So in other words we need a new function in the Assembler, which will work similar to the compile_in_context(), but will create a new ModuleContext inside the AssemblyContext and compile the provided program using it.
After that in the miden-base inside the compile_notes function we will need to change the compile_in_context function to the newly created one.
There was a problem hiding this comment.
Shouldn't this become a non-issue when we switch to using MAST as the compilation artifact? In other words, notes will be compiled separately to MAST ahead-of-time, rather than from MASM on-demand. Then a transaction script program (also in MAST form), would either reference the notes by MAST root, or would inline the MAST for the note into itself. To be clear, I know that isn't the case right now, but I'm referring to once we make the changes described in #1226 and #1217 and switch away from using MASM as the artifact.
There was a problem hiding this comment.
See also 0xPolygonMiden/compiler#132, which is a draft spec for those changes as well as the binary format for the MAST. The draft is being worked on there temporarily, it will be moved to this repo once I've worked through the first iteration of feedback.
There was a problem hiding this comment.
I think it should solve this issue indeed!
There was a problem hiding this comment.
Should we punt on solving this problem temporarily until that's done then? I think we're basically at the point where we can start doing the work to make that happen, we've just been taking our time with working through some of the details - but the PR I linked is basically the result of gathering the pieces of all those discussions together into a draft spec, and once we're happy with it, we can start implementing basically right away.
Bobbin recently mentioned in one of our conversations that you would probably be doing the implementation @Overcastan (if not both of us), though that's obviously not set in stone, but if you have any thoughts on the draft spec before I open it as a proper proposal here in the miden-vm repo, feel free to jump in as a reviewer.
In the meantime, it seems like it might be best to make the case of duplicate procedure names with differing MAST roots an error, to avoid accidentally treating them as equivalent. Then, once we implement the MAST changes, the practical issue with note scripts goes away.
There was a problem hiding this comment.
Yes - I think it makes sense to hold off on solving this as it will become a non-issue once we use MAST-based format to represent note scripts and account code. We should start working more actively on this starting next week, and hopefully, within 2 - 3 weeks we can be in a place where is issue becomes irrelevant.
There was a problem hiding this comment.
I think this issue is basically in the same boat as #1211.
|
@Overcastan are you still working on this? |
|
This may have been fixed by #1277. cc @bitwalker. |
|
Closing as superseded by #1277. |
Currently if we try to compile two programs in one context, which contain procedures with the same names, this will lead to a
DuplicateProcNameerror. However we need to be able to do so while compiling transaction with multiple notes.To achieve this, error was replaced with a warning log message.
This change should not lead to any issues with procedures parsing, compilation and execution.
Related issue: 0xPolygonMiden/miden-base#443