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

checker: fix index expr that left is if expr (fix #22654) #22661

Merged
merged 4 commits into from
Oct 27, 2024

Conversation

yuyi98
Copy link
Member

@yuyi98 yuyi98 commented Oct 26, 2024

This PR fix index expr that left is if expr (fix #22654).

  • Fix index expr that left is if expr.
  • Add test.
fn main() {
	mut array_a := [1, 2, 3]
	mut array_b := [4, 5, 6]
	(if true { array_a } else { array_b })[0] = 999
	println(array_a)
	assert array_a == [999, 2, 3]
}

PS D:\Test\v\tt1> v run .
[999, 2, 3]
fn main() {
	array_a := [1, 2, 3]
	array_b := [4, 5, 6]
	(if true { array_a } else { array_b })[0] = 999
	println(array_a)
	assert array_a == [999, 2, 3]
}

PS D:\Test\v\tt1> v run .
tt1.v:4:13: error: `array_a` is immutable, declare it with `mut` to make it mutable
    2 |     array_a := [1, 2, 3]
    3 |     array_b := [4, 5, 6]
    4 |     (if true { array_a } else { array_b })[0] = 999
      |                ~~~~~~~
    5 |     println(array_a)
    6 |     assert array_a == [999, 2, 3]
tt1.v:4:30: error: `array_b` is immutable, declare it with `mut` to make it mutable
    2 |     array_a := [1, 2, 3]
    3 |     array_b := [4, 5, 6]
    4 |     (if true { array_a } else { array_b })[0] = 999
      |                                 ~~~~~~~
    5 |     println(array_a)
    6 |     assert array_a == [999, 2, 3]

Huly®: V_0.6-21110

@JalonSolov
Copy link
Contributor

My question is: Do we really want this to work? To me, it is confusing/unreadable trying to figure out what will happen.

If it takes me a minute to figure it out, how much will it affect newer developers?

And how is

(if true { array_a } else { array_b })[0] = 999

better than

if true { array_a[0] = 999 } else { array_b[0] = 999 }

other than being a few characters shorter to type??

@medvednikov
Copy link
Member

I agree with @JalonSolov here.

I'd just make it an error.

@StunxFS
Copy link
Contributor

StunxFS commented Oct 27, 2024

My question is: Do we really want this to work? To me, it is confusing/unreadable trying to figure out what will happen.

If it takes me a minute to figure it out, how much will it affect newer developers?

And how is

(if true { array_a } else { array_b })[0] = 999

better than

if true { array_a[0] = 999 } else { array_b[0] = 999 }

other than being a few characters shorter to type??

Or, it is better to simply write:

mut arr := if true { array_a } else { array_b }
arr[0] = 999

@medvednikov
Copy link
Member

@StunxFS not really, because V requires cloning on arr1=arr2, or unsafe references. Better to avoid both.

@yuyi98
Copy link
Member Author

yuyi98 commented Oct 27, 2024

I think this is valid usage and should not be reported wrong.

@JalonSolov
Copy link
Contributor

Yes, it is wrong because, as Alex said, it may require either cloning or unsafe (especially since both arrays shown are immutable). The code as given is incorrect since it does neither.

If one of them was declared mutable but the other wasn't, the if may succeed sometimes, and fail others. Even if both are declared mutable, it is confusing to read, as the syntax doesn't make it clear exactly what will happen.

@spytheman
Copy link
Member

I agree with @yuyi98 - the code is valid, even if it does seem weird.

@medvednikov
Copy link
Member

medvednikov commented Oct 27, 2024

This works in C

    ( (condition) ? arrayA : arrayB )[0] = 999;

and in all langs that have if expressions / ?:

@medvednikov medvednikov merged commit e59115e into vlang:master Oct 27, 2024
78 checks passed
@spytheman
Copy link
Member

If the concern is about mutability, we can check that both arrays are mutable for the simple case of if cond { arr1 } else { arr2 }[idx] = value.

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.

cannot use if-expression on the left side of an indexexpr
6 participants