Allow field use after a conditional field move by shindakioku · Pull Request #994 · inko-lang/inko

shindakioku · 2026-07-09T17:12:17Z

When a field is conditionally moved out of self (e.g. if x { drop(@a) }), using a different field afterwards was wrongly rejected as "self has been moved". Joining the branches leaves self as MaybeMoved, which was treated the same as a whole move. We now report this only when self itself is moved on one of the paths.

This fixes #962.

Changelog: fixed

@A

When a field is conditionally moved out of `self` (e.g. `if x { drop(@A) }`), using a different field afterwards was wrongly rejected as "self has been moved". Joining the branches leaves `self` as `MaybeMoved`, which was treated the same as a whole move. We now report this only when `self` itself is moved on one of the paths. This fixes inko-lang#962. Changelog: fixed

yorickpeterse · 2026-07-09T20:44:20Z

+            // field is moved. In the latter case `self` itself is still valid,
+            // so reading a *different* field is fine. To tell the two apart we
+            // walk the predecessor blocks and only report an error if `self`
+            // was actually moved as a whole on one of them.


This comment isn't really useful as it mostly just replicates what's already in the issue, so it can/should just be removed.

yorickpeterse · 2026-07-09T20:57:30Z

+            // walk the predecessor blocks and only report an error if `self`
+            // was actually moved as a whole on one of them.
+            RegisterState::MaybeMoved => {
+                if self.self_fully_moved_on_any_path() {


This method isn't the direction we should take. It essentially implements a similar approach to the register_state method but that's not necessary.

I hinted at this in Discord but I think I didn't do a good job, but what I think we need to do is this:

LowerMethod::partially_move_self_if_field needs to be removed along with its call sites

When the state of a field is updated, we only update that field register's state (which the previous step should accomplish)

In self_expression and other places where self is used (e.g. as the implicit receiver of a method call), we iterate over the values in LowerMethod::field_mapping and for each register check if its state is Available. If not, we produce the "self is partially moved" diagnostic

In LowerMethod::field we only check the state of the the field's register, not that of LowerMethod::self_register, so basically the match is replaced with just the appropriate call to LowerMethod::check_if_moved

If self is moved, in addition to updating the state of its own register we also update the register state of each field to the same state as self, so basically move(self) translates to move(self), move(@field1), move(@fieldN), ...

yorickpeterse · 2026-07-09T20:59:33Z

+
+type Inner {}
+
+impl Drop for Inner {


These drop implementations are not necessary for the purpose of this test, so they can be removed. The same is true for the other diagnostic test.

Instead of marking `self` as partially moved and walking predecessor blocks to distinguish a whole-`self` move from a field move, the move state is now tracked per field register. Reading a field checks only that field's register; moving `self` marks all of its fields as moved; and whether `self` is partially moved is derived from its fields where needed (e.g. when using `self` directly or as a method receiver).

yorickpeterse · 2026-07-12T20:13:03Z

+
+        // Moving `self` as a whole also moves all of its fields.
+        if register == self.self_register {
+            let fields: Vec<RegisterId> =


Cloning and collecting here is redundant and can be avoided by just copying the body of update_register_state here. It's just one simple line so there's no harm in duplicating that.

yorickpeterse · 2026-07-12T20:13:21Z

+    /// Returns `true` if any field of `self` has been (possibly) moved.
+    fn self_has_moved_field(&mut self) -> bool {
+        let fields: Vec<RegisterId> =
+            self.field_mapping.values().cloned().collect();


Same here, cloning and collecting is redundant here.

yorickpeterse · 2026-07-12T20:14:29Z

@@ -0,0 +1,22 @@
+import std.drop (drop)


There should also be a diagnostic test added for when self is used explicitly after moving a field, and a test for when a method is called on an implicit self after a field is moved.

While we're at it, a diagnostic test in which self is captured by a closure (both explicitly and implicitly) after it's partially moved would be useful to ensure that continues to work as expected.

yorickpeterse · 2026-07-12T20:16:42Z

+                );
+            }
+        } else {
+            self.check_if_moved(reg, SELF_NAME, node.location);


Since this call only happens if self.self_register is not moved, using check_if_moved instead of directly calling self.state.diagnostics.moved_variable() is just redundant. It's also a bit weird because in the if body you do use moved_variable directly.

yorickpeterse · 2026-07-12T20:22:38Z

-            RegisterState::PartiallyMoved => {
+            // Fully moved: its fields are moved too, so there's nothing to drop.
+            RegisterState::Moved => {}
+            // Maybe moved as a whole: `drop_register` drops it conditionally.


The "drop_register drops it conditionally" is redundant because it just describes what the code does but in English. More precisely, comments should be used to describe intent/meaning or something else that isn't necessarily obvious, rather than describe in English what the code already describes.

yorickpeterse · 2026-07-12T20:22:57Z


        match self.register_state(self_reg) {
-            RegisterState::PartiallyMoved => {
+            // Fully moved: its fields are moved too, so there's nothing to drop.


This comment isn't really useful because self being Moved as a whole is obvious enough on its own.

yorickpeterse · 2026-07-12T20:23:43Z

+                self.drop_register(self_reg, location);
+            }
+            // Available but with some fields moved out: drop the remaining
+            // fields and free self's memory without running its dropper.


Same here as above: don't use comments to describe what the code already describes, there's no value in that. In fact, because of the if partially_moved below I feel this entire comment is redundant.

yorickpeterse · 2026-07-12T20:39:12Z

-                self.drop_partial_self(&fields, location);
-            }
-            RegisterState::Available | RegisterState::MaybeMoved => {
+            RegisterState::Available | RegisterState::PartiallyMoved => {


mark_register_as_partially_moved gets called when pattern matching with bindings, flagging self as PartiallyMoved when you match against it, so something like this:

match self { case { @field1 = a, @field2 = b, ... } -> { ... } }

This would then cause this pattern body to treat it as if self were fully available when it is in fact not, causing the fields to be (potentially) dropped twice.

I think in this case the PartiallyMoved pattern/body should just remain the same as-is.

Detect when `self` (or one of its fields) is used after a field has been moved out, emitting a `moved` diagnostic. Handle the `PartiallyMoved` state explicitly when dropping `self` so remaining fields are dropped and `self`'s memory is freed without running its dropper. Switch `field_mapping` to an `IndexMap` for stable positional iteration. Adds diagnostics fixtures covering self use, implicit method calls, and closure captures after a field move.

yorickpeterse self-requested a review July 9, 2026 20:37

yorickpeterse requested changes Jul 9, 2026

View reviewed changes

yorickpeterse requested changes Jul 12, 2026

View reviewed changes

yorickpeterse mentioned this pull request Jul 17, 2026

Release 0.21.0 #995

Open

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Allow field use after a conditional field move#994

Allow field use after a conditional field move#994
shindakioku wants to merge 3 commits into
inko-lang:mainfrom
shindakioku:fix-conditional-field-move

shindakioku commented Jul 9, 2026

Uh oh!

yorickpeterse Jul 9, 2026

Uh oh!

yorickpeterse Jul 9, 2026

Uh oh!

yorickpeterse Jul 9, 2026

Uh oh!

yorickpeterse Jul 12, 2026

Uh oh!

yorickpeterse Jul 12, 2026

Uh oh!

yorickpeterse Jul 12, 2026

Uh oh!

yorickpeterse Jul 12, 2026

Uh oh!

yorickpeterse Jul 12, 2026

Uh oh!

yorickpeterse Jul 12, 2026

Uh oh!

yorickpeterse Jul 12, 2026

Uh oh!

yorickpeterse Jul 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

shindakioku commented Jul 9, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants