Skip to content

fix(kmir.md): handle projected operandMove callees#983

Merged
automergerpr-permission-manager[bot] merged 9 commits intomasterfrom
jh/spl-multisig-master-repro
Mar 11, 2026
Merged

fix(kmir.md): handle projected operandMove callees#983
automergerpr-permission-manager[bot] merged 9 commits intomasterfrom
jh/spl-multisig-master-repro

Conversation

@Stevengre
Copy link
Contributor

Summary

Fix call dispatch for projected operandMove callees.
When the callee is reached through projectionElemDeref, kmir was not computing the effective callee correctly, which left the proof stuck. This PR adds a regression for that case and updates call dispatch to use the projected place type, allowing the repro to run through to #EndProgram.

Testing

Validated as a red -> partial green -> green series with:

make test-integration TEST_ARGS="-k spl-multisig-iter-eq-copied-next-fail"

Each commit in the final branch passes that scoped command on the remote validation workspace:

  • 86074df5 test(integration): add spl-multisig iter-eq copied next repro
  • f04e1656 fix(call): match projected operandMove callees
  • ceb4a2a0 fix(call): compute projected callee types through to EndProgram

@Stevengre Stevengre self-assigned this Mar 10, 2026
@Stevengre Stevengre changed the title Fix call dispatch for projected operandMove callees fix(kmir.md): handle projected operandMove callees Mar 10, 2026
@Stevengre Stevengre requested review from dkcumming and mariaKt March 10, 2026 09:41
@Stevengre Stevengre marked this pull request as ready for review March 10, 2026 09:41
Copy link
Collaborator

@dkcumming dkcumming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that this works to solve the problem. But there are a bunch of things that I feel look different to the typical structure I would see @jberthold do to make sure the backend is happy. I feel we typically want to make functions [total] and otherwise have [preserves-definedness]. Maybe we should be having index checks too for I in range of LOCALS, not sure though. I will message Jost in slack for his opinion

@Stevengre
Copy link
Contributor Author

Good point! I like total functions too. But this case shouldn't be a problem because we have the same check in the previous rule which is the one entrypoint for the function. Even though, let me fix it with total. It's not hard to refactor this.

@Stevengre Stevengre requested a review from dkcumming March 11, 2026 01:16
Copy link
Collaborator

@dkcumming dkcumming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

andBool isTy(getTyOf(tyOfLocal({LOCALS[I]}:>TypedLocal), PROJS))
[preserves-definedness]

rule #projectedCallTy(_, _, _) => ty(-1) [owise]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the function could return TyUnknown for undesired cases, and the use site could catch that?
Then this would be TyUnknown instead of ty(-1) (which, by convention, would mean main if it exists), and the rule above would not need a side condition isTy(...) or cast {...}:>Ty.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I forgot about TyUnknown! And I forgot -1 was main

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Make a commit for that.

andBool isTy(getTyOf(tyOfLocal({LOCALS[I]}:>TypedLocal), PROJS))
[preserves-definedness] // valid local indexing checked, projected call target must resolve to a Ty

syntax Ty ::= #projectedCallTy(Int, ProjectionElems, List) [function, total]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you were asking on slack about this:
I tend to prefer total functions with optional results over partial ones. For a partial function used in an important rewrite rule, whenever there is uncertainty about the undefined points the old backend would kick in and start inventing path conditions. Therefore I think this is the right choice, except I would make the function return a MaybeTy and catch the TyUnknown cases at the use site.

@automergerpr-permission-manager automergerpr-permission-manager bot merged commit 9cb1679 into master Mar 11, 2026
7 checks passed
@automergerpr-permission-manager automergerpr-permission-manager bot deleted the jh/spl-multisig-master-repro branch March 11, 2026 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants