[WIP] Add {HashMap,HashSet} to std's prelude #63418

tesuji · 2019-08-09T18:09:25Z

Centril · 2019-08-09T18:18:33Z

src/libstd/prelude/v1.rs

@@ -123,3 +123,6 @@ pub use crate::string::{String, ToString};
 #[stable(feature = "rust1", since = "1.0.0")]
 #[doc(no_inline)]
 pub use crate::vec::Vec;
+#[unstable(feature = "hashmap_prelude", issue = "0")]


@petrochenkov Does this have any effect? Probably not?

It has effect on documentation.
(It will also have effect if someone bothers to move stability checking to its proper place - resolve.)

src/test/ui/derived-errors/issue-31997-1.stderr

src/test/ui/error-codes/E0433.stderr

src/test/ui/resolve/use_suggestion_placement.stderr

src/libstd/prelude/v1.rs

tesuji · 2019-08-10T15:50:50Z

I cannot run the full test suite with x.py in local machine because of #63436.

tesuji · 2019-08-10T15:53:50Z

src/libstd/prelude/v1.rs

-// The file so far is equivalent to src/libcore/prelude/v1.rs,
-// and below to src/liballoc/prelude.rs.
+// The file so far is equivalent to `src/libcore/prelude/v1.rs`,
+// and below to `src/liballoc/prelude/v1.rs` (except `HashMap`).
 // Those files are duplicated rather than using glob imports
 // because we want docs to show these re-exports as pointing to within `std`.


Should I remove these comments or not? @RalfJung

Sorry, no idea. I just observed an inconsistency.

RalfJung · 2019-08-10T16:00:41Z

I cannot run the full test suite with x.py in local machine because of #63436.

I hardly ever run more than ./x.py test src/test/ui --pass check, then I let the PR CI point me to what else might be problematic. Things just take too long.^^

steveklabnik · 2019-08-13T13:19:12Z

I don't personally think HashMap is used often enough to deserve a spot in the prelude.

Centril · 2019-08-13T13:23:05Z

IMO we should rethink the prelude and not have it be so limited. I think the bar set by:

It's kept as small as possible, and is focused on things, particularly traits, which are used in almost every single Rust program.

is just too high. For example, I do not think that ToOwned meets that bar.

steveklabnik · 2019-08-13T13:24:23Z

I think that messing with something so important, and that affects imports to every single program, is something to be done with extreme care.

joshtriplett

Typo: HashpMap should be HashMap

joshtriplett · 2019-08-15T18:40:48Z

I'd love to see this happen.

I'm concerned about it generating warnings about redundant imports, as evidenced by the extensive test suite changes, but I don't see any reasonable way around that.

joshtriplett · 2019-08-15T18:41:46Z

Could you please rebase the series and squash some patches together to avoid fixing bugs introduced by the same series? For instance, you have several commits making the same kind of change to tests, and you have a commit changing a test incorrectly followed by a commit fixing the test correctly.

src/librustc_resolve/error_codes.rs

joshtriplett · 2019-08-15T19:16:39Z

We discussed this in the lang team meeting, and we developed a consensus that the lang team wants to defer decisions about the content of the prelude to the libs team, unless the libs team specifically wants to consult the lang team (such as for issues of coherence or alignment with new language features). As such, un-tagging the lang team.

joshtriplett · 2019-08-15T19:18:51Z

Now, taking my lang team hat off and speaking only for myself:

Yes, please, I'd love to see HashMap in the prelude. I'd also like to see a few other things added to the prelude that people commonly use.

We'll need to take some care about traits, but types should not cause issues to introduce.

ssokolow · 2019-08-16T05:51:55Z

For example, I do not think that ToOwned meets that bar.

I always use it when going from &str to String because it best communicates the purpose of the conversion to anyone else reading the code.

RalfJung · 2019-08-16T05:59:29Z

I always use it when going from &str to String because it best communicates the purpose of the conversion to anyone else reading the code.

