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

Unstabilize Arc and Weak #432

Closed
wants to merge 1 commit into from
Closed

Unstabilize Arc and Weak #432

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Nov 1, 2019

Two users on Discord have been confused by the existence of async_std::sync::Arc since they expected it to be somehow different from std::sync::Arc, but it's just a re-export. I share similar feelings. Perhaps it'd be wise to unstabilize it before publishing 1.0, just in case.

To elaborate a bit more... It's "obvious" to me how async_std::fs::File or async_std::sync::Mutex are different from their std counterparts. While we do re-export a bunch of types like std::fs::FileType or std::net::Shutdown, those are not important and only exist as auxiliary types supporting those that are important. In my mind, Arc is not such an auxiliary type, which is probably why it feels odd to have it re-exported. But that's just my mental model of the library :)

I should also note that it's possible I (and perhaps other users) will get warmed up to the idea of this re-export but it just doesn't sit right with me right now.

Opinions? How do you think about all this? What should we do here?
cc @yoshuawuyts @skade @matklad @taiki-e

@skade
Copy link
Collaborator

skade commented Nov 1, 2019

I agree with that reasoning. There's a case to be made that you should be able to write all async code just by using async-std and Arc is kind of an async concern, but I disagree for 2 reasons:

  • Most actual codebases will import some amount of code from std
  • A pure re-export runs into danger of people accidentally believing async-stds Arc is different from std's Arc, leading to confusion with API that expect an Arc passed.

Copy link
Collaborator

@skade skade left a comment

Choose a reason for hiding this comment

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

I approve, but I'd like a second approval from @yoshuawuyts

@yoshuawuyts
Copy link
Contributor

yoshuawuyts commented Nov 1, 2019

I strongly disagree with this direction. As I stated in #403 (comment) ("pedagogy") I think async-std would overall benefit from clearer enumeration of which APIs we export, which APIs are adaptations of std APIs, and which APIs we've designed ourselves.

Usability

Mutex is practically never used without Arc, and by splitting the two into separate imports we harm the overall usability of async-std. An example of the awkwardness of this can be seen in #419's client.rs:

use std::sync::Arc;
use async_std::{
io::{stdin, BufReader},
net::{TcpStream, ToSocketAddrs},
prelude::*,
task,
future::select,
};

The single import from std stands out. This will be a recurring pattern in async-std user's code if we follow through with this proposed change.

I don't think the right conclusion for us is to feature-flag a std re-export, but instead ask clearer questions on how we can communicate the nature of the types we provide. A primary goal of async-std is to enable people to be productive with Async Rust, and I believe this change would regress usability without addressing the underlying cause.

Feedback asked

Additionally I think saying that "two users were confused on Discord" is not an accurate characterization of the conversations that were had. The fact that the conversations ended in asking a binary choice of: "Would you want to keep or remove Arc" is a wrong way to solicit feedback.

[stjepan to leo60228] given that you saw the discussion, would you like to have Arc in async-std or not?

[stjepan to axie] what is in your opinion better? keeping Arc in async_std::sync or removing it from there?

Instead I think we should have asked what is confusing people. Which assumptions did they have, and how was reality different? What did they think we could improve, rather than offer a single choice that they could either agree or disagree with.

Conclusion

I don't think this is the right direction for us, and I'm confused about the framing of the problem. Perhaps from a Rust expert's point of view Mutex, SocketAddr, and Arc can somehow be classified as primary / secondary. But I don't think that framing translates well to the wider Rust audience. Regardless of classification, people will want to know what we've defined, and what's coming from the stdlib.

The reasoning for rejecting #403 has made me wary of marking these APIs as "unstable" because I feel the burden of proof will then be turned into "please provide reasons why we should include this" rather than assuming it makes sense for us to include re-exports from std to achieve parity.

Any confusion we may have perceived from users so far has been symptoms of an underlying issue that we need to communicate better, and unstabilizing these APIs will not change the root cause.

I don't think we'll be able to agree on a direction on this thread, and it's something we should discuss in a more synchronous manner before we release 1.0. I don't want a decision on this PR to be based on whether it's stalled / pushed through -- but instead would like us to align on overall design direction so we can create consensus on where we want to take this. So perhaps let's take some time to sit down and talk this week?

@nbdd0121
Copy link
Contributor

nbdd0121 commented Nov 2, 2019

Can we just remove #[doc(inline)], so that it communicates clearly to the user that they are re-exported?

@taiki-e
Copy link
Contributor

taiki-e commented Nov 3, 2019

The single import from std stands out. This will be a recurring pattern in async-std user's code if we follow through with this proposed change.

If there are multiple imports from std, I think it does not look bad. e.g...

use std::{
    collections::HashMap,
    sync::{atomic::AtomicUsize, Arc}, 
};

use async_std::{ 
    io::{stdin, BufReader}, 
    prelude::*, 
    sync::Mutex, 
}; 

@ghost
Copy link
Author

ghost commented Nov 6, 2019

@yoshuawuyts and I discussed this and we'd close this PR and leave Arc and Weak as they are.

If you have strong feelings about this, now is the time to speak up. Otherwise, we'll close it. :)

@yoshuawuyts
Copy link
Contributor

We've talked about this in person, and we're not going to go forward with this at this time. Thanks all!

@yoshuawuyts yoshuawuyts closed this Nov 9, 2019
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

Successfully merging this pull request may close these issues.

4 participants