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

Restrictions #3323

Merged
merged 9 commits into from
Nov 29, 2022
Merged

Restrictions #3323

merged 9 commits into from
Nov 29, 2022

Conversation

jhpratt
Copy link
Member

@jhpratt jhpratt commented Oct 9, 2022

(aka sealed traits and read-only fields, sort of)

Rendered

text/3323-restrictions.md Outdated Show resolved Hide resolved
@danielhenrymantilla
Copy link

danielhenrymantilla commented Oct 9, 2022

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 a cfg(feature = "__internal-integration-tests") to disable the restriction to let one write mock impls within integration tests, be it for traits or fields);
  • breaks trait TraitName-grep-ability (this could be pereceived to be a minor thing, but in any case it should very much not to be underestimated). EDIT: I had somehow misread the position of the 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
Copy link
Member

@joshtriplett joshtriplett Oct 9, 2022

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.

Copy link
Member

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!
Copy link
Member

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.

text/3323-restrictions.md Outdated Show resolved Hide resolved
@lemonlambda
Copy link

lemonlambda commented Nov 17, 2022

Why not the syntax of pub trait(crate) instead of pub impl(crate) trait?

@jhpratt
Copy link
Member Author

jhpratt commented Nov 18, 2022

I think that would be vastly more confusing.

@lemonlambda
Copy link

It just seems like a waste for the impl to be there because that's not usually where it's used for things

@SUPERCILEX
Copy link

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.

@jhpratt
Copy link
Member Author

jhpratt commented Nov 19, 2022

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.

@SUPERCILEX
Copy link

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, mut allows mutability. This RFC suddenly confuses that simple heuristic and makes mut sometimes disallow mutability.

@joshtriplett
Copy link
Member

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.

@ssokolow
Copy link

ssokolow commented Nov 19, 2022

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.)

@SUPERCILEX
Copy link

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.

@ssokolow
Copy link

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.

@SUPERCILEX
Copy link

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.

@jhpratt
Copy link
Member Author

jhpratt commented Nov 24, 2022

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!

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this RFC. and removed final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. labels Nov 24, 2022
@rfcbot
Copy link
Collaborator

rfcbot commented Nov 24, 2022

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.

@SOF3
Copy link

SOF3 commented Nov 25, 2022

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?

@jhpratt
Copy link
Member Author

jhpratt commented Nov 25, 2022

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).

@tmandry tmandry merged commit 5963091 into rust-lang:master Nov 29, 2022
@tmandry
Copy link
Member

tmandry commented Nov 29, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this RFC. T-lang Relevant to the language team, which will review and decide on the RFC. to-announce
Projects
None yet
Development

Successfully merging this pull request may close these issues.