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

Stabilize mutable slice API #17494

Closed
wants to merge 3 commits into from

Conversation

aturon
Copy link
Member

@aturon aturon commented Sep 23, 2014

This commit is another in the series of vector slice API stabilization. The focus here is the mutable slice API.

Largely, this API inherits the stability attributes previously assigned to the analogous methods on immutable slides. It also adds comments to a few unstable attributes that were previously missing them.

In addition, the commit adds several _mut variants of APIs that were missing:

  • init_mut
  • head_mut
  • tail_mut
  • splitn_mut
  • rsplitn_mut

Some of the unsafe APIs -- unsafe_set, init_elem, and copy_memory -- were deprecated in favor of working through as_mut_ptr, to simplify the API surface.

Due to deprecations, this is a:

[breaking-change]

@rust-highfive
Copy link
Collaborator

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

@@ -778,10 +801,11 @@ pub trait MutableSlice<'a, T> {
/// // v.unsafe_set(10, "oops".to_string());
/// }
/// ```
#[deprecated = "use as_mut_ptr"]
Copy link
Member

Choose a reason for hiding this comment

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

This may also want to mention the set part as well:

#[deprecated = "use `*foo.as_mut_ptr().offset(index) = val`"]

@alexcrichton
Copy link
Member

Looks good to me! r=me with some adjustments to the deprecation messages.

bors added a commit that referenced this pull request Sep 25, 2014
…ichton

This commit is another in the series of vector slice API stabilization. The focus here is the *mutable* slice API.

Largely, this API inherits the stability attributes [previously assigned](#16332) to the analogous methods on immutable slides. It also adds comments to a few `unstable` attributes that were previously missing them.

In addition, the commit adds several `_mut` variants of APIs that were missing:

- `init_mut`
- `head_mut`
- `tail_mut`
- `splitn_mut`
- `rsplitn_mut`

Some of the unsafe APIs -- `unsafe_set`, `init_elem`, and `copy_memory` -- were deprecated in favor of working through `as_mut_ptr`, to simplify the API surface.

Due to deprecations, this is a:

[breaking-change]
This commit is another in the series of vector slice API
stabilization. The focus here is the *mutable* slice API.

Largely, this API inherits the stability attributes [previously
assigned](rust-lang#16332) to the analogous
methods on immutable slides. It also adds comments to a few `unstable`
attributes that were previously missing them.

In addition, the commit adds several `_mut` variants of APIs that were
missing:

- `init_mut`
- `head_mut`
- `tail_mut`
- `splitn_mut`
- `rsplitn_mut`

Some of the unsafe APIs -- `unsafe_set`, `init_elem`, and `copy_memory`
-- were deprecated in favor of working through `as_mut_ptr`, to simplify
the API surface.

Due to deprecations, this is a:

[breaking-change]
@pcwalton
Copy link
Contributor

Why is this a regression for low-level Rust?

@pcwalton
Copy link
Contributor

This is a small change that affects two methods that are rarely used in the extant codebase. Nobody felt that it would be controversial and the implication that we were deliberately attempting to sidestep community consensus is incorrect.

@brson
Copy link
Contributor

brson commented Sep 26, 2014

@thestinger Can you be more specific about how this is making unchecked indexing on slices harder?

Discussion about this is happening now.

@brson
Copy link
Contributor

brson commented Sep 26, 2014

@thestinger Do you have specific criticisms of the content of this pull request that you would like to discuss?

@aturon
Copy link
Member Author

aturon commented Sep 26, 2014

I think the problem here is just a poor deprecation message. It should instead suggest:

Replace v.unsafe_set(i, x) with *v.unsafe_mut(i) = x.

/// }
/// ```
/// Deprecated: use `*foo.as_mut_ptr().offset(index) = val` instead.
#[deprecated = "use `*foo.as_mut_ptr().offset(index) = val`"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't *foo.unsafe_mut(index) = val a better choice here?

Edit: ah, this was already stated elsewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes -- see comment in the github thread above. This was just a mistake in the message.

@aturon
Copy link
Member Author

aturon commented Sep 26, 2014

init_elem and copy_memory are clearly more specialized operations than skipping a bounds check.

As with many other deprecations (of safe APIs), these were eliminated as conveniences that weren't worth the extra API surface area. There's also a general push toward funneling conveniences through central traits/data structures, like Iterator and in this case ptr. The benefit is that conveniences added to these central structures have a multiplicative effect, and it's much easier to remember a single API surface rather than ad hoc convenience methods spread through different types.

If you want to do extensive unsafe work with slices, the simplest avenue would be to use .as_mut_ptr() once, and then apply as many operations as you like to the resulting pointer. And of course there's always the option to add extension traits providing any conveniences you like.

It's also worth noting that deprecations like these don't entail never providing such conveniences in the future, if they are ultimately deemed worthwhile. But there has been a lot of accumulation of ad hoc APIs, and the stabilization process is trying to move things toward a somewhat minimalistic, consistent, coherent, clean, and reasonably ergonomic state heading toward 1.0. (For bigger changes, like conventions, or removing the collection traits, or consolidating the numerics hierarchy, this involves RFCs.)

As an aside, we should consider adding an Index implementation to raw pointers, which would recover ergonomics very similar to C's -- and all by funneling through raw pointers.

@thestinger
Copy link
Contributor

I still think it's a regression and I disagree with this process but I'm not interested in arguing about it anymore. I don't care enough about the standard library to deal with that.

bors added a commit that referenced this pull request Sep 26, 2014
…ichton

This commit is another in the series of vector slice API stabilization. The focus here is the *mutable* slice API.

Largely, this API inherits the stability attributes [previously assigned](#16332) to the analogous methods on immutable slides. It also adds comments to a few `unstable` attributes that were previously missing them.

In addition, the commit adds several `_mut` variants of APIs that were missing:

- `init_mut`
- `head_mut`
- `tail_mut`
- `splitn_mut`
- `rsplitn_mut`

Some of the unsafe APIs -- `unsafe_set`, `init_elem`, and `copy_memory` -- were deprecated in favor of working through `as_mut_ptr`, to simplify the API surface.

Due to deprecations, this is a:

[breaking-change]
@bors bors closed this Sep 26, 2014
lnicola pushed a commit to lnicola/rust that referenced this pull request Jul 11, 2024
do not normalize `use foo::{self}` to `use foo`

It changes behaviour and can cause collisions. E.g. for the following snippet

```rs
mod foo {

    pub mod bar {}

    pub const bar: i32 = 8;
}

// transforming the below to `use foo::bar;` causes the error:
//
//   the name `bar` is defined multiple times
use foo::bar::{self};

const bar: u32 = 99;

fn main() {
    let local_bar = bar;
}
```

we still normalize

```rs
use foo::bar;
use foo::bar::{self};
```

to `use foo::bar;` because this cannot cause collisions.

See: rust-lang/rust-analyzer#17140 (comment)
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.

8 participants