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.

Sign up for GitHub

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

RFC: Move std::net::IpAddr types into core::net. #2832

Merged
merged 5 commits into from
Feb 25, 2023
Merged

RFC: Move std::net::IpAddr types into core::net. #2832

Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
49a923b
RFC: Move `std::net::IpAddr` types into `core:.net`.
reitermarkus Dec 6, 2019
21569bf
Update current state of IP and socket address types.
reitermarkus Nov 12, 2022
90ecf8f
Rename feature.
reitermarkus Nov 12, 2022
bd6b549
Mention `ToSocketAddrs`.
reitermarkus Nov 12, 2022
d73abe6
RFC 2832
joshtriplett Feb 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 80 additions & 0 deletions text/0000-core-net-types.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
- Feature Name: `core_net_types`
- Start Date: 2019-12-06
- RFC PR: [rust-lang/rfcs#2832](https://github.com/rust-lang/rfcs/pull/2832)
- Rust Issue: [rust-lang/rust#0000](https://github.com/rust-lang/rust/issues/0000)

# Summary
[summary]: #summary

Make the `IpAddr`, `Ipv4Addr`, `Ipv6Addr`, `SocketAddr`, `SocketAddrV4`,
`SocketAddrV6`, `Ipv6MulticastScope` and `AddrParseError` types available in `no_std`
contexts by moving them into a `core::net` module.

# Motivation
[motivation]: #motivation

The motivation here is to provide common types for both `no_std` and `std`
targets which in turn will ease the creation of libraries based around IP
addresses. Embedded IoT development is one area where this will be beneficial.
IP addresses are portable across all platforms and have no external
dependencies which is in line with the definition of the core library.

# Guide-level explanation
[guide-level-explanation]: #guide-level-explanation

The `core::net::IpAddr`, `core::net::Ipv4Addr`, `core::net::Ipv6Addr`,
`core::net::SocketAddr`, `core::net::SocketAddrV4`, `core::net::SocketAddrV6`,
`core::net::Ipv6MulticastScope` and `core::net::AddrParseError` types are
available in `no_std` contexts.

Library developers should use `core::net` to implement abstractions in order
for them to work in `no_std` contexts as well.

# Reference-level explanation
[reference-level-explanation]: #reference-level-explanation

Since https://github.com/rust-lang/rust/pull/78802 has been merged, IP and
socket address types are implemented in ideal Rust layout instead of wrapping
their corresponding `libc` representation.

Formatting for these types has also been adjusted in
https://github.com/rust-lang/rust/pull/100625 and
https://github.com/rust-lang/rust/pull/100640 in order to remove the dependency
on `std::io::Write`.

This means the types are now platform-agnostic, allowing them to be moved from
`std::net` into `core::net`.

# Drawbacks
[drawbacks]: #drawbacks

Moving the `std::net` types to `core::net` makes the core library less *minimal*.
Copy link

@pinkforest pinkforest Jan 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
Moving the `std::net` types to `core::net` makes the core library less *minimal*.
Moving the `std::net` types to `core::net` makes the core library less *seemingly minimal*.
However optimisations relied from the compiler means the dead code types must never be included
in the built binary when not used.

I think this could be best clarified here what this "minimal" means ?

Since the code is optimised away on cases where it is not used as #2832 (comment) - This provides testing coverage for rust-lang/rust#104265 (comment)


# Rationale and alternatives
[rationale-and-alternatives]: #rationale-and-alternatives

- Eliminates the need to use different abstractions for `no_std` and `std`.

- Alternatively, move these types into a library other than `core`, so they
can be used without `std`, and re-export them in `std`.

# Prior art
[prior-art]: #prior-art

There was a prior discussion at

https://internals.rust-lang.org/t/std-ipv4addr-in-core/11247/15

and an experimental branch from [@Nemo157](https://github.com/Nemo157) at

https://github.com/Nemo157/rust/tree/core-ip

# Unresolved questions
[unresolved-questions]: #unresolved-questions

None.

# Future possibilities
[future-possibilities]: #future-possibilities

Move the `ToSocketAddrs` trait to `core::net` as well. This depends on having `core::io::Result`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is not part of the current RFC. But I'm not sure I think it's a good idea to move ToSocketAddrs into core. It's relying on DNS resolution, which is not something core should be involved in doing. for simple non-lookup conversion into SocketAddr and IpAddr they already implement FromStr

Copy link

Choose a reason for hiding this comment

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

Is this an adt? If that's true, I think it's not a issue. That's will helps different DNS resolver implement this

Copy link
Contributor

Choose a reason for hiding this comment

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

So you want to keep both the trait and the types in core but the impls in std? Can core/std do that? Regular crates can't but maybe these are special in that sense.

Copy link

Choose a reason for hiding this comment

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

It seems like there are several impls of ToSocketAddrs which could exist in core as well, for example:

  • (IpAddr, u16)
  • (Ipv4Addr, u16)
  • (Ipv6Addr, u16)
  • SocketAddr
  • SocketAddrV4
  • SocketAddrV6
  • &'a [SocketAddr]

The only problem impls are the DNS-based ones on types like str which call sys_common::net::LookupHost by way of resolve_socket_addr. Perhaps there's a way to have those impls still live in core as well, but with LookupHost stubbed out in the event it's linked without std?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. I don't have a strong opinion. Feels sort of like a hack to me. But maybe there are strong use cases for crates wanting ToSocketAddrs in core that I'm not aware of.

Since moving the trait seems not 100% straight forward I would vote for doing that in a separate RFC/PR, since the move of the actual types is straight forward at this point, and has been delayed for quite some time.

Copy link

Choose a reason for hiding this comment

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

@tarcieri I think it'd need to be override-able from user code (similarly to global allocator) to allow custom impls in core which seems to be the main motivation behind moving it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like a perfect thing for "Future possibilities". I temporarily forgot that this is where it was placed in the RFC currently. So all good. It is definitely a future possibility, and no need to fully discuss it here I guess.

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like there are several impls of ToSocketAddrs which could exist in core as well

Their functionality is covered by these From impls:

impl From<SocketAddrV4> for SocketAddr
impl From<SocketAddrV6> for SocketAddr 
impl<I: Into<IpAddr>> From<(I, u16)> for SocketAddr