Restrictions by jhpratt · Pull Request #3323 · rust-lang/rfcs (original) (raw)
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
(aka sealed traits and read-only fields, sort of)
Xiretza, joshtriplett, Noratrieb, jdahlstrom, cynecx, ShadowJonathan, Kolsky, GrayJack, Nokel81, luksan, and 24 more reacted with thumbs up emoji juntyr, Jules-Bertholet, joshtriplett, y86-dev, Noratrieb, mikeleany, jplatte, Kolsky, Nokel81, jder, and 14 more reacted with heart emoji darksv, Noratrieb, rodrimati1992, clarfonthey, aznhe21, tmccombs, dpc, stepancheg, Virgiel, SOF3, and 5 more reacted with eyes emoji
I like the idea:
- especially if later on this allowed us to have coherence be aware of these unimplementable traits 🤞
- the whole "replace fields with getters" in Rust is kind of a lie, given the different interactions with the borrow checker, so if read-only fields help alleviate the need for the "view types" proposals, which involve way more churn, it does seem like a win!
On the other hand, whilst pub mut(…) field_name: FieldTy is kind of manageable syntax, the trait impl(mod) Thing, on the other hand, is:
- yielding quite a weird combination of words (although this is indeed subjective / a matter of habit — but it could spook beginners);
- which requires a grammar extension (and thus for the macros to update and accomodate to it);
- non-
cfg-tweakable (e.g., imagine acfg(feature = "__internal-integration-tests")to disable the restriction to let one write mock impls within integration tests, be it fortraits or fields); breaks. EDIT: I had somehow misread the position of thetrait TraitName-grep-ability (this could be pereceived to be a minor thing, but in any case it should very much not to be underestimated)impl(…)element, my bad.
So maybe an attribute-based syntax could be, at least for traits, a more appropriate approach?
| # Drawbacks |
| - Additional syntax for macros to handle |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to cover how precisely macros should handle it. I think this needs at least one new matcher, possibly a few. First, we definitely need a matcher for "what goes inside the parentheses after mut or impl", matching self, crate, super, in path, etc. And second, we might want convenient matchers for "everything that can go before the name in a field" and "everything that can go before the name in a trait declaration", which types may use in place of "vis" for convenience.
As an alternative, I wonder if it might make sense to have these desugar to an attribute, and then macros that match and preserve attributes will Just Work. That would be a larger change, though.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You also need a second discussion about macros, separate from matching this syntax: is mutability/implementability determined by span or by expansion site?
Visibility currently uses expansion site, not span. However, span-based visibility would be incredibly valuable for many users (to avoid having to export a doc(hidden) type for use by a macro that users can technically use without the macro). Taking that into account, I wonder if we might want span-based mutability and span-based implementability.
| it is likely unwise to add a new matcher for each restriction. Should the new syntax be included |
|---|
| as part of the `vis` matcher? If so, it is important to note that restrictions are not the same |
| everywhere, if they are valid at all. |
| - Suggestions welcome! |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above: I think we should add some new matchers for this.
We can't include these in $vis unless we parse them everywhere a $vis can go (even if we reject them later), and I don't think we should do that.
Why not the syntax of pub trait(crate) instead of pub impl(crate) trait?
I think that would be vastly more confusing.
It just seems like a waste for the impl to be there because that's not usually where it's used for things
Can this RFC be split into two? I'll second everything @jstarks said about mut fields: sealed traits sound like a great addition, but I question the value of mut fields.
I will not be splitting the RFC in two unless a member of the lang team explicitly requests it. Given that they are all aware of this proposal, I do not expect this to happen.
Sounds good, I found @joshtriplett's rationale:
I personally think it's valuable to see the two together, since they're using similar syntax and striving for orthogonality and knowledge transfer.
While that may be true, here are my concerns with the mut part of this RFC:
- Before this RFC, if you owned a piece of visible data, you could always mutate it. I get that the point of this RFC is to change that, but I actually think disallowing mutation of visible data is a mistake in other languages. It leads to API design where everything is read-only by default because libraries assume "mutation is bad" and so you have to make copies of every API's data structures into your own module when you want to mutate one little thing.
- Everywhere else in Rust,
mutallows mutability. This RFC suddenly confuses that simple heuristic and makesmutsometimes disallow mutability.
Everywhere else in Rust, mut allows mutability. This RFC suddenly confuses that simple heuristic and makes mut sometimes disallow mutability.
I agree with this, and this is one of the reasons I'd like to seriously consider a simpler syntax. I would still like to see both pieces of this functionality, but I wonder if another syntax would be easier to work with long-term.
Before this RFC, if you owned a piece of visible data, you could always mutate it. I get that the point of this RFC is to change that, but I actually think disallowing mutation of visible data is a mistake in other languages. It leads to API design where everything is read-only by default because libraries assume "mutation is bad" and so you have to make copies of every API's data structures into your own module when you want to mutate one little thing.
As opposed to making everything a getter method like I tend to see these days, to the point of macro crates like getset existing to compensate for the added boilerplate?
(According to lib.rs, getset is the 11th-ranked crate in the procedural macros category and gets 322,385 downloads per month.)
Heck, I take the getter approach. It's about not wanting to be forced to bump my major version and support multiple versions because I painted myself into a corner letting other people poke at my internals willy-nilly. Allowing read-only fields on structs would make my APIs cleaner. (It doesn't help that I'm a big fan of correct-by-construction newtypes, which means that people could break the promised invariants by modifying the fields directly.)
I painted myself into a corner letting other people poke at my internals willy-nilly.
That's actually another one of my concerns: being forced to use getters is a good thing. Read-only fields do not solve the painted-into-a-corner problem. Here's a real-world example from nix: https://docs.rs/nix/latest/nix/errno/enum.Errno.html#method.as_errno. They had an API that returned an option, but eventually changed the internals to always be Some. This wouldn't have been possible if they exposed their raw internals.
I'd consider the getset crate to be a point in favor of getters, or at least it removes the argument that writing getters is too hard.
They had an API that returned an option, but eventually changed the internals to always be
Some. This wouldn't have been possible if they exposed their raw internals.
So you're saying they stopped storing an Option<T> and started constructing one on demand? ...OK if that works for them. I think it's a reasonable trade-off.
I'd consider the getset crate to be a point in favor of getters, or at least it removes the argument that writing getters is too hard.
I write my getters by hand to keep my supply chain attack surface down pending someone solving the "If you don't audit your dependencies yourself, it's your own damn fault" problem.
So you're saying they stopped storing an Option and started constructing one on demand? ...OK if that works for them. I think it's a reasonable trade-off.
It was a little bit more complicated than that, but yeah the deprecated method just unconditionally returned Some.
As FCP is nearing completion, I'll say this to avoid any duplicate efforts. I have an implementation that is nearly complete, and I will be posting a PR soon. mut restrictions work completely, while impl restrictions have one outstanding issue before I can say the same. So hopefully this will land quite soon!
The final comment period, with a disposition to merge, as per the review above, is now complete.
As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.
This will be merged soon.
read-only fields and sealed traits sound like two orthogonal features. Why are they in the same RFC?
Furthermore, is it relevant to expose similar syntax that denies calling methods on a trait as well? I had a use case where the trait is only used as a type bound to pass a generic type from a blackbox to another blackbox. Would it be appropriate to also add something like pub call(crate) trait?
The RFC was not split because no lang team member requested it be split.
As to avoiding calling a method, it sounds like what you're looking for is visibility on trait items, which is a significantly different problem (but one I would also like to tackle in the future).
Huzzah! The @rust-lang/lang team has decided to accept this RFC.
To track further discussion, subscribe to the tracking issue here: rust-lang/rust#105077
