rust-lang / wg-allocators

Home of the Allocators working group: Paving a path for a standard set of allocator traits to be used in collections!
http://bit.ly/hello-wg-allocators
205 stars 9 forks source link

Rename LayoutErr to LayoutError #73

Open exrook opened 3 years ago

exrook commented 3 years ago

We should rename LayoutErr to LayoutError in core::alloc. See prior discussion here: https://github.com/rust-lang/wg-allocators/issues/57#issuecomment-698482642

Since LayoutErr is stable, we can provide a type alias

pub type LayoutErr = LayoutError

I've implemented this change here, https://github.com/rust-lang/rust/compare/master...exrook:rename-layouterr. Some things that are still unclear to me are:

1) Should LayoutErr be deprecated 2) Should LayoutError be behind a feature flag at first

If LayoutErr is deprecated or marked for deprecation in a later version, it can no longer be used in std because of #[deny(deprecated,deprecated_in_future)] being in effect. I don't think it makes sense to have LayoutError replace LayoutErr in std, but then be behind a feature flag, only usable on nightly. This is mainly a usability issue because crates can of course continue to use LayoutErr despite the stdlib rustdocs showing LayoutError.

Option 1

Is it possible to make LayoutError immediately stable, then deprecate LayoutErr in the same or a later release? The following would then be possible:

#[stable(feature = "alloc_layout", since = "1.28.0")]
#[rustc_deprecated(since = "1.48.0", reason = "use LayoutError instead", suggestion = "LayoutError")]
pub type LayoutErr = LayoutError;

#[stable(feature = "alloc_layout_error", since = "1.48.0")]
#[derive(Clone, PartialEq, Eq, Debug)]
pub struct LayoutError {
    private: (),
}

Option 2

I think this would make more sense than trying to do a staged approach of adding LayoutError as unstable, then marking it stable, then deprecating the LayoutErr type alias.

Stage 1 - release 1.N

#[stable(feature = "alloc_layout", since = "1.28.0")]
pub type LayoutErr = LayoutError;

#[unstable(feature = "alloc_layout_error", issue = "32838")]
#[derive(Clone, PartialEq, Eq, Debug)]
pub struct LayoutError {
    private: (),
}

Stage 2 - release 1.N+1

#[stable(feature = "alloc_layout", since = "1.28.0")]
pub type LayoutErr = LayoutError;

#[stable(feature = "alloc_layout_error", since = "1.49.0")]
#[derive(Clone, PartialEq, Eq, Debug)]
pub struct LayoutError {
    private: (),
}

Stage 3 - release 1.N+2

#[stable(feature = "alloc_layout", since = "1.28.0")]
#[rustc_deprecated(since = "1.51.0", reason = "use LayoutError instead", suggestion = "LayoutError")]
pub type LayoutErr = LayoutError;

#[stable(feature = "alloc_layout_error", since = "1.49.0")]
#[derive(Clone, PartialEq, Eq, Debug)]
pub struct LayoutError {
    private: (),
}
SimonSapin commented 3 years ago

If and when this WG and the Libs team agree on a new name, I think there is no need to mess with #[unstable] items and the new name can be stable as soon as it’s added. It’s not like there are more design API details that are worth reserving the right to change later, as LayoutErr is already stable.

As to deprecation warnings, if we want them emitted we should use the since attribute to make it start two release cycles or more after the release that adds the new name (the current version of Nightly at the time of landing). That way the new name has reached the Stable channel when the Nightly channel starts warning. There is no need to wait for that cycle to add the attribute. That’s what since in rustc_deprecated is for, we can set it to start in the future.

However another option is to not emit any deprecation warning, and only soft-deprecate the old name in docs. What do we really gain by nudging existing code bases to actively migrate to the new name?

Additionally I personally don’t feel strongly that the naming consistency is worth bothering in the first place. Not renaming at all is also an option.

exrook commented 3 years ago

Thanks for the feedback Simon. It's good to hear that there is no need to use #[unstable] for a change like this, I would definitely prefer that route. I've removed the #[unstable] attributes and updated the deprecation attributes to be since = "1.50.0" in my branch.

As for deprecating the old name, given that the deprecated lint is warn by default, If we do decide to change the name I think it makes sense to let users know about the change through this mechanism.

As for whether this change is worth it, since we have also renamed AllocErr to AllocError, LayoutErr is the only error type in stdlib that does not follow the Error pattern. If we do make this change, the stdlib will be completely standardized on all error types ending with Error.

Additionally, doing some searches on github, there are approximately 370 uses of LayoutErr, of which 260 uses are of LayoutErr with AllocErr which will have to change anyways since AllocErr has now been renamed.

I think it makes sense to make this change for the sake of polish and consistency, this feature is not yet highly utilized, and the impact to existing users should be very low (they can continue to use LayoutErr if they wish, or even use LayoutError as LayoutErr),