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

Tracking Issue for Restrictions #105077

Open
10 tasks
tmandry opened this issue Nov 29, 2022 · 25 comments
Open
10 tasks

Tracking Issue for Restrictions #105077

tmandry opened this issue Nov 29, 2022 · 25 comments
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC F-impl_restriction `#![feature(impl_restriction)]` F-mut_restriction `#![feature(mut_restriction)]` T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@tmandry
Copy link
Member

tmandry commented Nov 29, 2022

This is a tracking issue for the RFC "Restrictions" (rust-lang/rfcs#3323).
The feature gates for the issue are:

  • #![feature(impl_restriction)]
  • #![feature(mut_restriction)]

About tracking issues

Tracking issues are used to record the overall progress of implementation.
They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions.
A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature.
Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

Steps

Unresolved Questions

  • Should a simpler syntax be provided for common cases? For instance, sealed or readonly. A
    different syntax altogether could be used as well.
  • Should an "unnecessary restriction" lint be introduced? It would fire when the restriction is as
    strict or less strict than the visibility. This warning could also be used for pub(self).
    • Does this necessarily have to be decided as part of this RFC?
  • How will restrictions work with macro_rules! matchers? There is currently a vis matcher, but
    it is likely unwise to add a new matcher for each restriction.
    • The proposed syntax cannot be added to the vis matcher, as it does not current restrict the
      tokens that can follow. For this reason, it could break existing code, such as the following
      example.
      macro_rules! foo {
          ($v:vis impl(crate) trait Foo) => {}
      }
      foo!(pub impl(crate) trait Foo);
    • A restriction matcher could work, but restrictions are not the same everywhere.
    • mut_restriction and impl_restriction are relatively long.
  • What is the interaction between stability and restrictions?
    • Suggestion: Visibility is an inherent part of the item; restrictions should be as well. Metadata
      can be added in the future indicating when an item had its restriction lifted, if applicable.
      The design for this is left to the language team as necessary. A decision does not need to be
      made prior to stabilization, as stability attributes are not stable in their own right.
  • Should the in syntax be permitted for restrictions? Including it is consistent with the existing
    syntax for visibility. Further, the lack of inclusion would lead to continued use of the
    workaround for impl. For mut, there is no workaround. The syntax is not used often for
    visibility, but it is very useful when it is used.
  • Should struct expressions be disallowed?
    • Where would it be desirable to prohibit mutability after construction, but still permit
      construction with unchecked values?

Implementation history

@tmandry tmandry added B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. T-lang Relevant to the language team, which will review and decide on the PR/issue. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC labels Nov 29, 2022
@jhpratt
Copy link
Member

jhpratt commented Nov 29, 2022

NB: My mostly-complete implementation uses #![feature(impl_restriction)] and #![feature(mut_restriction)], not a single shared feature.

@SUPERCILEX
Copy link
Contributor

Thanks for keeping the features separate! :)

@rinarakaki
Copy link

rinarakaki commented Dec 4, 2022

The whole this RFC is trying to solve is admittedly complicated in terms both of the concepts and the syntax but I found this RFC might relate to what I have proposed and provide with what I wanted: https://internals.rust-lang.org/t/structs-field-level-mutability-control-at-instantiation-not-at-definition/16061. The scoped mutability syntax included.

@jhpratt
Copy link
Member

jhpratt commented Dec 4, 2022

This is not the place to rehash the RFC, expect for the listed unresolved questions. Also, saying you "found" the RFC is misleading — you are the one who created the thread.

@rpjohnst
Copy link
Contributor

rpjohnst commented Dec 16, 2022

I would like to sketch a holistic mental model to shed some light on the unresolved questions: this, along with some other features like #[non_exhaustive], type-alias-impl-trait, safe transmute, unsafe fields, and private impls, are all fundamentally about making visibility more granular. Visibility is our one big, orthogonal tool for enforcing invariants, so keeping things under that umbrella keeps the language feeling clean and composable. A beginner encountering any variation on visibility for the first time should be directed to our documentation on this approach.

Under this model, the RFC splits up the existing visibility of fields into 1) the visibility of their value and address (read) and 2) the visibility of their construction and mutable place exprs (write). Similarly, it splits up the existing visibility of traits into 1) the visibility of the trait as a bound and 2) the visibility of its "construction" (impl-ability). (This comment on the RFC thread also notes that construction is fundamentally a whole-type property, not a per-field property.)

#[non_exhaustive] splits out the visibility of a struct's construction, and an enum's full set of variants. Type-alias-impl-trait splits out the visibility of the concrete type, as I noted on Zulip. Safe transmute is trying to consider the visibility of a type's layout. Unsafe fields would hang proof obligations off of the same "facet of visibility" of the field as this RFC's mut. And private impls would split out the visibility of an impl from the visibility of its Self type.

To me, this model implies answers to a few of the unresolved questions, and some future possibilities to consider:

  • First, syntax: we should go with the pub(..)-based design in spite of the concern about enum variant fields, neatly resolving the shorthand and macro matcher questions. We lose very little with post-expansion error checking, where we can give more meaningful errors anyway. Consider the current error message for pub on an enum variant field: "unnecessary visibility qualifier, pub not permitted here because it's implied." Also consider the cited prior art: the fully-general form of read-only properties in e.g. C# uses private set { .. }- a visibility modifier.
  • As noted in the RFC thread, controlling both mutable places and construction with mut, yet not supporting anything like mut(nowhere), makes for a rather false analogy with mut on bindings. We should resolve the struct expression question either by giving construction its own type-level visibility facet aligned with the existing #[non_exhaustive], or else by renaming mut to something that more fully encompasses what it does.
  • If we establish that these features work in terms of granular visibility, that gives us a useful precedent and vocabulary to guide future additions like visibility on trait items or enum variant fields, locally-transparent type-alias-impl-trait, safe transmute across crates, unsafe fields, etc. We can explicitly consider the way visibility is divided up and how that relates to semver.

However, having given my best shot above at making this proposal make sense to myself, I am concerned that the RFC was merged without sufficient motivation over existing practice. The cost of new syntax like this, especially when there are already idiomatic alternatives, weighs much more to me than having all these knobs available. Indeed, there were several well-reasoned comments in this direction, before and during the FCP (which I missed).

For instance, the suggestion in rust-lang/rfcs#3323 (comment) and rust-lang/rfcs#3323 (comment) to reduce the feature's surface area based on more concrete examples. Attributes are already our main "escape valve" for new and increasingly niche syntax- this would address all the syntactic open questions even more effectively than the pub(..) I advocated for above. The first comment also suggested splitting up the RFC specifically because sealed traits are much more strongly motivated than read-only fields.

Or consider rust-lang/rfcs#3323 (comment) which makes a strong case that the mut half of this RFC may not meaningfully improve the status quo. The response didn't point to any better examples, and merely handwaved that it might help in some cases with borrow checking. So on one hand we've got hand-written or macro-generated getters (including borrow-splitting ones or view types), and on the other we've got the RFC's new syntax which can't fully replace it or offer any truly new capabilities. This actually came up again during FCP!

@jhpratt
Copy link
Member

jhpratt commented Dec 18, 2022

I understand what you're saying with regard to a mental model, but I have to disagree with a fair amount.

we should go with the pub(..)-based design in spite of the concern about enum variant fields, neatly resolving the shorthand and macro matcher questions. We lose very little with post-expansion error checking, where we can give more meaningful errors anyway.

The concern is not about enum variant fields. It is about having what is permitted as part of visibility being different in different places. For example, you couldn't do pub(mut in crate) trait Foo {} for obvious reasons. I strongly believe that if something is a visibility, it should be the same everywhere. I cannot immediately think of any other construct that has such significant differences depending on where it is used. Further, I believe that a macro author should be able to reject invalid code upon invocation, not just deferring errors to AST validation. After all, what if the macro author doesn't want to permit a restriction?

