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

docs: os.walk() cannot update mut array passed to its closure - closure needs better explanation about copy vs reference #23166

Open
jorgeluismireles opened this issue Dec 14, 2024 · 7 comments
Labels
Unit: Documentation Bugs/feature requests, that are related to the documentations.

Comments

@jorgeluismireles
Copy link

jorgeluismireles commented Dec 14, 2024

Describe the bug

We pass a mutable array to os.walk closure where the array is appended with content and the array outside the closure is not updated.

Reproduction Steps

import os

fn main() {
	mut files := []string{}
	os.walk('.', fn[mut files](file string) {
		dump('${file}')
		files << file
	})	
	dump('${files}') // <- EMPTY!
}

Expected Behavior

[code.v:6] '$file': ./code
[code.v:6] '$file': ./code.v
[code.v:6] '$file': ./.vmodules/.cache/README.md
[code.v:9] '$files': [ code, code.v, README.md ] // <- three files outside the closure

Current Behavior

[code.v:6] '$file': ./code
[code.v:6] '$file': ./code.v
[code.v:6] '$file': ./.vmodules/.cache/README.md
[code.v:9] '$files': [] // no files outside the closure

Possible Solution

No response

Additional Information/Context

Warning: The bug is reproducible in the playground (see link below). Walking the current folder gives three files. Should be checked if the files are secure in case someone goes beyond just printing the names.

V version

V 0.4.8 9e71e32

Environment details (OS name and version, etc.)

https://play.vlang.io/p/6e3acff8fe

Note

You can use the 👍 reaction to increase the issue's priority for developers.

Please note that only the 👍 reaction to the issue itself counts as a vote.
Other reactions and those to comments will not be taken into account.

Huly®: V_0.6-21603

@jorgeluismireles jorgeluismireles added the Bug This tag is applied to issues which reports bugs. label Dec 14, 2024
@jorgeluismireles
Copy link
Author

jorgeluismireles commented Dec 14, 2024

Workaround:

import os

@[heap]
struct Files {
mut:
	files []string
}

fn (mut f Files) walk(file string) {
	dump('${file}')
	f.files << file
}

fn main() {
	f := Files{}
	os.walk('.', f.walk)
	dump('${f.files}')
}
[code.v:10] '$file': ./code
[code.v:10] '$file': ./code.v
[code.v:10] '$file': ./.vmodules/.cache/README.md
[code.v:17] '${f.files}': ['./code', './code.v', './.vmodules/.cache/README.md']

https://play.vlang.io/p/52eee9c8d5

@JalonSolov
Copy link
Contributor

This is not a bug. https://docs.vlang.io/functions-2.html#closures

"Inherited variables are copied when the anonymous function is created. This means that if the original variable is modified after the creation of the function, the modification won't be reflected in the function."

You first example is declaring a mutable copy of the files array.

Your second example works because you are passing a pointer to a method, and it is bring called to update the struct.

If you want to use a closer here, you have to pass a pointer to the thing you want to be modified.

@kbkpbot
Copy link
Contributor

kbkpbot commented Dec 15, 2024

So the checker should detect this, and issue an error message ?

@JalonSolov
Copy link
Contributor

No, it's working as designed. A copy is made, as per the documentation.

Perhaps the documentation could be improved to tell you what to do if you want to modify the original, but there are no changes needed in the compiler.

@aster-void
Copy link

this could be a warning such as "variable mutated but not used", for those with higher level lang experience, but not an error

@JalonSolov
Copy link
Contributor

I disagree. There is no reason to add a warning for code that is operating as documented.

@jorgeluismireles
Copy link
Author

JalonSolov is correct about documentation. But... instead consulting the docs I first assume mut guarantees modification. Because I remember is not permitted to do fn (mut s &Struct) method() { } that is, repeat both mut and & "unnecessarly".
So this now works:

import os

fn main() {
	mut files := []string{}
	mut ref := &files
	os.walk('.', fn[mut ref](file string) {
		dump('${file}')
		ref << file
	})	
	dump('${files}')
}

Please note it didn't require this code to de-reference the ref with * inside the closure as in the docs:

Since in the os.walk case the closure returns nothing I didn't pay attention to (and could not use) this doc example:

fn new_counter() fn () int {
    mut i := 0
    return fn [mut i] () int {
        i++
        return i
    }
}
c := new_counter()
println(c()) // 1
println(c()) // 2
println(c()) // 3

where a copy (as Jalon said) is passed, modified and shared to the exterior by a return (copy again). I wonder if this case is useful and wonder also how long the real variable i (the counter) is available in the program life.

Any way, thanks Jalon and if you want you could close this issue or wait for more questions since this is not trivial.

@JalonSolov JalonSolov added Unit: Documentation Bugs/feature requests, that are related to the documentations. and removed Bug This tag is applied to issues which reports bugs. labels Dec 15, 2024
@JalonSolov JalonSolov changed the title os.walk() cannot update mut array passed to its closure documentation: os.walk() cannot update mut array passed to its closure - needs better explanation about copy vs reference Dec 15, 2024
@JalonSolov JalonSolov changed the title documentation: os.walk() cannot update mut array passed to its closure - needs better explanation about copy vs reference docs: os.walk() cannot update mut array passed to its closure - closure needs better explanation about copy vs reference Dec 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Unit: Documentation Bugs/feature requests, that are related to the documentations.
Projects
None yet
Development

No branches or pull requests

4 participants