Remove uniqueness invariant from MastForestBuilder
#1473
Conversation
This reverts commit 72a7093.
|
@bobbinth ready for re-review. Only thing not done yet is adding the warning when multiple procedures have the same MAST root (depending on if/how we want to do it) |
| let mast_root = { | ||
| let proc_body_id = | ||
| self.resolve_target(InvokeKind::ProcRef, callee, proc_ctx, mast_forest_builder)?; | ||
| mast_forest_builder.get_mast_node(proc_body_id).unwrap().digest() |
There was a problem hiding this comment.
nit: could we add a comment here explaining why unwrap() is OK?
There was a problem hiding this comment.
I added a comment but honestly the unwrap() is true by construction: MastNodeId is only created by a MastForest, so all MastNodeIds are associated with a MastNode in a MastForest (unless you use it with the wrong mast forest or whatnot, but that's an internal error where I believe a panic! is appropriate).
We used to implement Index on MastForestBuilder for this purpose, and I'm not sure why we removed it. We had added a MastForest::get_mast_node() to be used in deserialization code, but it was not intended to be used anywhere else.
There was a problem hiding this comment.
Yeah I was surprised we removed Index as well, since I thought it was more-common-than-not that we were accessing node data from the forest, by id, with an id that we know a priori is in the forest, such as this case. Might be worth adding it back if the thinking was that it wasn't being used.
There was a problem hiding this comment.
Looks good! Thank you! I left one small comment inline.
Also, would be good to get @bitwalker to look through this as well.
| // insert external node into the MAST forest for this procedure; if a procedure | ||
| // with the same MAST rood had been previously added to the builder, this will | ||
| // with the same MAST root had been previously added to the builder, this will | ||
| // have no effect |
There was a problem hiding this comment.
I think this comment should go above line 529 - no?
There was a problem hiding this comment.
I actually removed this comment (in #1466) since it's now redundant with the Assembler::resolve_target() docs
| /// [`MastNode`]s have equal [`MastNodeId`]s. | ||
| fn ensure_node(&mut self, node: MastNode) -> Result<MastNodeId, AssemblyError> { | ||
| let node_digest = node.digest(); | ||
| /// Note adding the same [`MastNode`] twice will result in two different [`MastNodeId`]s being |
There was a problem hiding this comment.
This isn't true if the MAST nodes are exactly identical right? i.e. same hash, same decorators? I think the only condition where duplicate nodes are required/expected, are when the MAST root is the same, but the decorators differ, otherwise we can collapse duplicates into a single node as before. That way we still largely avoid duplicating a bunch of identical nodes, and only duplicate on an as-needed basis.
There was a problem hiding this comment.
Oops, this is what the implementation does, but forgot to fix the docstring.
Fixed in #1466
| /// calls to library procedures will be compiled down to a [`vm_core::mast::ExternalNode`] (i.e. | ||
| /// a reference to the procedure's MAST root). This means that when executing a program compiled | ||
| /// against a library, the processor will not be able to differentiate procedures with the same | ||
| /// MAST root but different decorators. Hence, it is not recommended to export two procedures |
There was a problem hiding this comment.
As an aside, I think this is unlikely to ever be an explicit choice, except in the specific case of APIs being renamed/moved between modules, where during a deprecation period, the same procedure is exported under both its old and new paths. This wouldn't have any practical effect on debugging and such, since in both cases the procedure hasn't changed.
It'll be interesting to see how frequently this actually occurs in practice. I do think it would be a good idea for us to have a pedantic warning mode, or some other tooling that would let us analyze things like this on an opt-in basis, but I suspect that in general we're unlikely to see scenarios in which this causes confusion except for in large programs.
Just to be clear, I think this is a good note to have, but I would maybe suggest illustrating the circumstances in which this can occur, and the expected worst-case scenarios. Otherwise it sounds a bit dire, but also isn't clear to the reader whether or not this is something they need to be actively worried about or not, or how to even vet/prevent this from happening.
There was a problem hiding this comment.
I added to the comment, and created #1479
| // ================================================================================================ | ||
|
|
||
| /// Generic container for a choice between two types. | ||
| #[derive(Debug, Clone, Copy)] |
There was a problem hiding this comment.
It doesn't look like this is used, so I would suggest removing it for now. I would pull in the either crate in the future if we determine having an Either type would be useful, as it provides a lot of conveniences you'd expect on a monadic type like this.
There was a problem hiding this comment.
Removed.
Note: I defined it instead of pulling in the either crate, since @bobbinth typically pushes for us to limit the amount of dependencies we bring in. We can decide on whether we implement it ourselves or pull in the either crate next time we need it.
We now use
MastNodeIdinstead of MAST root as an identifier for procedures.This is because 2 procedures can have identical MAST roots, but different decorators (resulting in different
MastNodeIds).