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

Put Vec in with the rest of the collections #34944

Closed
wants to merge 3 commits into from

Conversation

notriddle
Copy link
Contributor

Tracking issue: #34943

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@apasel422 apasel422 added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jul 20, 2016
@apasel422
Copy link
Contributor

This appears to have caused some tests to fail by changing the path displayed in some error message, e.g.

unexpected errors (from JSON output): [
    Error {
        line_num: 34,
        kind: Some(
            Note
        ),
        msg: "34:30: 34:37: a collection of type `std::option::Option<std::collections::Vec<u8>>` cannot be built from an iterator over elements of type `&u8`"
    }
]
not found errors (from test file): [
    Error {
        line_num: 34,
        kind: Some(
            Note
        ),
        msg: "a collection of type `std::option::Option<std::vec::Vec<u8>>` cannot be built from an iterator over elements of type `&u8`"
    }
]

@notriddle
Copy link
Contributor Author

We don't want to refer to the new path until the re-export is stabilized. Should I just mark it stable and update the tests, or should I change that lint so it'll prefer the stable path?

@petrochenkov
Copy link
Contributor

petrochenkov commented Jul 21, 2016

@notriddle
Reexports are insta-stable currently (if the reexported item is stable), try to access std::collections::Vec from stable code - it will compile.

@notriddle
Copy link
Contributor Author

Alright, then I'll just mark it stable and fix the test cases. I guess we really don't need a trial period to determine how this will impact the community anyway.

@alexcrichton
Copy link
Member

@notriddle thanks for the PR! Because Vec is such a widely use collection, however, deprecation related to it shouldn't be taken lightly. To that end I think this may have a difficult time landing with a #[rustc_deprecated] tag, but in any case the libs team should hopefully get around to discussing this at triage next Monday

@ollie27
Copy link
Member

ollie27 commented Jul 22, 2016

This causes a couple of problems with the docs:

@notriddle
Copy link
Contributor Author

Most uses of Vec are through the prelude, anyway, so the deprecation won't do anything to the vast majority of client code. The only change this really makes is to reorganize the documentation.

And the relative links problem already needs to be solved.

@alexcrichton
Copy link
Member

The libs team got a chance to talk about this during triage yesterday, but our conclusion was that we'd prefer to not merge this for now. The Vec collection, like strings and slices, is essentially important enough to have its own module and also is unique from other std::collections collections in that it's in the prelude. That essentially means that no one ever imports Vec and overall this wouldn't change too much.

In that sense we feel that it's location at std::vec is still justified and we wouldn't want to necessarily move other collections like strings/slices into std::collections. We could perhaps rethink this for Rust 2.0 (if ever), but until that date we can't deprecate anything here at least and conventionally don't want to shift everyone to something else (as it doesn't seem worth the cost).

I'm going to close this for now but add the breakage-wishlist tag so we can come back to it, but thanks for the PR!

@alexcrichton alexcrichton added the rust-2-breakage-wishlist In the wishlist of breaking changes that requires rust 2.0 label Jul 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rust-2-breakage-wishlist In the wishlist of breaking changes that requires rust 2.0 T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants