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

convention issue: structs with all pub fields are a hazard and should be avoided in libstd #22045

Closed
pnkfelix opened this issue Feb 7, 2015 · 14 comments
Assignees

Comments

@pnkfelix
Copy link
Member

pnkfelix commented Feb 7, 2015

Tuple structs are convenient at times, but they present issues for forward compatibility, especially (but not only) when they carry all pub fields

Example: std::ptr::Unique is currently defined as follows:

#[unstable(feature = "core", reason = "recently added to this module")]
pub struct Unique<T: ?Sized>(pub *mut T);

This is unfortunate, since assuming rust-lang/rfcs#738 is accepted (namely the PhantomData part, which I need for rust-lang/rfcs#769), we really should add PhantomData<T> as a member of Unique<T>.

So, why did we use tuple structs instead of a struct with private named fields?

I suspect it was for the convenience of the person authoring the library when it was first made. But these details end up leaking out to the users of these modules, and there does not seem to be much reason for it. The main use of a tuple-struct in a case like this is for composability with pattern matching, but I do not think that use outweighs the cost of being stuck with an API that hinders future changes to the internals of the stdlib.


Another related case where this comes up: we currently define std::boxed::Box as a tuple struct. Now, its only field is non-pub. However, the use of a tuple struct here puts Box itself into the value namespace, and thus the name Box in the value namespace of the prelude is essentially unavailable for any useful purpose. I think this is a bug in that API design; see my comment here: rust-lang/rfcs#809 (comment)

UPDATE: I have been convinced that tuple structs with a non-pub field are not a real hazard (see reasoning in my comment below).

@pnkfelix
Copy link
Member Author

pnkfelix commented Feb 7, 2015

(not sure if this belongs here or in the RFC's repo, but I definitely want it to get attention before the beta release.)

@pnkfelix pnkfelix changed the title convention issue: pub tuple structs are a hazard and should be avoided in libstd convention issue: tuple structs are a hazard and should be avoided in libstd Feb 7, 2015
@pnkfelix
Copy link
Member Author

pnkfelix commented Feb 7, 2015

cc @aturon

@pnkfelix
Copy link
Member Author

pnkfelix commented Feb 7, 2015

cc @nikomatsakis (whom I think has similar concerns regarding the over-use of tuple-structs in libstd).

@edwardw
Copy link
Contributor

edwardw commented Feb 7, 2015

cc me

edwardw added a commit to edwardw/rust that referenced this issue Feb 8, 2015
It is benign and could be activated by individual crates.

cc rust-lang#22045
@pnkfelix
Copy link
Member Author

actually, maybe a tuple struct with some non-pub fields is still not an issue for forward-compatibility, if it is true that it is always sound to replace such a tuple struct definition with a non-tuple struct definition, by the logic that the client-side end user could not have actually been invoking the constructor that had been put into the value namespace.

@Gankra
Copy link
Contributor

Gankra commented Feb 11, 2015

The proliferation of tuple structs is imo the consequence of wrapping and newtyping being tedious. foo.0.blah() and foo = Blah(bar) are nicer to deal with than the struct equivalents. The "anonymous" 0 field is also easier to recall than some random name that made sense at the time (inner? ptr? buf? data?). I have similar gripes with mandatory names on associated types for one-type traits. e.g. Deref, Iterator, Reader, etc.

@aturon
Copy link
Member

aturon commented Feb 11, 2015

@pnkfelix FWIW, I don't think that we've stabilized any public tuple structs in std. I will check.

@aturon
Copy link
Member

aturon commented Feb 11, 2015

The only places this shows up publically in std are:

  • Unique, which was recently added and has not been stabilized,
  • simd, which is wholly unstable and contains tuple structs that represent vectors

@pnkfelix
Copy link
Member Author

@aturon Great! Then it really is just a conventions issue, and not something that requires revising existing code.

(That's assuming that it is sound for one to switch from a tuple struct with private data to a non-tuple struct, which we do have instances of in libstd, such as Box, as I noted above.)

@nikomatsakis
Copy link
Contributor

Note that Unique (in my variance branch) is changed to Unique::new.

@pnkfelix
Copy link
Member Author

(will move to RFC repo)

@ftxqxd
Copy link
Contributor

ftxqxd commented Feb 24, 2015

(Sorry about commenting on a closed issue, but it doesn’t seem to have been moved to the RFCs repo yet.) The motivation for this convention as stated in the description seems to me like it applies to non-tuple structs, too. If the old Unique, without a marker field, were defined as a non-tuple struct instead, as so…

pub struct Unique<T: ?Sized> {
    pub ptr: *mut T,
}

…how would that be any better forward-compatibility-wise? Adding a field (private or not) would still break code that constructs or pattern-matches on it. This seems to be exactly the same situation as for tuple structs. The main problem here as I see it is with privacy—the ptr field should be private—but tuple structs can have private fields too.

I understand that there is a problem that exists with tuple structs (even with private fields) about polluting the value namespace as well as the type one, but that seems like a relatively minor problem that doesn’t affect backward-compatibility. I’ve filed rust-lang/rfcs#902 as a way of solving that for tuple structs with private fields.

@pnkfelix
Copy link
Member Author

@P1start yes in in my head I was focused on the effect on the value namespace, but the "hazard" is really for structs with all pub fields. Perhaps a tempest in a teapot

@huonw
Copy link
Member

huonw commented Mar 2, 2015

@pnkfelix FWIW, I don't think that we've stabilized any public tuple structs in std. I will check.

There is std::sync::mpsc::SendError, which is stable.

@pnkfelix pnkfelix changed the title convention issue: tuple structs are a hazard and should be avoided in libstd convention issue: structs with all pub fields are a hazard and should be avoided in libstd Dec 30, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants