-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
RFC: A portability lint #1868
RFC: A portability lint #1868
Conversation
cc @rust-lang/libs: this is what the "scenarios" idea evolved into. |
If you attempted to call `as_raw_fd`, when compiling on Unix you'd get a warning | ||
from the portability lint that you're calling an API not available on all | ||
mainstream platforms. There are basically three ways to react (all of which will | ||
make the warning go away): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably worth making it clear that there's a fourth option of explicitly suppressing the lint in cases where the other three options won't work for whatever reason.
I find this much nicer than scenarios; great job! |
For me it's super important that |
#[cfg(any(windows, unix))] | ||
fn portable() { | ||
// the expression here has portability `any(windows, unix)` | ||
match_cfg! { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this smarter than a normal macro_rules!
macro that expands to #[cfg(windows)] { windows_only() } #[cfg(unix)] { unix_only() }
would be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes -- using cfg
directly like that would not give you the any(windows, unix)
semantics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if we could macro it some other way, match_cfg
as a special primitive will make things faster by allowing formulae to be filtered prior to SAT-solving in the exhaustive match case.
text/0000-portability-lint.md
Outdated
|
||
The `std` portability will include several implications, e.g.: | ||
|
||
- `std` implies `any(windows, macos, linux)` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like we should be able to cover BSDs in a reasonable way in the base portability level, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
99% of the platform-specific code I've wound up writing in Rust is simply cfg(unix)
vs. cfg(windows)
, honestly. I know that for very low-level things you can wind up having to write code that's not POSIX, but I have a hard time thinking of an example that would crop up in the standard library.
evidence in either direction. But it is yet another place where the fact that | ||
it's a lint could help: we may be able to simply skip checking pathological | ||
cases, if they indeed arise in practice. In any case, it's hard to know how | ||
concerned to be until we try it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My impression is that modern solvers are good enough that if you run into an exponential case, you're in "doctor, it hurts when I do this" territory, particularly in a context as constrained as this one. People are not going to be feeding randomized cfgs containing hundreds of clauses into rustc (hopefully)!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's exponential in the number of variables, not the size of the formula (consider any formula can put into a normal form equivalent to a truth table, whose size is 2^num-vars
). Since all the variables are defined up-front (no features yet), so the number of crates/items doesn't impact the exponential, I am cautiously optimistic.
text/0000-portability-lint.md
Outdated
struct MyType; | ||
|
||
impl Foo for MyType { | ||
#[cfg(unix)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you'd probably want this on the overall impl, right? It seems like it'd be relatively easy to see that the impl is missing definitions in some cfgs here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if the annotations are restricted to impls themselves (rather than to impl items), then we can catch the lint violation here:
use_foo::<MyType>(); // `MyType: Foo` requires `unix`
^^^^^^
with good enough precision.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aturon @nikomatsakis I think we can even encode this in the trait system, as where cfg("unix")
(a custom kind of "predicate" that would get automatically propagated when impls are selected).
The lint could be emitted from the function type-inference/checking context, which tracks obligations (predicates that have to be satisfied) well enough to allow blaming the original source.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not the way I would think about it, but I agree it is probably something we can handle. I think it's more akin to a lifetime constraint. The idea would be to disallow these configuration constraints from affecting how inference works, but propagate them back as a kind of "side constraint" that the caller may wish to enforce. (This fits in with my newer prototypes for how the trait system should work.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(We are quite likely just saying the same thing here, though, in our own ways.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I was unclear I meant the where cfg(x)
as "was kept because cfg(x)
holds", not doing the actual gating on the fly, but remembering it was done. Though I should probably read the whole RFC before commenting, shouldn't I? 😆
text/0000-portability-lint.md
Outdated
As with many lints, the portability lint is *best effort*: it is not required to | ||
provide airtight guarantees about portability. In particular, the proposed | ||
definition in this RFC ignores trait implementations and dispatch. In practice, | ||
this hole shouldn't be too significant: it's pretty rare to implement a trait |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But... but... integer conversions!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very true. I guess this can still be future work?
standard primitives. For example `std::os::unix::io::AsRawFd` is a trait with | ||
the `as_raw_fd` method (to extract a file descriptor). If you were to ignore | ||
Windows, however, one might expect this API instead to live as a method | ||
directly on types like `File`, `TcpStream`, etc. Forcing code to live in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the running example of as_raw_fd
needing to be an inherent method. If you're on unix (which you are since you're calling as_raw_fd
), it makes perfect sense for all file descriptor wrapper types to implement a common trait. For example, if you're writing a crate that implements select/poll, you might want to be generic over AsRawFd
.
Same story applies to Windows/HANDLEs/WaitForMultipleObjects
text/0000-portability-lint.md
Outdated
provide airtight guarantees about portability. In particular, the proposed | ||
definition in this RFC ignores trait implementations and dispatch. In practice, | ||
this hole shouldn't be too significant: it's pretty rare to implement a trait | ||
conditionally based on platform information, and the goal of this RFC is to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this exactly what many things in the std::os
module do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That approach would presumably fall out of "style" if this was was implemented.
**What does it mean for a portability to be narrower?** In general, portability | ||
is a logical expression, using the operators `all`, `any`, `not` on top of | ||
primitive expressions like `unix`. Portability `P` is narrower than portability | ||
`Q` if `P` *implies* `Q` as a logic formula. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If attribute values are extended to support basic arithmetic operations in the future (to support version requirements like #[cfg(_MSC_VER >= 1600)]
and generally catch up with configuration abilities of C preprocessor), will it create additional problems for this scheme?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like it might require a subset (heh) of a SMT solver.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eh, if we are doing SAT, SMT is no big leap.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO the problem formulation for SAT is simple enough that you can use heuristics with brute-force worst-case, but for SMT you need a proper solver if you want to get anywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still need time to think the idea through, mostly thinking about corner cases. I’d like more examples and samples. I found missing from the RFC details such as where would the implications be stored. RFC says the cargo features get ignored, but does not say what happens with user specified cfg
s in general. Some details there would be nice.
but it's increasingly holding us back from enhancements we'd like to make, and | ||
even for the needs it covers, it's suboptimal in a few ways. | ||
|
||
**Problems with `std::os`**: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be level-3 header, not a bold text. Semantics matter.
text/0000-portability-lint.md
Outdated
definition in this RFC ignores trait implementations and dispatch. In practice, | ||
this hole shouldn't be too significant: it's pretty rare to implement a trait | ||
conditionally based on platform information, and the goal of this RFC is to | ||
*guide* code toward mainstream compatibility without imposing a heavy burden. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds like a serious problem to me and I feel like the wording here is downplaying the seriousness of this suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed - I feel like we should at least be able to set the lint to error to get hard gurantees, which means not having holes like that in it.
definitions and checks that the item's portability is *narrower* than the | ||
portability of items it references or invokes. For example, `bar` in the above | ||
could invoke an item with portability `unix` and/or `target_pointer_width = | ||
"32"`, but not one with portability `linux`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably meant “portability target_os="linux"
.”?
// portability `all(any(windows, unix), windows)` | ||
windows_only() | ||
} | ||
unix => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be reassuring to see a match_cfg
example with more complex CFGs. So far no examples of match_cfg
in conjunction with value-cfg (e.g. target_os = "linux"
) have been provided.
text/0000-portability-lint.md
Outdated
|
||
The `std` portability will include several implications, e.g.: | ||
|
||
- `std` implies `any(windows, macos, linux)` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noting again, that linux
and macos
are not cfg
that get set by compiler itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me like this implication would make any crate with target_os ≠ any("windows", "macos", "linux")
fire the lint on any crate whenever used in code specific to non-currently-tier-1 platform.
For example this code:
#[cfg(target_os = "freebsd")]
fn new_vec() -> Vec<u8> { Vec::new() }
So the body here is windows | macos | linux
but the function itself is target_os = "freebsd"
. Since target_os = "freebsd"
is not implied by any of the windows
, macos
or linux
, Vec::new()
is considered to be incompatible and gets linted. Am I right? If so, that sounds like a serious issue.
The way I see it, std portability should be derived from every supported target. Looking at platform support page, there’s a number of targets supported, I’ll pick a few:
x86_64-unknown-linux-gnu
=all(target_arch = "x86", target_os= "linux", target_vendor = "unknown", target_abi = "gnu", target_pointer_width = "64", ...)
,mips64-unknown-linux-gnuabi64
=all(target_arch = "mips64", target_os = "linux", target_vendor = "unknown", target_abi = "gnuabi64", target_pointer_width = "64", ...)
,thumbv7m-none-eabi
(std not supported) =not(all(target_arch = "...", target_os = "none", ...))
,- ...
and if any of those conditions above hold, then the std
portability is implied.
Now the question how do you specify it? #![cfg(...)]
of all these on src/libstd/lib.rs
makes no sense as there’s also a number of custom targets (e.g. x86_64-unknown-linux-{arbitrarylibcabi}
) on which libstd runs fine… probably… and wouldn’t anymore.
Eh, I wrote this whole comment and only remembered about non-normative nature of this section.
text/0000-portability-lint.md
Outdated
|
||
Another aspect of portability comparison is the relationship between things like | ||
`unix` and `linux`. In logical terms, we want to say that `linux` implies | ||
`unix`, for example. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noting again, that linux
is not a cfg that gets set by rustc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That being said, would be nice to see some expansion in this section wrt value-cfg (e.g. target_os = "linux"
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
key-value stuff should be transformable to black box constants with a disjointness axiom? At least in the target_os case we don't support arbitrary strings anyways which helps make that feasible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Ericson2314 you can write a custom target which would set a user-specified target_os string (there can’t be multiple possible values though, so your point stands). While I do not think it would be a problem to support key = value
cfgs, I would like to see expansion just for completeness sake.
# Alternatives | ||
[alternatives]: #alternatives | ||
|
||
The main alternatives are: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notably missing is the status quo.
I like the overall feeling of this RFC more than the scenarios proposal. I do have some lingering questions and remarks. It feels like we're trying to prove transitive properties of code. But we're using a lint instead of the type system. Wouldn't this make more sense to be encoded directly into types? What is the portability of
Lints are not enabled for dependent crates. How will I know if my dependencies meet the platform I'm targeting? There's nothing in this RFC about solving the cfg+rustdoc problem. That's probably intentional, but I think it should be brought up nonetheless. |
Glad to hear it!
There are several reasons to make it a lint. Most importantly, portability is not such a crucial property that we want to make strong guarantees and issue hard errors around it, I think. Being able to opt out of this checking seems desirable. Lints also give us greater flexibility to change the errors produced over time, which the RFC talks a bit about. It's also worth saying that there's not a fundamental distinction between lints and type checking; we can do arbitrary static analysis in the lint.
The portability is literally the
I'm not sure! Part of the reason I only sketched out the rollout for
Metadata will be recorded for portability in upstream crates, and thus checked in downstream crates.
Will do. |
Thanks to those who've already provided feedback! It looks like there's more interest in tracking trait usage than I expected. I left this out in the initial proposal because it didn't seem worth the complexity. I do think that limiting |
What about |
In general: I like it! A few thoughts though:
|
Thanks for writing this up @aturon! I know it's been a long haul getting here but I'm quite excited to see this RFC as I think it strikes a great balance with all the thoughts we've heard so far. One thing I always find useful is to game out common usage patterns and see what they would look like. First I figured it'd be good to take a look at the approach the standard library takes in the definition of In pub struct File {
inner: imp::File,
}
impl File {
pub fn cross_platform_method(&self) {
self.inner.cross_platform_method()
}
} The actual definition of
I think that the most ergonomic solution for this would be to disable the lint inside these modules, right? Basically In terms of other concrete cases I like the ergonomics of writing a platform-specific API. Once you're in a I personally feel that Ideally I think we could add a definition like https://is.gd/qvrAm0 which basically just expands to a bunch of cfg expressions on statements. The crucial piece that While writing this comment though @Kimundi brought up an interesting point that it might be pretty awesome if we can auto-detect something like this. I would imagine it's very hard to do (running resolution multiple times... kinda), but the benefits would be that my example above wouldn't need any modifications either. In terms of subsets, I feel like that's where the RFC gets very hand-wavy. Splicing out threads, compiling for emscripten, or trying to compile out floats I feel will need more thought than just applying the
|
`try_push` method provided via an extension trait). It's not clear how to do | ||
that with the current [facade] setup. | ||
|
||
* Kernels and embedded environments often want to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought this was already solved? Using the right codegen options removed the LLVM assertions that x86 devs were seeing by lowering float operations to intrinsics (software implementations) without having to modify the core
crate. cc @steveklabnik @phil-opp Does the #rust-osdev community still recommend patching core
to remove floats?
On the ARM Cortex-M side of embedded development, if your hardware doesn't have floating point instructions then you use, for example, the thumbv7em-none-eabi
target instead of the "hard float" variant: thumbv7em-none-eabihf
, which does have floating point instructions. No need to strip anything from the core
crate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most people use xargo
with the right options, yes. Or at least, I do, @phil-opp does, most people I've talked to do.
That still doesn't mean that it wouldn't be nice to have a more real solution here; IIRC, with the codegen options we use, if we use floating point, we get software emulation; I'd rather just straight-up outlaw it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@japaric still seems nice to allow people to float altogether if they don't need it, rather than fallback on soft-float but hope it isn't used.
@alexcrichton : I had the impression that implementing the auto detection would not be all that much harder a what has been proposed already. Basically, you'd just look for matching sets of differently-cfgs items with the same nominal names. What I can see as problematic is the need to descent down into each of them - but it sems like that is already a inherent complexity of the RFC as is? |
Wow; I'm really excited for this! I had a few times rambled about SAT for this, but, well, wasn't sure who heard. Three points that no one has brought up here yet:
All in all, I'm ecstatic :) |
Wanted to add that:
|
@pornel What kind of code patterns require |
@joshtriplett In general I'm really disappointed that Rust has very poor support for integer sizes other than I've tried using smaller, more efficient types, but amount of dangerous casting back and forth is unmaintainable. |
File sizes should use a Rust equivalent of |
@Kimundi I'm personally under the impression that inferring |
@aturon: I guess what I haven't really realized is that most situations in which this lint can miss portability issues are also situations where you either would get compiler errors on the problematic platforms anyway, or where all supported platforms already mutually imply portability considerations because there is a However, I think there is one scenario where this issue still applies: Platform-specific optional code. For example: // mycrate.rs
pub fn mycrate_function() {
// ...
}
#[cfg(windows)]
pub fn windows_specific_mycrate_function() {
// ...
windows_more_specific_mycrate_function();
}
#[cfg(all(windows, target_pointer_width = "64"))]
pub fn windows_more_specific_mycrate_function() {
// ...
} Compiling this on Of course the real issue here is that the crate author has written platform-specific code without testing it himself, but I can see that happening in practice due to things like CI services testing the code on all platforms, but not necessarily informing the author about warnings. |
That seems like the primary value here: taking an error you'd normally only see if you attempted to target that platform, and turning it into a lint you'll see on a platform where it'd otherwise pass silently. That helps people think more consciously about portability and avoid unintentional non-portability just because it works on the platform in front of them. |
@Kimundi I just pushed an additional commit to try to clarify the questions you were raising (which also includes your example). Please let me know if there are further issues that need to be addressed. |
@aturon: Looks good! |
In this section of the design it says that, faced with a non-portability warning, you can do one of three things:
But what if this crate is actually a non-portable crate? Take nix for example, which is an inherently UNIX specific crate. Do you take the third option? But the third option seems designed around the assumption that your code is actually portable, which is why it is simply |
@withoutboats Doesn't this solve your question? Use the 2nd option? #![cfg(unix)] |
@withoutboats -- @kennytm's comment is basically what I had in mind for platform-specific crates. There is an interesting question, though, about whether we need finer-grained allows in general for the lint. Personally I'd be inclined to punt on this for now, and see what happens in practice; we can easily add such a mechanism if needed. |
I guess that makes sense! I wonder if there's a better error message we could provide than just having the crate have no items in it though (that is, if you use a unix crate on windows). In any event that's the kind of minor improvement we can iterate on, marking my check. |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
The final comment period is now complete. |
This RFC has now been merged! Further discussion should happen on the tracking issue. Please let me know if you're interested in working on the implementation. |
Once we get |
Implement always-fallible TryFrom for usize/isize conversions that are infallible on some platforms This reverts commit 837d6c7 "Remove TryFrom impls that might become conditionally-infallible with a portability lint". This fixes rust-lang#49415 by adding (restoring) missing `TryFrom` impls for integer conversions to or from `usize` or `isize`, by making them always fallible at the type system level (that is, with `Error=TryFromIntError`) even though they happen to be infallible on some platforms (for some values of `size_of::<usize>()`). They had been removed to allow the possibility to conditionally having some of them be infallible `From` impls instead, depending on the platforms, and have the [portability lint](rust-lang/rfcs#1868) warn when they are used in code that is not already opting into non-portability. For example `#[allow(some_lint)] usize::from(x: u64)` would be valid on code that only targets 64-bit platforms. This PR gives up on this possiblity for two reasons: * Based on discussion with @aturon, it seems that the portability lint is not happening any time soon. It’s better to have the conversions be available *at all* than keep blocking them for so long. Portability-lint-gated platform-specific APIs can always be added separately later. * For code that is fine with fallibility, the alternative would force it to opt into "non-portability" even though there would be no real portability issue.
Implement always-fallible TryFrom for usize/isize conversions that are infallible on some platforms This reverts commit 837d6c7 "Remove TryFrom impls that might become conditionally-infallible with a portability lint". This fixes #49415 by adding (restoring) missing `TryFrom` impls for integer conversions to or from `usize` or `isize`, by making them always fallible at the type system level (that is, with `Error=TryFromIntError`) even though they happen to be infallible on some platforms (for some values of `size_of::<usize>()`). They had been removed to allow the possibility to conditionally having some of them be infallible `From` impls instead, depending on the platforms, and have the [portability lint](rust-lang/rfcs#1868) warn when they are used in code that is not already opting into non-portability. For example `#[allow(some_lint)] usize::from(x: u64)` would be valid on code that only targets 64-bit platforms. This PR gives up on this possiblity for two reasons: * Based on discussion with @aturon, it seems that the portability lint is not happening any time soon. It’s better to have the conversions be available *at all* than keep blocking them for so long. Portability-lint-gated platform-specific APIs can always be added separately later. * For code that is fine with fallibility, the alternative would force it to opt into "non-portability" even though there would be no real portability issue.
There has long been a desire to expand the number of platform- and architecture-specific APIs in the standard library, and to offer subsets of the standard library for working in constrained environments. At the same time, we want to retain the property that Rust code is portable by default.
This RFC proposes a new portability lint, which threads the needle between these two desires. The lint piggybacks on the existing
cfg
system, so that using APIs involvingcfg
will generate a warning unless there is explicit acknowledgment of the portability implications.The lint is intended to make the existing
std::os
module obsolete, to allow expansion (and subsetting) of the standard library, and to provide deeper checking for portability across the ecosystem.Rendered