I've been bitten by that quite badly when, for some reason, I ended up with an &&str. Then to_owned turns that into an &str.
Since then, I am using .to_string(), and for literals sometimes format!.

ssokolow · 2019-08-16T06:57:47Z

I'm having trouble imagining a situation where I'd use .to_owned(), that might happen, and the type system wouldn't catch it.

(If the borrow checker is happy with a reference, then why would I be calling .to_owned() in the first place?)

...and, in that case, I'd say that it's a good thing for the same reason that I don't want string+integer concatenation doing automatic type conversion.

As far as I'm concerned, if .to_string() or format! let an unexpected && Just Work™, then it's a design flaw because I want to catch, understand, and make an explicit decision about that unexpected &&.

RalfJung · 2019-08-16T07:27:14Z

I disagree, I think if what I want is a string I should express that -- "to owned" doesn't convey enough information about what we own then. This is related to accidentally cloning a shared reference instead of what it points to. But I'll stop digressing here now. ;)

ssokolow · 2019-08-16T08:47:45Z

Fair enough. I won't keep things off-topic either but I will say that I hadn't considered shared references.

(I'm still used to choosing project ideas where Rust is just serving as a more maintainable alternative to Python and, given Python's Global Interpreter Lock, that means I don't do much parallel stuff.)

chris-morgan · 2019-08-17T12:35:08Z

Quite apart from the question of what items are in the prelude: I had understood that if items were added to the prelude in future, they’d be in a different module than v1. Otherwise I’m not particularly sure what the purpose of the v1 bit would be.

* Fix `HashMap` redundantly import errors * Replace HashMap by BTreeMap in tests suggested by Jonas

JohnCSimon · 2019-08-24T02:13:58Z

Ping from triage
@Centril @lzutao Hi, this has sat idle for over a week, can you post the status on this?
thank you

Centril · 2019-08-24T12:48:25Z

r? @alexcrichton

alexcrichton · 2019-08-26T15:21:47Z

Thanks for the PR! I'm a bit lost reading this PR though. None of the description, the commits, or the discussion thread seem to discuss the motivation here for why HashMap should be added to the prelude at this time. Additionally modifying the prelude is such an important operation that we currently require an accepted RFC to land it. I don't think that's happened though?

bors · 2019-09-01T21:57:16Z

☔ The latest upstream changes (presumably #63870) made this pull request unmergeable. Please resolve the merge conflicts.

alexcrichton · 2019-09-04T17:09:31Z

I'm going to close this because this change needs an accepted RFC which hasn't happened, but feel free to ping if assistance is needed in writing an RFC!

rust-highfive assigned Centril Aug 9, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 9, 2019

Centril reviewed Aug 9, 2019

View reviewed changes

Centril added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Aug 9, 2019

This comment has been minimized.

Sign in to view

jonas-schievink suggested changes Aug 9, 2019

View reviewed changes

src/test/ui/derived-errors/issue-31997-1.stderr Outdated Show resolved Hide resolved

src/test/ui/error-codes/E0433.stderr Outdated Show resolved Hide resolved

src/test/ui/resolve/use_suggestion_placement.stderr Outdated Show resolved Hide resolved

tesuji force-pushed the hashmap-prelude branch 2 times, most recently from f1cb215 to b46f828 Compare August 10, 2019 05:02

This comment has been minimized.

Sign in to view

tesuji force-pushed the hashmap-prelude branch from fd466cb to ffb85f3 Compare August 10, 2019 13:08

RalfJung reviewed Aug 10, 2019

View reviewed changes

src/libstd/prelude/v1.rs Show resolved Hide resolved

This comment has been minimized.

Sign in to view

tesuji commented Aug 10, 2019

View reviewed changes

Centril mentioned this pull request Aug 14, 2019

Add HashMap<K, V> to the prelude #63574

Closed

joshtriplett reviewed Aug 15, 2019

View reviewed changes

src/librustc_resolve/error_codes.rs Show resolved Hide resolved

joshtriplett removed I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Aug 15, 2019

tesuji force-pushed the hashmap-prelude branch from ac72da7 to c066237 Compare August 16, 2019 01:17

tesuji added 3 commits August 19, 2019 12:56

Add HashMap to std prelude

e195181

* Fix `HashMap` redundantly import errors * Replace HashMap by BTreeMap in tests suggested by Jonas

Reword comment about alloc prelude

e351f05

Add HashSet to prelude

0fb8f5f

tesuji force-pushed the hashmap-prelude branch from c066237 to 0fb8f5f Compare August 19, 2019 15:08

Centril added I-nominated S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 24, 2019

rust-highfive assigned alexcrichton and unassigned Centril Aug 24, 2019

tesuji changed the title ~~[WIP] Add HashMap to std's prelude~~ [WIP] Add HashMap, HashSet to std's prelude Aug 24, 2019

tesuji changed the title ~~[WIP] Add HashMap, HashSet to std's prelude~~ [WIP] Add {HashMap,HashSet} to std's prelude Aug 24, 2019

alexcrichton closed this Sep 4, 2019

tesuji deleted the hashmap-prelude branch May 20, 2024 00:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Add {HashMap,HashSet} to std's prelude #63418

[WIP] Add {HashMap,HashSet} to std's prelude #63418

tesuji commented Aug 9, 2019

Centril Aug 9, 2019

petrochenkov Aug 9, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

tesuji commented Aug 10, 2019

tesuji Aug 10, 2019

RalfJung Aug 10, 2019

RalfJung commented Aug 10, 2019

steveklabnik commented Aug 13, 2019

Centril commented Aug 13, 2019

steveklabnik commented Aug 13, 2019

joshtriplett left a comment

joshtriplett commented Aug 15, 2019

joshtriplett commented Aug 15, 2019 •

edited

Loading

joshtriplett commented Aug 15, 2019

joshtriplett commented Aug 15, 2019

ssokolow commented Aug 16, 2019

RalfJung commented Aug 16, 2019

ssokolow commented Aug 16, 2019 •

edited

Loading

RalfJung commented Aug 16, 2019

ssokolow commented Aug 16, 2019 •

edited

Loading

chris-morgan commented Aug 17, 2019

JohnCSimon commented Aug 24, 2019

Centril commented Aug 24, 2019

alexcrichton commented Aug 26, 2019

bors commented Sep 1, 2019

alexcrichton commented Sep 4, 2019

[WIP] Add {HashMap,HashSet} to std's prelude #63418

[WIP] Add {HashMap,HashSet} to std's prelude #63418

Conversation

tesuji commented Aug 9, 2019

Centril Aug 9, 2019

Choose a reason for hiding this comment

petrochenkov Aug 9, 2019

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

tesuji commented Aug 10, 2019

tesuji Aug 10, 2019

Choose a reason for hiding this comment

RalfJung Aug 10, 2019

Choose a reason for hiding this comment

RalfJung commented Aug 10, 2019

steveklabnik commented Aug 13, 2019

Centril commented Aug 13, 2019

steveklabnik commented Aug 13, 2019

joshtriplett left a comment

Choose a reason for hiding this comment

joshtriplett commented Aug 15, 2019

joshtriplett commented Aug 15, 2019 • edited Loading

joshtriplett commented Aug 15, 2019

joshtriplett commented Aug 15, 2019

ssokolow commented Aug 16, 2019

RalfJung commented Aug 16, 2019

ssokolow commented Aug 16, 2019 • edited Loading

RalfJung commented Aug 16, 2019

ssokolow commented Aug 16, 2019 • edited Loading

chris-morgan commented Aug 17, 2019

JohnCSimon commented Aug 24, 2019

Centril commented Aug 24, 2019

alexcrichton commented Aug 26, 2019

bors commented Sep 1, 2019

alexcrichton commented Sep 4, 2019

joshtriplett commented Aug 15, 2019 •

edited

Loading

ssokolow commented Aug 16, 2019 •

edited

Loading

ssokolow commented Aug 16, 2019 •

edited

Loading