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

os: Fix File.read() in JS backends [Issue : #20501] #20633

Merged
merged 2 commits into from
Jan 29, 2024

Conversation

GGRei
Copy link
Contributor

@GGRei GGRei commented Jan 22, 2024

Fixed #20501 File.read() function for the JavaScript backend.

The issue was due to incorrect array access in the JavaScript generated from V code.

To respond at ticket expectation :

  • Using the js backend: expected to either read a non-zero amount of bytes, or raise an error.

The original function already includes a try-catch block to manage errors. Unless specifically requested, I don't plan to add additional error handling

- Using the js_browser: expected to either fail compilation (since File I/O is unavailable in the browser), or for os.open() to always return an error.

the existing code already handles errors. If open_file() is called in a non-NodeJS environment, an error is generated. However, it appears that the test in ticket was conducted using Node.js, which could explain the observed results.

I'm always available for any suggestions/modifications.

…code for 'read' function, implementing precise array access and type conversion to ensure proper interaction with the V data structure
@spytheman spytheman force-pushed the fix_read_file_output_in_js_backend branch from a91f702 to 492527b Compare January 28, 2024 14:54
@spytheman
Copy link
Member

(rebased over current master)

@spytheman
Copy link
Member

image

The observed behavior for the C backend and for the JS backend is still different.
You can use this file open_and_read_from_file_test.js.v to try (and please add it to vlib/os/ for example, to prevent future regressions).

import os

fn test_read_from_file() {
	mut buf := []u8{len: 10}
	f := os.open(@FILE)!
	n := f.read(mut &buf)!
	println(buf[..n])
}	

@spytheman
Copy link
Member

Note that the C version https://github.com/vlang/v/blob/master/vlib/os/file.c.v#L233 , reads up to buf.len bytes, and it returns how many bytes it could read.
It does not read the whole file.

@GGRei
Copy link
Contributor Author

GGRei commented Jan 28, 2024

Done. let me know if everything is okay. Thanks.

@spytheman
Copy link
Member

I can confirm, that the fix for File.read/1 is working.

It still prints differently, but fixing that is out of scope for this PR.

Excellent work.

@spytheman spytheman merged commit 2d68230 into vlang:master Jan 29, 2024
48 checks passed
raw-bin pushed a commit to raw-bin/v that referenced this pull request Jan 29, 2024
raw-bin pushed a commit to raw-bin/v that referenced this pull request Jan 30, 2024
@GGRei GGRei deleted the fix_read_file_output_in_js_backend branch January 31, 2024 15:44
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.

File.read() always returns 0 bytes in JS backends
2 participants