Could we add a safety comment explaining why we expect prefix_component to always be valid?

More generally, why do we need to do component validation on iteration rather than on Path construction? Seems like it would be easier to reason about some of these things if we could always assume that a Path is well-formed.

Even if we assert in Path::new (which just losslessly converts from str to Path), we'd need two implementations of the components iterator: one used when validating/constructing from a string, and one that assumes the components are valid and just asserts on that assumption being violated. Most of the implementation could actually be shared, we'd probably just have the "assume valid" Iterator wrap the current impl and assert on Err. That might be better than the current setup.

In any case, I couldn't make those sorts of changes here because they'd be breaking changes, and I wanted to keep this as non-invasive as possible.

It may be worth adding a comment noting that this compares the prefix only against the first component (excluding the root marker). It took me a bit of time to understand this.

Good point - we document this for join now, but didn't reflect those semantics in the docs here.

Similar comment as before re adding a note about why we are guaranteed to have a valid prefix_component here.

We're not guaranteed to - but it is part of the inherent contract of these APIs that paths are valid once constructed. If we make the changes discussed in the other comment, then we can actually enforce that, but that sort of change will have to happen in next

Was Line 269 supposed to be deleted? The content seems to be the same as line 270, but it is numbered as 1 (same as line 268).

Whoops, no, it's just incorrectly numbered

The old resolve function had a branch that rebound gid.module to kernel_index when context.in_syscall() was true. That guard is gone here, so syscall targets can now bind to non-kernel items.

For example, I think that if a user module defines foo and the kernel also has foo, syscall.foo might now resolve to the user's foo instead of the kernel's foo.

Good point, this should be enforced by resolve_invoke_target, seems like we don't have a regression test for that either, so I'll add one

The old find function tracked visited full paths like [a, b, a] to detect cycles. This only tracks import names like a, so multi-alias cycles can slip through.

IIUC, if module A imports b and module B imports a, then resolving a::b::a::x would keep expanding instead of raising a recursive alias error. The ignored_imports set catches use lib::lib style cycles, but not cycles with distinct names.

I'll add a regression test for this and make sure we're still properly handling cycles like that - but I don't believe the issue you've described is actually an infinite expansion (at least not without a reproducer), though the behavior would certainly depend on where one is resolving a path like a::b::a::x from. But let's assume for a second that it is from b, which looks like so:

pub use a

pub proc foo
    exec.a::b::a::x
end

And a looks like:

pub use b

pub proc x
    push.1
end

So we have a cyclic dependency between a and b, but one that should be resolvable. Resolving exec.a::b::a::x in b would proceed as follows:

• The path is relative, so we split off a to resolve as an import, which expands to just a. We add a to ignored_imports and proceed to resolve the now partially-expanded path a::b::a::x
• The path is relative, so we split off a to resolve as an import, but this time it is ignored, instead we attempt to expand the path as absolute, i.e. ::a::b::a::x
• The path is absolute, so we search for a module with the longest matching prefix, which is a, so we attempt to resolve b::a::x relative to a
• The path is relative, so we split off b to resolve as an import, which expands to just b, which is added to ignored_imports, and we proceed to resolve the now partially-expanded path b::a::x
• The path is relative, and again we ignore the import b, and convert the path to absolute form and attempt to resolve as ::b::a::x
• The longest matching prefix of ::b::a::x is b, so we attempt to resolve a::x relative to b
• We expand the a import to a, ignore it, and proceed to resolve ::a::x
• The longest matching prefix of ::a::x is a, so we attempt to resolve x relative to a
• The relative path x is a symbol, so we resolve it to a local definition in a

I did find a particularly pathological case though which does produce an issue - but I'm not sure if it would have succeeded previously either, in any case, here's the test:

#[test]
fn test_linking_recursive_expansion() -> TestResult {
    let context = TestContext::default();

    let a_lib = context.parse_module_with_path(
        "a",
        source_file!(
            &context,
            r#"
        pub use b::a
        pub proc x
            push.1
        end
        "#
        ),
    )?;

    let b_lib = context.parse_module_with_path(
        "b",
        source_file!(
            &context,
            r#"
        pub use a::a
        pub proc foo
            exec.a::x
        end
        "#
        ),
    )?;

    let assembler = Assembler::new(context.source_manager());
    let _ = assembler.assemble_library([a_lib, b_lib])?;

    Ok(())
}

Nit: This only checks the first component against the entire prefix string, so path("foo::bar").starts_with("foo") returns true but path("foo::bar").starts_with("foo::bar") returns false.

The old behavior used string prefix matching, so path("foo::bar").starts_with("foo") would return true as well, but the behavior was different for multi-component prefixes. This might be quite fine if the API is only used with single-component prefixes, but it's a subtle change.

Correct - however in our uses we only ever intend to match the leading component when using a string, and the subtleties around matching portions of a path with strings caused bugs when dealing with quoted identifiers.

This change, while technically breaking, actually guarantees the correct semantics for our intended uses of these functions. Essentially, like join, we want to ensure that when component structure matters, one has to be explicit about constructing a Path for multi-component matching/joining, in order to guarantee consistent behavior when dealing with paths that may have quoted components.

Question: what's the motivation for moving this file?

We had the miden-assembly crate depending on miden-processor and miden-core-lib (both of which depend on miden-assembly) just for two integration tests (and also used by the doctests in the miden-assembly README).

The problem is that this meant that changing anything in the assembler and then running an assembler test, would first need to build both of those crates (and of particular note, miden-core-lib). If there was an issue in the assembler that you were trying to work out, and that issue affected any of the MASM in miden-core-lib, you would be unable to run any of the miden-assembly tests without getting miden-core-lib to build successfully first. That was obviously a problem for me when tinkering with anything involving the linker/symbol resolution.

Now, the two tests that used the processor live in the processor test suite, and this cyclic dependency is now gone. The only downside is that a couple of the doctests in the miden-assembly readme can no longer reference miden-core-lib, but the need for that is going away soon anyway.

I don't mind this change, but what's the motivation for making it?

See my other comment above.

More generally though, I think the miden-assembly README should probably not be referencing stuff from miden-core-lib just for the sake of an example. We can illustate the APIs just fine without doing that, and it saves us the need to have a cyclic dependency between miden-assembly and miden-core-lib which can be a real pain as mentioned in my other comment.

Lastly, referencing Library via Rust crates like that is going away anyway, so all of these examples will need to be rewritten regardless.

I left the comment in though just to preserve the usage that was documented there, it just isn't statically checked anymore.