As noted in the RFC thread, controlling both mutable places and construction with mut, yet not supporting anything like mut(nowhere), makes for a rather false analogy with mut on bindings. We should resolve the struct expression question either by giving construction its own type-level visibility facet aligned with the existing #[non_exhaustive], or else by renaming mut to something that more fully encompasses what it does.

I have no objection to full immutability. Without rereading the full RFC PR, I'm fairly certain I asked at least once for potential syntax, and no one provided anything that could be agreed upon. I would personally prefer mut(!), following the ! type.

For struct expressions, there is a key question that you did not address.

Where would it be desirable to prohibit mutability after construction, but still permit
construction with unchecked values?

Unless there is a use case here, I see no reason to permit it. If you have a suggestion for a keyword other than mut, I'm all ears.

If we establish that these features work in terms of granular visibility, that gives us a useful precedent and vocabulary to guide future additions like visibility on trait items or enum variant fields, locally-transparent type-alias-impl-trait, safe transmute across crates, unsafe fields, etc. We can explicitly consider the way visibility is divided up and how that relates to semver.

I believe that process-wise, this should require significant public discussion and possibly an FCP of some sort. Suggesting a mental model is one thing, but making decisions based off of it is another.

For instance, the suggestion in rust-lang/rfcs#3323 (comment) and rust-lang/rfcs#3323 (comment) to reduce the feature's surface area based on more concrete examples.

I addressed those links in the comments immediately following them.

The first comment also suggested splitting up the RFC specifically because sealed traits are much more strongly motivated than read-only fields.

If even one lang team member wanted it split, it would have been. I stated this repeatedly. If you have an issue with the RFC not being split, the time has passed. It was accepted as one, and as such both restrictions have been accepted (minus the unresolved questions). There is absolutely no reason to believe that the outcome would have been any different were the RFC split in two.

Since you mention view types, however, the "handwaving" you're referring to (language that I find borderline rude) is identical to that of view types. They are complementary, and would have the same effect on splitting borrow checking.

@jhpratt
Copy link
Member

jhpratt commented Dec 18, 2022

As a general note to anyone, I have a nearly complete implementation of restrictions on my fork of rustc. There is only one outstanding bug (impl restrictions not being enforced for traits from external crates). Aside from that, a PR should be up soon. For anyone that has concerns about syntax, you'll be able to play around with it on nightly soon.

