Uh oh!
There was an error while loading. Please reload this page.
Reject a method whose self is not the first parameter - #397
Merged
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes#377.
What was wrong
A struct method could declare its
self/mut selfreceiver at any parameter position. The parser accepted it, the type checker accepted it, and the two halves of codegen then disagreed about the ABI:compiler.rs,visit_function_definition_body);lower_instance_method_call, plus the two sret call paths).So for the issue's program the receiver pointer bound to
deltaand the literal2bound toself. Nothing in between noticed:has_selfwas computed withargs.iter().any(..)— position-blind, and contradicting its own documented contract — whileparam_typesfiltered the receiver out of whatever index it sat at, so arity and per-argument checks passed.Three observed outcomes, all at exit 0:
fn plus(delta: i32, self)65520(a stack pointer) instead of42fn get(k: i32, self), called with1000000wasm trap: out of bounds memory access— the mis-bound integer is read as an addressvalue: i64,fn plus(delta: i64, self)expected i32, found i64)The emitted Rocq
.vfaithfully encoded the same broken ABI, so a proof built over it would have been a proof about the mis-lowered program.Scope
The defect surface is struct methods only (top-level and spec-inner, which register through the same site). Standalone
fnandexternal fnalready rejected aselfparameter at any position, andfn m(self, self)was already fatal. Confirmed by grep that no.inffixture or inline test source in the repo declared a non-leadingself, so the new rejection breaks nothing in the corpus.What changes
Frontend rejection —
core/type-checker/. NewTypeCheckError::SelfReferenceNotFirstParameter, emitted from the single struct-method registration site by one positional scan, exactly once per malformed method and anchored at the receiver to move:has_selfstays deliberately position-blind (.any(..), now folded into the sameposition(..)scan): a misplaced receiver still classifies the function as an instance method for the rest of checking, so the declaration-site error is not joined by anAssociatedFunctionCalledAsMethodat every call site of a method that is already rejected.MethodInfo's doc block now states the invariant it actually has — true for every program that passes type checking, with thecheck_with_diagnosticsrecovery path named explicitly as the exception.Defense in depth —
core/wasm-codegen/. A hardassert_eq!(local_idx, u32::from(is_sret))in theArgKind::SelfRefparameter arm, in the house style of the two sibling "the type-checker should have rejected …" asserts beside it. This covers the library route (type_check_with_diagnostics→codegen) that bypasses the fatal type-check boundary, and it is ordered after the duplicate-name check so a repeated receiver reports the duplicate rather than the misplacement. The three receiver-push sites now each carry a comment naming the invariant they depend on.Testing
19 new tests.
tests/src/type_checker/self_parameter_position.rs(16):mut selfbetween two parameters (span covers the wholemut self), receiver in the fourth slot, i64-width shape, struct-returning (sret) method, spec-inner struct, and a multi-file case asserting the diagnostic is attributed to the importing file. Two of these asserterrors.len() == 1to pin the no-cascade property.fn m(self),fn m(self, x),fn m(mut self, x), an associated function with no receiver, an sret method, and the issue's own program withselfmoved to the front returning42.fn twice(self, self)still yields the duplicate-registration diagnostic and not the new one; standalone andexternal fnreceivers keep their existing diagnostics.tests/src/codegen/wasm/negative.rs(3): the codegen assert is proven reachable viacatch_unwindon the diagnostics-ignoring path — at index 1, at index 2 behind an sret pointer, and a duplicated receiver landing on the duplicate message instead.Full default workspace
cargo test: 5526 passed, 0 failed (exit 0). Codegen goldens byte-identical —git status -- tests/test_datais empty, and bothcompile_mode_corpus_validates_as_wasm_1_0and the method golden family pass unchanged. Clippy clean (the only 4 workspace warnings are pre-existing intests/src/ast/builder_features.rs). Every table row above was re-verified end to end against a rebuilttarget/debug/infc— including rebuilding the pre-fix binary to confirm all three failure modes, and checking that the rejecting cases now emit no artifact and surface identically throughinfs build. The-vRocq path on accepted programs is unaffected.Reviewed by a separate adversarial pass, which found no correctness defect and five documentation/consistency nits; all five are fixed in this PR.
Docs
New
SelfReferenceNotFirstParameterentry incore/type-checker/docs/errors.md, and a### BreakingCHANGELOG entry.Deliberately out of scope
selfkeeps its leaky wording (error registering variable \self`: Variable `self` already declared in this scope`). Already fatal, so not a miscompile — worth its own issue.reject_duplicate_spec_struct_or_enumcontinues past the wholeDef::Structarm. Not unsound (the duplicate-struct error is itself fatal, so nothing is emitted, and the codegen assert backstops the library path), but it costs a second round-trip. This is a pre-existing property of thatcontinueaffecting every diagnostic in the arm, not something this change introduces.selfinside afn(i32, self)type. Only the parser can see it —lower.rsdiscards fn-type parameters entirely as a pinned parity quirk — so it never reaches lowering and cannot miscompile.register_definition_from_externalsilently drops struct methods, so the rule would be vacuous there. Currently dead code (load_preludehas no production caller); relevant to whoever revives the prelude path.Confidence Score: 5/5
The PR appears safe to merge; no concrete changed-code defect or independently actionable non-blocking issue remains.
The frontend check covers struct-method registration, valid receiver layouts satisfy the backend slot assertion for both ordinary and sret functions, and malformed diagnostics-recovery contexts are stopped before WASM emission.
Important Files Changed
Sequence Diagram
Reviews (1): Last reviewed commit: "Reject a method whose self is not the fi..." | Re-trigger Greptile
Context used (3)