Skip to content

Fix: check ref mut mutability for method call arguments#7630

Open
Dnreikronos wants to merge 7 commits into
FuelLabs:masterfrom
Dnreikronos:fix/ref-mut-method-check
Open

Fix: check ref mut mutability for method call arguments#7630
Dnreikronos wants to merge 7 commits into
FuelLabs:masterfrom
Dnreikronos:fix/ref-mut-method-check

Conversation

@Dnreikronos

@Dnreikronos Dnreikronos commented May 25, 2026

Copy link
Copy Markdown
Contributor

Description

Method calls weren't checking whether immutable arguments were being passed to ref mut parameters. The check already existed for free and associated function calls in function_application.rs, but method_application.rs never had it, so violations slipped through.

This adds the same check to method calls and fixes the existing violations it surfaced. gather_mutability() now recurses through StructFieldAccess and TupleElemAccess prefixes and treats constants as immutable, so p.x, t.0, and const arguments get caught too. The contract-call path skips the implicit self when lining up argument and parameter indices.

The check turned up two real cases. Hasher::write was passing its immutable bytes param straight into Bytes::append(ref mut), and a reexport test passed an immutable hasher into a ref mut hash method. Both rebind to a mutable local first now.

A new should_fail test covers the immutable variable, struct field, tuple element, and constant cases, plus a mutable variable that should still pass.

Closes #7621

Checklist

  • I have linked to any relevant issues.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation where relevant (API docs, the reference, and the Sway book).
  • If my change requires substantial documentation changes, I have requested support from the DevRel team
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added (or requested a) parsing test in core_lang/tests/sway_parse, if relevant.
  • I have added the necessary breaking and new feature labels where relevant.
  • I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
  • I have requested a review from the relevant team or maintainers.

The mutability check for `ref mut` parameters was only performed on free
function and associated function calls, not on method calls via dot
syntax. This allowed passing immutable variables to `ref mut` method
parameters without any compiler error.

Extend `gather_mutability()` to recurse through struct field access and
tuple element access, and add the same mutability check to
`type_check_method_application` that `function_application` already has.

Closes FuelLabs#7621
Hasher::write passed its immutable `bytes` parameter directly to
`Bytes::append` which requires `ref mut`. Rebind as mutable local
before the call. Also fix the same pattern in the reexport_paths test.
@Dnreikronos Dnreikronos requested review from a team as code owners May 25, 2026 12:09
@cursor

cursor Bot commented May 25, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Changes compile-time borrow/mutability rules for method calls and std Hasher::write; behavior is tightened with targeted e2e tests but may surface new errors in existing code.

Overview
Method calls now enforce the same immutable argument → ref mut parameter rule that free and associated functions already had, by running a shared check_arg_mutability during method application’s second argument pass (with correct arg/param alignment for contract calls, skipping self).

gather_mutability is extended so constants, struct fields (p.x), and tuple elements (t.0) on immutable bindings count as immutable, enabling those cases to be rejected at method sites.

Follow-up fixes: Hasher::write in std rebinds Bytes before append(ref mut), and a reexport test uses let mut hasher. A new should_fail e2e test covers the error cases and a passing mutable variable.

Reviewed by Cursor Bugbot for commit 6367ec9. Bugbot is set up for automated code reviews on this repo. Configure here.

@Dnreikronos Dnreikronos force-pushed the fix/ref-mut-method-check branch from e67dca4 to da57ea7 Compare May 25, 2026 12:27
@codspeed-hq

codspeed-hq Bot commented Jun 4, 2026

Copy link
Copy Markdown

Merging this PR will improve performance by 11.01%

⚡ 1 improved benchmark
✅ 24 untouched benchmarks

Performance Changes

Benchmark BASE HEAD Efficiency
tokens_for_program 591.2 µs 532.5 µs +11.01%

Tip

Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.


Comparing Dnreikronos:fix/ref-mut-method-check (6367ec9) with master (ac933ff)

Open in CodSpeed

The mutability check ran in a separate loop that assumed args_buf[0]
was always the contract-call receiver. The type-check loop only pushes
that receiver when its first-pass type is present, so when it is absent
args_buf is shorter and the index-1 mapping pairs arguments with the
wrong parameters. Fold the check into the type-check loop so it reuses
the same source-argument-to-parameter mapping and stays correct.
Both function and method application emitted the same
new_from_ref_mut + gather_mutability + ImmutableArgumentToMutableParameter
check inline. Pull it into one check_arg_mutability helper so the two
paths can't drift. Also rename the misleading arg_had_err binding to
needs_recheck, since it tracks needs_second_pass (errors or numerics).
@Dnreikronos

Copy link
Copy Markdown
Contributor Author

bugbot run

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 6367ec9. Configure here.

@Dnreikronos

Copy link
Copy Markdown
Contributor Author

@ironcev ready, and it picked up a small perf win on the way (codspeed has tokens_for_program about 11% faster). It runs the same immutable-arg into ref mut-param check on method calls that free and associated fns already had, and extends gather_mutability to cover consts, struct fields, and tuple elements. The bit to double-check is the arg/param alignment for contract calls, where it skips self.

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.

ref mut check for mutable argument not done on methods

1 participant