(in a separate comment because it's not in response to the prior comment)

@jhpratt
Copy link
Member

jhpratt commented Dec 23, 2022

Mostly-complete implementation is up: #106074

Any assistance with figuring out the one bug is appreciated!

@dtolnay
Copy link
Member

dtolnay commented Jan 31, 2023

Are restrictions proposed for the mutability of statics and extern statics? I didn't find this addressed one way or the other in the RFC text or RFC PR discussion.

pub static mut(in) ASDF: usize = 0;

extern "C" {
    pub static mut(in) JKL: usize;
}

@jhpratt
Copy link
Member

jhpratt commented Jan 31, 2023

@dtolnay No one brought this up at any point. I think it's a reasonable enough extension that it shouldn't require another RFC. Once the current implementation PR is merged, I can look into it.

@tgross35
Copy link
Contributor

tgross35 commented Apr 5, 2023

Was something like a std::marker::Sealed lang item trait ever considered?

In any case I'm +1 for something like #[sealed] or the trait Foo: marker::Sealed, or even sealed trait Foo in favor of the current trait impl(crate) Foo syntax. I don't think that using the keyword impl in anything that's not an implementation builds the right intuition, and it's a bit keyword soupy as-is.

I kind of feel similarly for restricted fields. The RFC says

The derive-getters and getset crates are derive macros that are used to generate getter methods. The latter also has the ability to derive setters. This demonstrates the usefulness of reduced syntax for common behavior. Further, getset allows explicitly setting the visibility of the derived methods. In this manner, it is very similar to the ability to provide a path to the mut restriction.

But to me, that sounds more like a reason to add some sort of "official" autoderevation for getters and setters, instead of adding complexity to how fields are defined. A benefit that these crates have is that the getters can be rewritten if their exact implementation needs to change, which is not possible with this proposal. Is introducing new, complicated syntax justified when it doesn't fully replicate the features of these crates? I'm not convinved it is. (Personally, I would continue to use getset instead of mut_restriction for this very reason - flexibility for future changes can be critical.)

edit: removed comment supporting the pub(mut in crate) option as a fallback, I see the discussion now

@tgross35
Copy link
Contributor

tgross35 commented Apr 5, 2023

Regarding @dtolnay's suggestion - I've seen quite a bit that there's a general goal to migrate away from static mut in favor of static with interior mutability, #95439 for example, which would make the syntax not necessary in that case.

Currently I find alternatives to static mut extremely clunky, but that's a separate issue.

@jhpratt
Copy link
Member

jhpratt commented Apr 6, 2023

current trait impl(crate) Foo syntax

Nit: it's impl(crate) trait Foo, so as to preserve the ability to do rg "trait Foo" and find the definition.

that sounds more like a reason to add some sort of "official" autoderevation for getters and setters

Barring the addition of view types, getters and setters fundamentally cannot do the same thing that a public field can.

A benefit that these crates have is that the getters can be rewritten if their exact implementation needs to change, which is not possible with this proposal

Is introducing new, complicated syntax justified when it doesn't fully replicate the features of these crates?

I disagree on the syntax being "complicated", but it absolutely replicates the features of the named crates and goes one step further in permitting partial borrows.

Note that no one is forcing you to use mut restrictions. If you want to continue with getters and setters, you are free to do so. The ability to change the field's type (or remove it altogether) is inherent to a field being public, and is not affected in any way by restrictions.

mut restrictions have been formally adopted via the RFC; the only undecided questions are the ones listed in the top of this thread. I don't believe revisiting whether restrictions are useful is productive; feel free to re-read the RFC thread for the multiple, lengthy discussions.

@rben01
Copy link

rben01 commented Apr 7, 2023

Was something like a std::marker::Sealed lang item trait ever considered?

In any case I'm +1 for something like #[sealed] or the trait Foo: marker::Sealed, or even sealed trait Foo in favor of the current trait impl(crate) Foo syntax. I don't think that using the keyword impl in anything that's not an implementation builds the right intuition, and it's a bit keyword soupy as-is.

“Keyword soup” is exactly how this feels. #[attributes] would seem a lot cleaner and might generalize better if there are additional changes to make in the future. Keyword soup would only get soupier.

@jhpratt
Copy link
Member

jhpratt commented Apr 8, 2023

@rben01 Do you have a concrete idea for the syntax?

@rben01
Copy link

rben01 commented Apr 8, 2023

@rben01 Do you have a concrete idea for the syntax?

First, let me just say that I very much like the semantics of this RFC. It's just the syntax that I'm not a fan of.

I think attributes would be more natural, and also more generalizable to similar cases of restriction not covered by this RFC, that there might be desire to cover in the the future. The concrete syntax I'd suggest would be something like this:

pub mod foo {
    pub mod bar {
        #[restrict(impl(super))]
        pub(crate) trait Foo {}
    }

    // can impl Foo here
}
// but not here

pub struct Time {
    #[restrict(mut(crate))]
    pub hour: u8,
    #[restrict(mut(crate))]
    pub minute: u8,
    #[restrict(mut(crate))]
    pub second: u8,
    #[restrict(mut(crate))]
    pub nanosecond: u32,
}

This also has the advantage that it's clear a restriction is being added. The fact that pub hour: u8 would more mutable than pub mut(crate) hour: u8 is counterintuitive — were I a newbie to Rust and I saw pub mut(crate) hour: u8, I'd expect pub mut hour: u8 or pub mut(all) hour: u8 to be the universally mutable version; I'd expect that pub hour: u8 made an immutable field because, after all, there is no mut there. To me, #[restrict(mut(crate))] makes it much clearer that a restriction is being added instead of lifted.

In addition, attributes would open the door to “distributive” shorthands like

#[restrict(mut(crate))] // applies to all fields in struct
pub struct Time {
    pub hour: u8,
    pub minute: u8,
    pub second: u8,
    pub nanosecond: u32,
}

pub enum Foo {
    #[restrict(mut(crate))] // applies to all fields in variant
    Alpha { x1: u8, x2: u8 },
    Beta { y: u8 },
}

#[restrict(mut(crate))] // applies to all fields in all variants in enum
pub enum Bar {
    Alpha { x1: u8, x2: u8 },
    Beta { y: u8 },
}

You could imagine, I suppose, pub mut(crate) struct Time, but there is no place to stick that marker in the Foo::Alpha variant. Attributes are a natural and generalizable solution.


In addition, having read the excellent analysis of all of the flavors of sealed-ness here, which explores more axes of restriction than just can-impl, I think attributes would also be a natural way to annotate trait functions. This is not in the Restrictions RFC, but it seems like a natural follow-up, and so I think it should be considered in the design of Restrictions.

One example the article brings up is std::error::Error::type_id, whose signature is

fn type_id(&self, _: private::Internal) -> TypeId where Self: 'static

_: private::Internal is not actually used; it's just there to prevent crates outside std from overriding or calling type_id (since they'd have to reference private::Internal to do so, which they can't do). It would be more natural IMO to write (say)

#[restrict(override(crate), call(crate))]
fn type_id(&self) -> TypeId where Self: 'static

There is no obvious keyword analog for this (fn override(crate) call(crate) type_id(...) -> ...? Very soupy.). And, again, using the word #[restrict(...)] makes it clear that we are adding, not lifting, a restriction.


I understand that this is not the place to rehash the RFC — it's already been accepted — but, well, that's my concrete idea for a different syntax.

@jhpratt
Copy link
Member

jhpratt commented Apr 9, 2023

First, let me just say that I very much like the semantics of this RFC. It's just the syntax that I'm not a fan of.

I have no problem discussing it! My last comment was more of "ideas without proposals don't lead anywhere". It's worth noting that I originally proposed #[sealed] on traits, but there was some pushback to that as some people wanted it to mean "exhaustive", in that downstream users could rely on there never being new implementations (having an impact on the orphan rule). Part of the reason I chose the syntax I did for the RFC is because it reuses existing keywords, eliminating any concern about syntactical ambiguity.

Overall I'm actually quite indifferent on syntax, and what you suggest is more than reasonable; it would solve a couple of the unresolved questions immediately as a result of being an attribute.

pub mut(crate) hour: u8, I'd expect pub mut hour: u8 or pub mut(all) hour: u8 to be the universally mutable version

That is a very reasonable concern to have. Personally I wouldn't mind changing the default in an edition, but there would unquestionably be significant pushback as it's such a major change. For that reason I'm not actually proposing to do this.

In addition, attributes would open the door to “distributive” shorthands like

This doesn't necessarily require attributes; the same could be done with native syntax.

In addition, having read the excellent analysis of all of the flavors of sealed-ness here, which explores more axes of restriction than just can-impl, I think attributes would also be a natural way to annotate trait functions. This is not in the Restrictions RFC, but it seems like a natural follow-up, and so I think it should be considered in the design of Restrictions.

Clearly you don't follow me on Mastodon 😉 I responded to the author in that restrictions covers one row of the table, while proper visibility on traits covers the remaining row. It is something that I want to do in the future, but I am viewing this as an extension of visibility, not restrictions.

I understand that this is not the place to rehash the RFC — it's already been accepted — but, well, that's my concrete idea for a different syntax.

The way I view it is this. Restrictions have been accepted, but the syntax has not, as it's explicitly listed as an unresolved question. Right now it's an idea that is being used in the initial implementation. Rehashing implies questioning the usefulness of restrictions as a whole, rather than trying to figure out details. I don't think it's that at all.

@WaffleLapkin WaffleLapkin added F-impl_restriction `#![feature(impl_restriction)]` F-mut_restriction `#![feature(mut_restriction)]` labels May 26, 2023
@the8472
Copy link
Member

the8472 commented Aug 14, 2023

Another syntax alternative is (ab)using where clauses.

trait Foo<T>: Bar
where
  impl in crate::mod_foo,
  T: Copy
{
}

This might especially make sense if the restriction has an impact on coherence

@tgross35
Copy link
Contributor

tgross35 commented Sep 6, 2023

The RFC was not split since the features require shared design work, but could trait restrictions and field restrictions be gated separately in implementation?

They can keep a shared tracking issue for discussion since the syntax and some of the semantics must be cohesive. My thought is that assuming consensus on syntax is reached, it would be unfortunate to tie the stability of the items together if they turn out to require different amounts of design iteration.

Of course they could also be split down the line, but easier to do it now than after implementation. Somewhat easier to review the initial impl too and narrow eventual bugs if the implementation isn't monolithic.

@jhpratt
Copy link
Member

jhpratt commented Sep 6, 2023

The features are split! See the top issue and my first comment. The open PR does this already, and just needs me to update it.

I do have thoughts of a different syntax that I believe will please everyone, but I'm holding off on saying what exactly that is until there's an implementation merged.

@Zoybean
Copy link

Zoybean commented Dec 17, 2023

please excuse if this has already been mentioned, but I have one thought on the field mutability restrictions: is there a downside to allowing struct initialisation and destructuring unless the non-exhaustive attribute is also set?
though I do agree that it seems unlikely for an invariant to need to prevent mutability but allow arbitrary construction.

@jhpratt
Copy link
Member

jhpratt commented Dec 17, 2023

though I do agree that it seems unlikely for an invariant to need to prevent mutability but allow arbitrary construction.

That's pretty much it. In the RFC PR, I asked for situations where this would be the case and never received an answer (to my recollection).

@ratmice
Copy link
Contributor

ratmice commented Dec 17, 2023

One thing that I don't immediately see how to do with the restrictions in this RFC, that I do see how to implement with
the current sealed trait pattern is feature-based restriction lifting. I don't particularly think this is any kind of deal breaker, but it seemed interesting that the current patterns to achieve this seem a little more expressive in this regard.

Let me give an example:

mod internals {
   pub trait InternalTrait{}
   pub struct InternalApiKey;
}

pub mod internal_api {
   #[cfg(feature = "_unsealed_internal_traits")
   pub use crate::internals::InternalTrait;
   #[cfg(feature = "_internal_api_key")
   pub use crate::internals::InternalApiKey;
}

pub trait Foo: internals::InternalTrait {
  fn foo(key: internals::InternalApiKey);
}

Like I said, I really don't think it's necessary that the restrictions API cover all these weird possibilities... but it seemed like it might be worth mentioning

@jhpratt
Copy link
Member

jhpratt commented Dec 17, 2023

The RFC as written doesn't permit this. However, once the initial implementation is merged (it's waiting on me) I intend to propose a syntax change to be attribute-based. That would resolve a lot of concerns and open the door to things not considered previously (like your example).

@tczajka
Copy link

tczajka commented Mar 26, 2024

This does for traits the analogous thing #[non_exhaustive] does for instantiating structs. However, non_exhaustive is more limited: the only boundary it allows is the crate.

I would like to see similar syntax for structs, something like:

pub instantiate(super) struct Foo {
    ...
};

The #[non_exhaustive] attribute could migrate to instantiate(crate) in an edition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC F-impl_restriction `#![feature(impl_restriction)]` F-mut_restriction `#![feature(mut_restriction)]` T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests