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

Tracking issue for Seek::{stream_len, stream_position} (feature seek_convenience) #59359

Open
2 tasks
LukasKalbertodt opened this issue Mar 22, 2019 · 13 comments
Open
2 tasks

Comments

@LukasKalbertodt
Copy link
Member

@LukasKalbertodt LukasKalbertodt commented Mar 22, 2019

This is a tracking issue for Seek::{stream_len, stream_position}. Proposed and implemented in #58422.

Unresolved questions:

  • Override stream_len for File? (is metadata syncing a problem? comment a, comment b)
  • Final names:
    • Rename to len and position? (but that's a strange signature for len and position clashes with Cursor
    • ...
lo48576 added a commit to lo48576/fbxcel that referenced this issue Apr 9, 2019
`reader.stream_pos()` is equivalent to
`reader.seek(SeekFrom::Current(0))`.

See <rust-lang/rust#59359>.
lo48576 added a commit to lo48576/fbxcel that referenced this issue Apr 21, 2019
`reader.stream_position()` is equivalent to
`reader.seek(SeekFrom::Current(0))`.

See <rust-lang/rust#59359>.
lo48576 added a commit to lo48576/fbxcel that referenced this issue Apr 21, 2019
`reader.stream_position()` is equivalent to
`reader.seek(SeekFrom::Current(0))`.

See <rust-lang/rust#59359>.
lo48576 added a commit to lo48576/fbxcel that referenced this issue Aug 9, 2019
`reader.stream_position()` is equivalent to
`reader.seek(SeekFrom::Current(0))`.

See <rust-lang/rust#59359>.
@NieDzejkob
Copy link
Contributor

@NieDzejkob NieDzejkob commented Nov 28, 2019

As the issue calls for a bikeshed... tell seems to be an established name for what is called stream_position here. I don't see an analogous alternative name for stream_length, though, and the two methods having similar names might be advantageous. Besides, stream_position is more descriptive.

@marmistrz
Copy link
Contributor

@marmistrz marmistrz commented Feb 24, 2020

From the API user perspective, stream_position should probably borrow immutably.

@LukasKalbertodt
Copy link
Member Author

@LukasKalbertodt LukasKalbertodt commented Apr 7, 2020

I opened a stabilization PR here: #70904


From the API user perspective, stream_position should probably borrow immutably.

@marmistrz Yes, that would be nice. But it's not possible as the method is just a small helper around seek. And seek requires &mut self.

@marmistrz
Copy link
Contributor

@marmistrz marmistrz commented Apr 9, 2020

Yes, therefore this should be implemented separately, on the low level, just as it's done in yanix:
https://github.com/bytecodealliance/wasmtime/blob/c4e90f729ca5e17a29a778ace7971aeda7c22391/crates/wasi-common/yanix/src/file.rs#L251-L258

@LukasKalbertodt
Copy link
Member Author

@LukasKalbertodt LukasKalbertodt commented Apr 9, 2020

@marmistrz Unfortunately, I don't quite understand what you are suggesting. And in particular: are you opposing stabilization of the features as they currently are?

@marmistrz
Copy link
Contributor

@marmistrz marmistrz commented Apr 9, 2020

In my opinion, instead of the cross-platform implementation present in
https://github.com/rust-lang/rust/pull/58422/files#diff-668f8f358d4a93474b396dcb3727399eR1407-R1410, platform-specific implementations should be used (lseek on Unix/something else on Windows/etc), so that a mutable borrow is not required.

I think that the API should not be stabilized, because it's semantically incorrect to require mutable borrows for immutable queries. If the API user only has an immutable reference, they'll still have to use platform-specific unsafe code. (casting & to &mut is UB)

@LukasKalbertodt
Copy link
Member Author

@LukasKalbertodt LukasKalbertodt commented Apr 9, 2020

A few things:

  • You are mainly talking about files, but Seek is implemented for many more things which potentially do not have a way to get the length otherwise. So "it's semantically incorrect to require mutable borrows for immutable queries." is only true for files and the like where mutability is indeed not required. But for some of Seeks implementors, a mutable borrow is indeed correct.

  • For files, there is already an API that safely gets you the length with only an immutable borrow (file.metadata().len()).

  • Even if we want to add another API for tell-like behavior (e.g. because metadata().len() is not the same), this would not conflict with Seek::stream_len. The only reason I see for not stabilizing this API is if you think that it is "a bad influence on users". I.e. "users should only very rarely use three seeks to get the length and instead prefer other ways to obtain the length, so having a method that does this easily accessible is harmful". Is that what you were implying? However, again remember that Seek is not just about files.

@zseri
Copy link
Contributor

@zseri zseri commented Apr 9, 2020

@LukasKalbertodt "Seek is not just about files" I'm curious, on which other "I/O objects" is this intended to be implemented on (just to make clear what we're talking about)? Are there existing I/O objects on which stream_len can only be implemented with a mutable borrow?

@marmistrz
Copy link
Contributor

@marmistrz marmistrz commented Apr 9, 2020

  • For files, there is already an API that safely gets you the length with only an immutable borrow (file.metadata().len()).

file.metadata().len() returns the total file length, not the current offset in the file and doesn't change after seeking. It's not a replacement for ftell() or stream_position.

@t-rapp
Copy link
Contributor

@t-rapp t-rapp commented Jul 24, 2020

