Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mut #692

Closed
wants to merge 7 commits into from
Closed

mut #692

wants to merge 7 commits into from

Conversation

sbillig
Copy link
Collaborator

@sbillig sbillig commented Mar 29, 2022

This adds the mut keyword, used in a fashion similar to rust (with some differences caused by the fact that we don't yet have explicit references in fe).

For example:

fn mutate_array(mut array: Array<u8, 3>) {  # this would be `array: &mut Array<u8, 3>`
  array[0] = 10
}
let mut foo: Array<u8, 3> = [1, 2, 3]
mutate_array(foo)

This is an important language feature, and this pr is a "better than nothing" (I hope) implementation, which should ultimately be replaced or improved. There are some problems caused by the fact that all compound types are implicitly references, and this pr doesn't really attempt to solve them.

Consider:

struct X {
  my_array: Array<u8, 3>
  pub fn foo(self) -> Array<u8, 3> { ... }
}
let x: X = ...
let mut y: Array<u8, 3> = x.foo()

Does X::get_array return a (non-mut) reference to the my_array field? Or does it return a new array? The return type in the current fe syntax is ambiguous, so there's no way to tell from that. We could attempt to answer this question by doing some dataflow analysis on the body of foo to figure out the actual return type and represent it as a Type::Ref { inner: Type::Array { .. }} or whatever, but this pr does no such thing. Instead, mutability is sort of tacked onto the side, in a way that mostly works, but can't possibly be perfect.

The correct solution is probably to copy rust (of course), where the return type would be either &Array<u8, 3> or Array<u8, 3> and would thus be unambiguous. Alternatively, if we want to keep the all-compound-types-are-implicit-references thing (like in python&co), we could use -> mut Array<u8, 3> to signify that the returned array can be mutated by the recipient. If the body of foo was return self.my_array, it would be an obvious error. I prefer the ownership semantics of rust, where -> Array<u8, 3> unambiguously means that the recipient is getting its own array that isn't shared with anyone else, but of course that brings with it a pile of complexity around references.

Note that this pr also definitely doesn't do anything about aliasing. There can be multiple mutable references to the same thing. It shouldn't, however, be possible to create a mutable reference to a thing that's declared as immutable.

Because I was in there messing with mut ctx stuff, I changed how ctx works in a couple cases. I could split this out into a separate pr, but knowing me I probably won't. I did! #703

To-Do

  • OPTIONAL: Update Spec if applicable

  • Add entry to the release notes (may forgo for trivial changes)

  • Clean up commit history

@sbillig sbillig marked this pull request as draft March 29, 2022 01:51
@codecov-commenter
Copy link

Codecov Report

Merging #692 (6d00d3c) into master (d5c43d5) will increase coverage by 0.03%.
The diff coverage is 90.47%.

❗ Current head 6d00d3c differs from pull request most recent head 96bc925. Consider uploading reports for the commit 96bc925 to get more accurate results

@@            Coverage Diff             @@
##           master     #692      +/-   ##
==========================================
+ Coverage   82.03%   82.07%   +0.03%     
==========================================
  Files         125      125              
  Lines       10865    10922      +57     
==========================================
+ Hits         8913     8964      +51     
- Misses       1952     1958       +6     
Impacted Files Coverage Δ
crates/analyzer/src/context.rs 61.34% <ø> (-0.85%) ⬇️
crates/parser/src/lexer/token.rs 17.77% <0.00%> (-0.20%) ⬇️
crates/analyzer/src/traversal/assignments.rs 86.44% <33.33%> (-6.42%) ⬇️
crates/lowering/src/ast_utils.rs 83.67% <50.00%> (-0.71%) ⬇️
crates/parser/src/ast.rs 88.41% <88.88%> (-0.13%) ⬇️
crates/analyzer/src/db/queries/functions.rs 90.47% <93.33%> (+0.10%) ⬆️
crates/analyzer/src/namespace/items.rs 93.12% <100.00%> (+0.60%) ⬆️
crates/analyzer/src/namespace/scopes.rs 79.64% <100.00%> (+0.50%) ⬆️
crates/analyzer/src/namespace/types.rs 72.65% <100.00%> (ø)
crates/analyzer/src/traversal/declarations.rs 93.84% <100.00%> (+1.11%) ⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d5c43d5...96bc925. Read the comment docs.

@sbillig
Copy link
Collaborator Author

sbillig commented Aug 4, 2022

Closed in favor of #777

@sbillig sbillig closed this Aug 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants