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

portability risk: impls for libstd type can be platform dependent #22228

Closed
pnkfelix opened this issue Feb 12, 2015 · 8 comments
Closed

portability risk: impls for libstd type can be platform dependent #22228

pnkfelix opened this issue Feb 12, 2015 · 8 comments
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc P-medium Medium priority
Milestone

Comments

@pnkfelix
Copy link
Member

Our cfg system makes it a bit too easy for a given type to implement a given libstd trait on one platform and not implement that trait on a different platform. This risk is especially high for default impls, where a negative impl may be present for a field of a given abstraction but not on another.

According to this reddit post, this problem manifested itself for the Process type, which was/is Send on non-windows platforms but is not Send on windows, due to the use of a *mut () field in the windows version.

Now, the obvious answer here is "testing!" But we might want to try to go further.

E.g., we could implement an analysis that pulls in the libstd code under every target cfg, and then compares all of the pub types to ensure that they are all consistent with respect to which pub traits they implement.

@pnkfelix
Copy link
Member Author

background discussion on #rust-internals here: https://botbot.me/mozilla/rust-internals/2015-02-12/?msg=31827404&page=7

@pnkfelix
Copy link
Member Author

nominating, as I am not sure whether the presence/absence of the current set of default impls in libstd represents a potential forward-compatibility risk for libstd. (I.e., if we are sure that it will be safe to add an impl of e.g. Send after the fact for Process on windows, then we need not try to address this any time soon; we can just deal with each occurrence as it arises for the time being.)

@pnkfelix
Copy link
Member Author

cc @aturon

@brson
Copy link
Contributor

brson commented Feb 12, 2015

Currently Process implements Send on Unix and not on Windows: http://internals.rust-lang.org/t/what-rate-of-importability-do-we-allow-in-libstd/1556/4

@brson brson added A-testsuite Area: The testsuite used to check the correctness of rustc A-infrastructure labels Feb 12, 2015
@brson
Copy link
Contributor

brson commented Feb 12, 2015

Calls for some automated testing infra.

@brson brson mentioned this issue Feb 12, 2015
65 tasks
@pnkfelix
Copy link
Member Author

we should do some kind of audit before 1.0 itself, though it can happen after 1.0 beta.

Polish issue, 1.0, P-high.

@pnkfelix pnkfelix added this to the 1.0 milestone Feb 12, 2015
@pnkfelix pnkfelix added P-medium Medium priority and removed I-nominated labels Feb 12, 2015
@Diggsey
Copy link
Contributor

Diggsey commented Feb 12, 2015

There's a similar issue with rustdoc, in that it can only document a single platform at a time. If the "cfg" system could be extended, it could solve both issues in one go.

I'm envisaging something that would allow compiling multiple sets of "cfg" parameters in one go: this could be used to output documentation for all platforms, or to compare the non-platform-specific parts to check that they implement the same set of traits across all "cfg" variations.

Whether or not it's done that way, I think this would be a good way of telling the compiler about cfg options:

  • A module attribute, "[no_cfg()]" is applied to modules which re-export platform-specific types in a non-platform-specific way.
  • The compiler can check that modules marked in this way export identical types under every possible value for that config argument.
  • To ensure the module attribute is applied wherever appropraite, the compiler can automatically find publicly visible types which are platform specific, (ie. depend on a "cfg" attribute, and haven't been re-exported as "no_cfg")

@pnkfelix
Copy link
Member Author

pnkfelix commented Apr 2, 2015

an audit was done. closing.

@pnkfelix pnkfelix closed this as completed Apr 2, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc P-medium Medium priority
Projects
None yet
Development

No branches or pull requests

3 participants