I think for files there could be a specialized File::position() method that takes an immutable &self, similar to the existing Cursor::position(). As the Seek trait is more general (and due to API stability) the Seek::stream_len() and Seek::stream_position() convenience / optimization functions unfortunately require a mutable borrow. They are still useful, in my opinion.

@HeroicKatora
Copy link
Contributor

@HeroicKatora HeroicKatora commented Sep 17, 2020

@marmistrz As with most io-traits there is the impl<'_> Seek for &'_ File and the &mut &File signature for its stream_position is for all functional means equivalent to taking a &File as self. This wouldn't require any changes to the current design. It does mean, however, that any specialization to File should be done to &File as well as the implementation of the latter does not dispatch to the former.

jonas-schievink added a commit to jonas-schievink/rust that referenced this issue Jan 27, 2021
…enience, r=m-ou-se

Stabilize `Seek::stream_position` (feature `seek_convenience`)

Tracking issue: rust-lang#59359

Unresolved questions from tracking issue:
- "Override `stream_len` for `File`?" → we can do that in the future, this does not block stabilization.
- "Rename to `len` and `position`?" → as noted in the tracking issue, both of these shorter names have problems (`len` is usually a cheap getter, `position` clashes with `Cursor`). I do think the current names are perfectly fine.
- "Rename `stream_position` to `tell`?" → as mentioned in [the comment bringing this up](rust-lang#59359 (comment)), `stream_position` is more descriptive. I don't think `tell` would be a good name.

What remains to decide, is whether or not adding these methods is worth it.
jonas-schievink added a commit to jonas-schievink/rust that referenced this issue Jan 27, 2021
…enience, r=m-ou-se

Stabilize `Seek::stream_position` (feature `seek_convenience`)

Tracking issue: rust-lang#59359

Unresolved questions from tracking issue:
- "Override `stream_len` for `File`?" → we can do that in the future, this does not block stabilization.
- "Rename to `len` and `position`?" → as noted in the tracking issue, both of these shorter names have problems (`len` is usually a cheap getter, `position` clashes with `Cursor`). I do think the current names are perfectly fine.
- "Rename `stream_position` to `tell`?" → as mentioned in [the comment bringing this up](rust-lang#59359 (comment)), `stream_position` is more descriptive. I don't think `tell` would be a good name.

What remains to decide, is whether or not adding these methods is worth it.
jonas-schievink added a commit to jonas-schievink/rust that referenced this issue Jan 27, 2021
…enience, r=m-ou-se

Stabilize `Seek::stream_position` (feature `seek_convenience`)

Tracking issue: rust-lang#59359

Unresolved questions from tracking issue:
- "Override `stream_len` for `File`?" → we can do that in the future, this does not block stabilization.
- "Rename to `len` and `position`?" → as noted in the tracking issue, both of these shorter names have problems (`len` is usually a cheap getter, `position` clashes with `Cursor`). I do think the current names are perfectly fine.
- "Rename `stream_position` to `tell`?" → as mentioned in [the comment bringing this up](rust-lang#59359 (comment)), `stream_position` is more descriptive. I don't think `tell` would be a good name.

What remains to decide, is whether or not adding these methods is worth it.
jonas-schievink added a commit to jonas-schievink/rust that referenced this issue Jan 27, 2021
…enience, r=m-ou-se

Stabilize `Seek::stream_position` (feature `seek_convenience`)

Tracking issue: rust-lang#59359

Unresolved questions from tracking issue:
- "Override `stream_len` for `File`?" → we can do that in the future, this does not block stabilization.
- "Rename to `len` and `position`?" → as noted in the tracking issue, both of these shorter names have problems (`len` is usually a cheap getter, `position` clashes with `Cursor`). I do think the current names are perfectly fine.
- "Rename `stream_position` to `tell`?" → as mentioned in [the comment bringing this up](rust-lang#59359 (comment)), `stream_position` is more descriptive. I don't think `tell` would be a good name.

What remains to decide, is whether or not adding these methods is worth it.
JohnTitor added a commit to JohnTitor/rust that referenced this issue Jan 28, 2021
…enience, r=m-ou-se

Stabilize `Seek::stream_position` (feature `seek_convenience`)

Tracking issue: rust-lang#59359

Unresolved questions from tracking issue:
- "Override `stream_len` for `File`?" → we can do that in the future, this does not block stabilization.
- "Rename to `len` and `position`?" → as noted in the tracking issue, both of these shorter names have problems (`len` is usually a cheap getter, `position` clashes with `Cursor`). I do think the current names are perfectly fine.
- "Rename `stream_position` to `tell`?" → as mentioned in [the comment bringing this up](rust-lang#59359 (comment)), `stream_position` is more descriptive. I don't think `tell` would be a good name.

What remains to decide, is whether or not adding these methods is worth it.
lo48576 added a commit to lo48576/fbxcel that referenced this issue Mar 26, 2021
`reader.stream_position()` is equivalent to
`reader.seek(SeekFrom::Current(0))`.

See <rust-lang/rust#59359>.
@bluss
Copy link
Member

@bluss bluss commented Apr 2, 2021

For BufWriter, it might not be clear to users that this method will write out the buffer. Even though that shouldn't be a big problem (and possible to deduct). Same with stream_position

@t-rapp
Copy link
Contributor

@t-rapp t-rapp commented Apr 6, 2021

I assume an optimized implementation of stream_position could be done, similar to how it was done to BufReader in #74366.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
9 participants