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

Implement conversion traits for usize/isize together with a portability lint #70460

Open
joshlf opened this issue Mar 27, 2020 · 16 comments
Open
Labels
T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@joshlf
Copy link
Contributor

joshlf commented Mar 27, 2020

This PR was closed 3.5 years ago just prior to the development of the portability lint RFC. In light of the portability lint RFC having been accepted, we should be able to resurrect that PR.

This issue tracks that work. I've submitted it to TWiR's CFP.

@estebank estebank added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Mar 28, 2020
@joshtriplett
Copy link
Member

joshtriplett commented Apr 2, 2020

Some notable considerations and use case here (note: speaking for myself here, not for any team):

  • 32-bit systems are common enough that it seems useful to warn (via the portability lint) on code that assumes usize is at least 64-bit. Code that wants to say "this only runs on 64-bit" should need to specify that.
  • 16-bit systems are uncommon enough that it seems unhelpful to warn by default about code that assumes usize is at least 32-bit. I would argue that code should need to opt into dealing with the case where usize is 16-bit.
  • 128-bit systems are not yet common, but they lie in the future rather than the past, so it seems reasonable to debate whether we should warn by default about assumptions that usize fits in 64-bit. I personally feel that we should not warn about this by default, but that we may wish to change that in the future.

People have expressed the desire for checked .into() conversions between usize and sized integer types such as u32 or u64; in particular, the two most common requests that don't exist are:

  • impl From<u32> for usize
  • impl From<usize> for u64

In addition, code that explicitly declares that it only works on a 64-bit target should be able to use impl From<u64> for usize.

@c3st7n
Copy link
Contributor

c3st7n commented Apr 6, 2020

I would be interested in picking this up.

@joshlf
Copy link
Contributor Author

joshlf commented Apr 7, 2020

@joshtriplett IIU the portability lint RFC correctly, non-portable APIs must be linted no matter how rare the exception cases are, so we'd need to lint even, e.g., impl From<u16> for usize.

@c3st7n Looks like nobody else has spoken up so far, so it's all yours!

@joshtriplett
Copy link
Member

joshtriplett commented Apr 8, 2020 via email

@joshlf
Copy link
Contributor Author

joshlf commented Apr 8, 2020

I'm arguing that that shouldn't necessarily be the case.

Pretty sure that'd need another RFC or at least some other kind of official approval process?

@petrochenkov
Copy link
Contributor

@joshlf
https://github.com/rust-lang/rfcs/blob/master/text/1868-portability-lint.md#background-portability-and-the-standard-library

  • We identified a set of "mainstream" platforms, consisting of 32- and 64-bit
    machines running Windows, Linux, or macOS. "Portability by default" thus more
    specifically means portability to mainstream platforms.

The intent was to not warn by default on non-portability to rare platforms, #37423 also implemented the "portability to 16-bit targets" lint as allow-by-default.

@joshlf
Copy link
Contributor Author

joshlf commented Apr 8, 2020

Huh TIL. Thanks for clarifying!

@nikomatsakis
Copy link
Contributor

I'm removing the nomination since I think T-lang doesn't have anything else to say, and I don't think @rust-lang/libs has any objections.

@c3st7n, in terms of implementation work, do you know how to get started?

@c3st7n
Copy link
Contributor

c3st7n commented Apr 18, 2020

@nikomatsakis I thought I did, my plan was to implement a macro like in @petrochenkov's original PR to add the impl from for each usize/uXX combination, with appropriate target_pointer_width cfg values.

Where I am getting stuck/confused is the portability lint parts. I'm not sure if I am fully understanding them, is the best documentation the RFC (if so, this seems a little odd to me and I would expect something concrete in say the rust book or similar). The above thread refer to warning on different platforms and scenarios, but the way I read the portability lint RFC if I annotate a function with #[cfg(any(target_pointer_width = "32", target_pointer_width = "64"))], tried to call that function and then tried to compile on a 16 bit system, I would get an actual error as the function would essentially not exist. I don't follow how you would use this to warn only?

@sfackler
Copy link
Member

The portability lint has never been implemented.

@c3st7n
Copy link
Contributor

c3st7n commented Apr 18, 2020

I think that probably means I would need a fair bit of help to understand this work or for it to be re-assigned. Happy to carry on if someone can give me some pointers.

@petrochenkov
Copy link
Contributor

The portability lint in #37423 was different from what the RFC described.
It didn't consider any #[cfg]s and only warned if the conversion functions from the non-portable impls were used (on best-effort basis).

@c3st7n
Copy link
Contributor

c3st7n commented Apr 18, 2020

Ok, this is starting to make a lot more sense to me. I completely misunderstood the full scope of the task and thought there were already portability lints that existed but I was just failing to find/understand them. I will do some further reading and make sure I understand the original PR.

@SimonSapin
Copy link
Contributor

PR #71360 proposes an alternative to this without a portability lint, using instead the same pattern as std::os.

@c3st7n
Copy link
Contributor

c3st7n commented May 17, 2020

I haven't even tried working on this by the way because I am assuming from @SimonSapin comment that this will be made redundant.

@nashley
Copy link

nashley commented Feb 11, 2021

I haven't even tried working on this by the way because I am assuming from @SimonSapin comment that this will be made redundant.

Looks like that attempt was abandoned: #71360 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

9 participants