search

move-language / move

Apache License 2.0
2.26k stars 688 forks source link

[compiler] Enforce a canonical order for module handles #904

Closed amnn closed 1 year ago

amnn commented 1 year ago

Add a pass to the compiler, immediately after conversion from IR to bytecode, to re-order module handles to guarantee a stable ordering.

Test Plan

New tests for canonical order:

$ cargo nextest -- canonicalize_
amnn commented 1 year ago

(Just tested a bump on sui and we will also need to canonicalize the identifier pool).

amnn commented 1 year ago

Alright, we're good to go, this passes the regression test in https://github.com/MystenLabs/sui/pull/8248 and the repro instructions for building packages in https://github.com/capsule-craft/capsules from @PaulFidika.

amnn commented 1 year ago

Thanks both! Recording a summary of a conversation that I had with @tnowacki about the scenario where a new field gets added:

Yep, this is a real issue, and the best solution I can come up with is to adopt a visitor pattern that's used here and everywhere else that traverses bytecode indices in this way, so that folks only have to update one place. Unfortunately, it seems like this kind of traversal isn't super common, so it doesn't buy us much to use a visitor pattern just here.

That being said, whenever I made a mistake while building this pass, I got a test failure in the Move testsuite, so we do have pretty good test coverage (usually it's a verifier test that fails which is reassuring, albeit with an invariant violation).