fix: use unchecked constructor for ProcedureName on Library deserialization

[greenhat]

Describe your changes

Following the conversation with @bitwalker the checked constructor fits only when parsing MASM code. The Miden package contains the export names crafted according to the Wasm CM naming scheme, i.e. namespace:package/interface@version#function.
Discovered in 0xPolygonMiden/compiler#347 when parsing the Miden package file of the basic wallet account code.
The test for the failing case added in #1544 in the Package deserialization test suite at

miden-vm/package/src/tests.rs

Lines 136 to 147 in 9ff342e

	#[test]
	fn basic_wallet_package_deserialization() {
	// Test for the https://github.com/0xPolygonMiden/compiler/issues/347
	// The included Miden package file is built at
	// https://github.com/0xPolygonMiden/compiler/blob/6cd29e17b34c5abef7f6328c33af06f8bf203344/tests/integration/src/rust_masm_tests/rust_sdk.rs#L48-L63
	let bytes = include_bytes!("../tests/data/basic_wallet.masp");
	let package = Package::read_from_bytes(bytes).unwrap();
	dbg!(&package.manifest);
	assert_eq!(package.name, "basic_wallet");
	}

.

Checklist before requesting a review

• Repo forked and branch created from next according to naming convention.
• Commit messages and codestyle follow conventions.
• Relevant issues are linked in the PR description.
• Tests added for new functionality.
• Documentation/comments updated according to changes.

[bobbinth]

Would it make sense to relax procedure naming rules to accomodate WASM CM format rather than making these types of exception? Or maybe we introduce another more relaxed constructor that is not fully "unchecked" but relaxed enough to handle WASM CM naming.

[bitwalker]

Would it make sense to relax procedure naming rules to accomodate WASM CM format rather than making these types of exception? Or maybe we introduce another more relaxed constructor that is not fully "unchecked" but relaxed enough to handle WASM CM naming.

The naming rules are pretty much already there, though bare identifiers in MASM source code are a little more restrictive, you can use quoted identifiers to represent pretty much any possible identifier.

PR looks good, the only change I would also like to make would be to fix the naming of ProcedureName::{new, new_unchecked} and Ident::{new, new_unchecked} (and I think there are also related LibraryPath/LibraryNamespace APIs as well), to make it clear that new_unchecked is what should be used internally for virtually everything. The new variants (and their stricter validation, only exist for the parsing frontend, as MASM syntax requires bare identifiers of various kinds to be validated more strictly, but quoted identifiers do not require validation (and the parser doesn't restrict those today, just bare identifiers).

The whole new/new_unchecked naming thing ended up being misleading IMO, which was a poor choice on my part. This at least fixes the immediate issue, but to avoid future issues of the same nature, we'll want to update those function names and their docs to be clearer about their intended use.

[bobbinth]

PR looks good, the only change I would also like to make would be to fix the naming of ProcedureName::{new, new_unchecked} and Ident::{new, new_unchecked} (and I think there are also related LibraryPath/LibraryNamespace APIs as well), to make it clear that new_unchecked is what should be used internally for virtually everything. The new variants (and their stricter validation, only exist for the parsing frontend, as MASM syntax requires bare identifiers of various kinds to be validated more strictly, but quoted identifiers do not require validation (and the parser doesn't restrict those today, just bare identifiers).

The whole new/new_unchecked naming thing ended up being misleading IMO, which was a poor choice on my part. This at least fixes the immediate issue, but to avoid future issues of the same nature, we'll want to update those function names and their docs to be clearer about their intended use.

Great! I'll merge as is and we can refine naming in follow-up PRs.