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

Rename .as_mut() to .as_mut_ref() in Option/Result/AnyMutRefExt #14299

Closed
wants to merge 1 commit into from

Conversation

lilyball
Copy link
Contributor

For consistency with other naming patterns, rename .as_mut() to
.as_mut_ref().

Leave .as_mut() as a deprecated function.

[breaking-change]

For consistency with other naming patterns, rename .as_mut() to
.as_mut_ref().

Leave .as_mut() as a deprecated function.

[breaking-change]
@thestinger
Copy link
Contributor

The reasoning behind naming it as_mut was to avoid verbosity, since it was replacing a family of functions with composition.

@lilyball
Copy link
Contributor Author

Every other library method I've looked at that has mut and non-mut variants works by inserting mut into the name, rather than replacing an existing component with mut. For example, iter() vs mut_iter(). as_ptr() vs as_mut_ptr(). as_slice() vs as_mut_slice(). get() and get_mut(), find() and find_mut().

as_ref() vs as_mut() is the only instance that breaks this pattern.

@alexcrichton
Copy link
Member

This may need some more context to take action on it before moving forward. I view as_mut as the best name for this function, regardless of our current guidelines. Considering our other existing apis, it may be better to reconsider how other functions are named rather than blindly changing this to conform (everything is still in a state of flux).

I know that @aturon has recently been analyzing our patterns throughout the stdlib, and I know he'd be interested in this.

@lilyball
Copy link
Contributor Author

I was motivated to do this because I tried to write code and just used .as_mut_ref() without thinking about it, because that seemed to be the obvious name. I was surprised when it didn't work. And I think I was surprised because I've been trained by all those other methods to expect "mut" to be a qualifier that adds to a name, rather than replacing it. After all, we know what a "mut ref" is, so as_mut_ref() makes sense, but what is a "mut"?

The only context where .as_mut() makes sense to me is if it somehow returns a variant of the receiver that is mutable without adding any indirection (e.g. it would make sense for RawPtr to has as_mut()). But that doesn't apply to the existing .as_mut() methods.

I would be interested to hear what @aturon has to say on this subject.

@aturon
Copy link
Member

aturon commented May 20, 2014

There's a clear convention about mutable variants of as_foo being as_mut_foo.

On the other hand, the foo is often an abbreviation or synonym for some type (e.g. iter for Iterable, slice for &[T]). From that perspective, shortening mut_ref to mut seems fine. The question is just whether we're comfortable abbreviating the type differently for different borrow styles.

Personally, I tend to agree with @kballard: part of the help of conventions is that you (and your finger muscles) don't have to remember special cases, even if that sometimes means typing more.

Two other points.

@lilyball
Copy link
Contributor Author

@aturon Do you think we should hold off on resolving this local consistency issue until #13660 is resolved? I don't think there's a need to wait on that (especially because, as I commented in #13660, I think having <verb>_mut and mut_<noun> coexisting is fine (e.g. find_mut vs as_mut_slice)).

Also, until such time as a decision is made to rename &mut T to something else, I'd rather assume it will keep its current name and behave accordingly.

@aturon
Copy link
Member

aturon commented May 20, 2014

@kballard I would not be opposed to resolving this ahead of #13660 if there was clear consensus about it, since the underlying, informal convention is otherwise consistent. OTOH, if there is going to be a debate on this name, it should connect to the wider debate around #13660, which is asking for a formal convention around _mut.

I also agree about the name change for `&mut T. Just something to keep in mind.

As far as I can tell, this is a unique exception to the usual as_foo, as_mut_foo convention. @alexcrichton, can you elaborate on the broader reconsideration you have in mind, as well as why you feel the current name is the right one regardless?

@alexcrichton
Copy link
Member

Some thoughts that went into the original decision:

  1. as_mut_ref is slightly redundant because there's not much mutable you can get other than a reference.
  2. as_mut has the same number of letters than as_ref.

They're not very strong arguments in the face of the rest of the standard library, however. I'd also be fine with resolving this ahead of #13660 and the mut/uniq debate. May as well get everything in to line first.

@alexcrichton
Copy link
Member

Closing due to inactivity, but these naming issues with mutability definitely need to be resolved for 1.0!

lnicola pushed a commit to lnicola/rust that referenced this pull request Mar 13, 2023
fix: Fix search not searching bodies of attributed items

Closes rust-lang/rust-analyzer#14229
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.

4 participants