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

RFC: A portability lint #1868

Merged
merged 8 commits into from
Apr 29, 2017
Merged

RFC: A portability lint #1868

merged 8 commits into from
Apr 29, 2017

Conversation

aturon
Copy link
Member

@aturon aturon commented Jan 23, 2017

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 involving cfg 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

@aturon
Copy link
Member Author

aturon commented Jan 23, 2017

cc @rust-lang/libs: this is what the "scenarios" idea evolved into.

@aturon aturon added T-libs-api Relevant to the library API team, which will review and decide on the RFC. T-lang Relevant to the language team, which will review and decide on the RFC. labels Jan 23, 2017
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):
Copy link
Member

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.

@steveklabnik
Copy link
Member

I find this much nicer than scenarios; great job!

@kornelski
Copy link
Contributor

kornelski commented Jan 23, 2017

#[cfg()] based solution looks good for system-specific APIs. But I'm worried about traits being handwaved in the proposal.

For me it's super important that let u64 = usize.into() and let usize = u32.into() work on mainstream platforms. Currently they don't, because 16/128-bit platforms couldn't implement them.

#[cfg(any(windows, unix))]
fn portable() {
// the expression here has portability `any(windows, unix)`
match_cfg! {
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Contributor

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.


The `std` portability will include several implications, e.g.:

- `std` implies `any(windows, macos, linux)`
Copy link
Member

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?

Copy link

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.
Copy link
Member

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)!

Copy link
Contributor

@Ericson2314 Ericson2314 Jan 25, 2017

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.

struct MyType;

impl Foo for MyType {
#[cfg(unix)]
Copy link
Member

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.

Copy link
Contributor

@petrochenkov petrochenkov Jan 23, 2017

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.

Copy link
Member

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.

Copy link
Contributor

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.)

Copy link
Contributor

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.)

Copy link
Member

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? 😆

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But... but... integer conversions!

Copy link
Contributor

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
Copy link
Contributor

@jethrogb jethrogb Jan 23, 2017

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

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
Copy link
Contributor

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?

Copy link
Member

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.
Copy link
Contributor

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?

Copy link
Member

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.

Copy link
Contributor

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.

Copy link
Member

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.

Copy link
Member

@nagisa nagisa left a 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 cfgs 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`**:
Copy link
Member

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.

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.
Copy link
Member

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.

Copy link
Member

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`.
Copy link
Member

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 => {
Copy link
Member

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.


The `std` portability will include several implications, e.g.:

- `std` implies `any(windows, macos, linux)`
Copy link
Member

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.

Copy link
Member

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.


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.
Copy link
Member

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.

Copy link
Member

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")

Copy link
Contributor

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.

Copy link
Member

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:
Copy link
Member

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.

@jethrogb
Copy link
Contributor

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 cfgs that included both portability statements and other cfgs? e.g. what is the portability of #[cfg(any(feature="windows_compat",windows))]?

windows is currently defined as not(unix). Will we keep this definition? Will the portability lint solver know about this definition for purposes of exhaustiveness?

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.

@aturon
Copy link
Member Author

aturon commented Jan 23, 2017

@jethrogb

I like the overall feeling of this RFC more than the scenarios proposal.

Glad to hear it!

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?

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.

What is the portability of cfgs that included both portability statements and other cfgs? e.g. what is the portability of #[cfg(any(feature="windows_compat",windows))]?

The portability is literally the cfg pattern (so here, any(feature="windows_compat",windows)). This is why in general comparing them involves SAT solving -- they can involve arbitrary combinations of and, or, and not.

windows is currently defined as not(unix). Will we keep this definition? Will the portability lint solver know about this definition for purposes of exhaustiveness?

I'm not sure! Part of the reason I only sketched out the rollout for std is that there are a lot of such questions, but I think they are by and large policy questions that can be separated from the basic mechanism. We might want to nail down some of that policy before landing this RFC, but I wanted to focus discussion first on the basic mechanism.

Lints are not enabled for dependent crates. How will I know if my dependencies meet the platform I'm targeting?

Metadata will be recorded for portability in upstream crates, and thus checked in downstream crates.

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.

Will do.

@aturon
Copy link
Member Author

aturon commented Jan 23, 2017

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 cfg to just the impl itself, and not items within it, makes good sense, and is plausible to fit into selection. I'll try to give it some deeper thought soon.

@jethrogb
Copy link
Contributor

What about cfgs on statements/expressions?

@Kimundi
Copy link
Member

Kimundi commented Jan 24, 2017

In general: I like it!

A few thoughts though:

  • I generally agree that it should be made to work with traits and impls. While I understand that defining it as a lint allows ignoring the warning, I'd want to be able to opt-in to making the lint an error to get hard guarantees. I see the idea behind scenarios as a "(opt-in) trait system for APIs", which requires the lint to be exact and cover everything.

  • Is there an actual need for match_cfg? I can see that it is convenient to have, but it seems like its logic could be done implicitly:

    #[cfg(unix)]
    fn foo() {
        // ...
    }
    
    #[cfg(windows)]
    fn foo() {
        // ...
    }
    
    fn calls_foo() {
        foo(); // detect as any(unix, windows)
    }

    The basic idea would be to have the "overloaded" use of the same name as a mechanism to merge the portability requirements.

  • This proposal has moved away from the term "scenario" because it simplified down to a extension on top of the cfg system. As far as the actual mechanism is concerned, I think that is fine. However, I think that if we attempt to properly extend this proposal to be more general across the crate ecosystem, eg by integrating cargo features and the "implications" definitions, we might end up with an annotation-based system on top of cfg variables that would be complex enough to end up being called "scenario" again. 😉

  • To expand on the previous point: In general this proposal seems to head into the direction of the sketch I did in the original "scenarios" thread here. My basic idea there was to have scenarios as a "extended" cfg system, with the distinction between the two being that a scenario, amongst other things,

    • gets explicitly declared and namespaced to a crate, so that there is no "global" cfg space.
    • can define "implications" and aliases.

    I feel that something like that is the logical conclusion we would want to draw from this RFC if we want to generalize the "std" cfg "implications" to be definable in arbitrary crates for arbitrary new cfg flags and/or cargo-features. Specifically, having a global namespace for cfg names is bad if arbitrary crates might want to define their own scenarios/cfg subsets/portability spaces.

    Concretely this would involve a way to "promote" a global cfg flag or cargo feature to a crate-owned scenario by something like

    // std
    #![define_scenario(unix)]
    #![define_scenario(linux), all(unix, ...)]
    
    // custom crate
    #![define_scenario(feature="foo")]

@alexcrichton
Copy link
Member

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 std::fs. I've seen the same pattern across the ecosystem, such as the mio crate, as well.

In std::fs we've got (roughly):

pub struct File {
    inner: imp::File,
}

impl File {
    pub fn cross_platform_method(&self) {
        self.inner.cross_platform_method()
    }
}

The actual definition of imp::File is platform-specific and so will have a #[cfg] annotation leading up to it. For the cross-platform File to actually use the inner version it then needs to say that it's abstracting over platforms. That leads me to two questions:

  1. For the type definition, I don't think this RFC provides a solution other than allowing the lint, right?
  2. For the method we could use match_cfg!, but I feel like that'd get burdensome. Each arm of the match would be the same (self.inner.cross_platform_method()) and this'd be repeated for all methods (quite a few here!).

I think that the most ergonomic solution for this would be to disable the lint inside these modules, right? Basically std::fs would have #![allow(nonportability)] at the top?

In terms of other concrete cases I like the ergonomics of writing a platform-specific API. Once you're in a #[cfg(unix)] you've unlocked the whole Unix world of APIs for free, which semantically feels right.


I personally feel that match_cfg! may not wish to be built into the compiler/standard library from the get go. If we like the ergonomics of it then I think we should add it on that basis, but not as a fundamental piece of the puzzle for the platform compatibility lint.

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 match_cfg! does in this RFC though which a crate implementation wouldn't do is propagate the any business. That is, it wouldn't propagate that the expression is the any of the possible arms.

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 #[cfg] lint we're concocting here. For example, some questions I might have are:

  • How do I tag a crate as emscripten-compatible? If the default set of enabled cfg is the standard set, then any API I export will require the threads cfg, right? Put another way, I think the goal here is to compile for Linux but get warnings about emscripten incompatibility? How would that work?
  • What does the API evolution story look like for excising a portion of the standard library for a platform? Even if it's a lint it'd be good to have in mind what the warnings would look like.

`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
Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor

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.

@Kimundi
Copy link
Member

Kimundi commented Jan 24, 2017

@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?

@Ericson2314
Copy link
Contributor

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:

  1. I hope we can eventually work with Cargo features too? I've talked about packages defining axioms for their features so both rustc and cargo know if some features are mutually exclusive, etc. I'm not to worried about this, as the warning/lint based approach avoids any backwards comparability problems. (#[deny(foo) voids any must-continue-to-compile guarantees, right?) That means we can incrementally approach soundness and completeness.

  2. Be careful with the axioms on the built-in cfgs. For example, @eternaleye has many time pointed out that http://midipix.org/ and friends violate nand(unix, windows).

  3. Whether or not this becomes more popular to use than the facade, I think we should still push the facade internally. I'd argue smaller, finer-grained, crates are easier for new contributors to work with, and furthermore, being able to soundly and completely do a #![cfg(..)] for the entire crate as would be the case with under-the-facade crates is nice.

All in all, I'm ecstatic :)

@eddyb
Copy link
Member

eddyb commented Jan 25, 2017

violate nand(unix, windows).

Wanted to add that:

  • those are synonyms for target_family values (target specs allow any value)
  • Redox also violates assumptions like that

@joshtriplett
Copy link
Member

@pornel What kind of code patterns require let u64 = usize.into() to work? Why can that code not just use usize in place of u64?

@kornelski
Copy link
Contributor

kornelski commented Jan 25, 2017

@joshtriplett usize can't be used for file sizes for example (because 32-bit systems can work with >4GB files) (doesn't matter whether that's u64, long long, off_t, NSUInteger, they are all second class citizens in Rust). Portability lints would be fantastic here, because I use a 64-bit system and I wouldn't discover in casual testing if I truncated values accidentally on 32-bit.

In general I'm really disappointed that Rust has very poor support for integer sizes other than usize. I work with C APIs a lot (rewriting C or gluing C codebases together is the no.1 use-case for Rust for me). The other .into() case I've mentioned is far worse, because C uses int for almost everything, and Rust uses usize for almost everything, so they aren't interoperable without casting.

I've tried using smaller, more efficient types, but amount of dangerous casting back and forth is unmaintainable.

@joshtriplett
Copy link
Member

joshtriplett commented Jan 25, 2017

@pornel

usize can't be used for file sizes for example (because 32-bit systems can work with >4GB files).

File sizes should use a Rust equivalent of off_t, not u64.

@alexcrichton
Copy link
Member

@Kimundi I'm personally under the impression that inferring any sets based on what you might call would be a significant change to the compiler (but I'd love to be wrong!). Historically at least #[cfg] processing happens very early in the compiler, even before name resolution. This means that you don't actually know what you could have called by the time you know what you actually called.

@Kimundi
Copy link
Member

Kimundi commented Mar 7, 2017

@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 cfg for each of them.

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 unix would cause no error, compiling it on 64bit windows would cause no error but give a portability warning.

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.

@joshtriplett
Copy link
Member

@Kimundi

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

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.

@aturon
Copy link
Member Author

aturon commented Mar 10, 2017

@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.

@Kimundi
Copy link
Member

Kimundi commented Mar 11, 2017

@aturon: Looks good!

@withoutboats
Copy link
Contributor

In this section of the design it says that, faced with a non-portability warning, you can do one of three things:

  • Decide not to use the API, after discovering that it would reduce portability.
  • Decide to use the API, putting the function using it within a cfg(unix) as well (which will flag that function as Unix-specific).
  • Decide to use the API in a cross-platform way, e.g. by providing a Windows version of the same functionality. In that case you allow the lint, explicitly acknowledging that your code may involve platform-specific APIs but claiming that all platforms of the current cfg are handled. (See the appendix at the end for a possible extension that does more checking).

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 #[allow(nonportable)]. What if what you want is to specifically be "assume this crate is UNIX, but continue to lint for every other portability issue( e.g. pointer size)?"

@kennytm
Copy link
Member

kennytm commented Apr 12, 2017

@withoutboats Doesn't this solve your question? Use the 2nd option?

#![cfg(unix)]

@aturon
Copy link
Member Author

aturon commented Apr 17, 2017

@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.

@withoutboats
Copy link
Contributor

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.

@rfcbot rfcbot added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Apr 17, 2017
@rfcbot
Copy link
Collaborator

rfcbot commented Apr 17, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot
Copy link
Collaborator

rfcbot commented Apr 27, 2017

The final comment period is now complete.

@aturon
Copy link
Member Author

aturon commented Apr 29, 2017

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.

@joshlf
Copy link
Contributor

joshlf commented Feb 6, 2018

Once we get cargo to be better at cross-compiling, a more complete approach would be to simply run cargo check for every target platform for which you're worried about compatibility. It's probably expensive enough to do that that it'd be worth having it be a separate step that you can run (and thus it wouldn't obsolete the portability lints proposed here), but it would solve the problem of the portability lint having some edge cases in which it doesn't detect non-portable code.

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Jul 2, 2018
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.
bors added a commit to rust-lang/rust that referenced this pull request Jul 3, 2018
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.
@Centril Centril added the A-cfg Conditional compilation related proposals & ideas label Nov 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cfg Conditional compilation related proposals & ideas final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-lang Relevant to the language team, which will review and decide on the RFC. T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.