Bikeshed: mut-restricted field, impl-restricted trait syntax

Except for the initialization, right? Also, mutability currently doesn't interact with visibility (something is either mutable or immutable/shared, regardless of where in the code it is used). That's why I see writeability as a distinct concept.

I don't see implicitness as a problem per se. For example, Send and Sync bounds being implicit is ok, because the syntax to remove them is different: The question mark in front of ?Send communicates to the user that this relaxes a trait bound. There is no such marker at mut(crate): It looks exactly like a visibility modifier (except that pub is replaced with mut), so people will probably be confused why this is a restriction rather than an added feature. Especially since you're re-using the mut keyword, which is short for "mutable", but mutability is opt-in everywhere else.

2 Likes

I addressed that right after the bit you quoted. I won't go into detail as to why I am including construction, as that is not relevant to the discussion of syntax, which is what this thread is about.

Do you have any proposals for different syntax? Keeping in mind that having this work in all editions is very much preferred, as is avoiding the introduction of new keywords.

Both mut(self) and the alternative of pub(mut in self) have the disadvantage of being somewhat verbose for the common case. It seems like the common cases will be mut in self and occasionally mut in crate. Without sacrificing orthogonality or generality, I'd love to have a syntax that optimizes for the common case.

Brainstorming:

  • pub(const) (semantic meaning: "this is public but only as a constant"). Might be confusing because it has a substantially different meaning than const.
  • pub(readonly). I think this would work as a contextual keyword in this context. This doesn't cover the mut in crate case, but we could allow pub(readonly, mut in crate) or pub(readonly) mut(crate) for that case. I'd expect that case to be somewhat less common, so making it slightly more verbose seems fine.
  • pub(ro). Same as readonly but more abbreviated.
  • pub(read). Same as readonly, but in addition to being shorter, this potentially fits better with other future capabilities in a capability-based sytem. readonly is an adjective; read is a verb that pub users are permitted to do.
  • pub(mut in self). Not that much longer than pub(readonly). May be less clear for the common case, though. But it naturally extends to pub(mut in crate).

Of the various options, I think I like pub(read) best, with the extension of pub(read, mut in crate) if you want to allow mutability elsewhere.

For traits, pub(impl in self) and pub(impl in crate) seem most clear to me. But we could add a pub(sealed) as an alias for pub(impl in self) for the common case, perhaps..

11 Likes

I'm not super familiar with the reasoning for requiring arbitrary module paths in visibility specifiers to use "in" pub (in crate::path::to::whatever), would there be problems allowing paths without that disambiguator pub(mut in crate::path::to::whatever)?

pub(mut in in crate::path::to::whatever) would be less than ideal :sweat_smile:

I would expect to have to write that as pub(in path::to:whatever, mut in crate).

It's not obvious to me that having the mut-ness in the pub is the best place, since having it as part of the vis matcher doesn't feel right. That'd start getting allowed in so many macros where it shouldn't go.

2 Likes

In contrast [1], I find putting this modifier into pub the most intuitive by far; these are all knobs on who can do what with the field and that feels like they should go in the same place (even if pub evolves to be a poor over-all description). I also don't really want to end up with

pub(crate) mut(self) impl(friends) kitchen(sink) foo: Bar,

I'd rather just go with attributes at that point.


  1. and as a typical bike-shedding outcome ↩︎

7 Likes

Funnily enough, PHP is currently debating adding a similar feature (and also mostly bikeshedding about the syntax): Asymmetric Visibility

It currently proposes public private(set) int $property;, based on Swift's syntax for the same feature.

I just wanted to point this out as prior art - I think mut and mod certainly make more sense with Rust's existing use of the keywords, rather than private and set.

(Note that PHP already has a readonly keyword for init-only properties, and that the feature proposed here would also be described well by "Asymmetric visibility" rather than "readonly".)

2 Likes

I think there's a fair amount of value in letting a field be mutable but only within the defining crate. Are you suggesting these accept a path? If not, surely that's a limitation worth considering.

Granted, but pub(crate) is widely used and I haven't seen anyone say "this is too much to write". mut(self) / mut(mod) is basically the same length (slightly shorter, but it's not just a single keyword).

Part of my concern with having the restrictions integrated into the existing visibility modifier is that this means what a valid visibility is depends on the type of item it's being attached to. While this would likely be rejected in AST validation, that pushes the error farther in the compilation process. This concern also applies to the :vis macro matcher; the extended syntax would be accepted even if the macro author doesn't desire this behavior. @scottmcm voiced these concerns, both in this thread and here, which is slightly after this sort of syntax was first mentioned. I agree with his concerns for the reasons stated.

Accepting it as part of visibility would also increase the size of a number of structs, namely those that don't even accept the restriction. This should always be secondary in decision making, however. The parser/AST could presumably be adjusted to avoid this drawback.

Formatting could help here. But this certainly doesn't apply when only adding one restriction (impl and mut can never be on the same item). Worth considering? Sure. But I don't foresee a future where there are a slew of restrictions that all apply on a single item.

1 Like

I'm not sure if this is quite for or against this sentiment, but crate as an alternative for pub(crate) stuck around in unstable for a long time because of a "pub(crate) is unwieldy" expectation, but was recently removed due to parsing concerns. Rustdoc made significant use of crate as a visibility, and the switch to replace crate with pub(crate) was IIRC considered a positive minimally to approachability of the codebase.

3 Likes

I think I'd prefer an attribute in order to not make the grammar more complicated. And since attributes go in a separate line, there's no need to make extremely short.

For example, it could be

struct Foo {
    #[readonly]
    pub field1: u8,
    #[readonly(not in crate)]
    pub field2: u8,
}

Where #[readonly] means "readonly except in this module" and #[readonly(not in crate)] means "readonly except in this crate".

Not to say it's a good or necessary thing, but if we have "derived fields", readonly field can come out naturally:

struct Rectangle {
     pub width: f32,
     pub height: f32,
} deriving {
     pub area: f32 = self.width * self.height,
}

Then read-only fields can be just:

struct Foo{
     real_bar: f32,
} deriving {
     pub bar: f32 = self.real_bar,
}

This may be an unhelpful divergence, but if this had existed from the beginning I wonder if "externally immutable" would be the default and the syntax we might have arrived at would be something like:

mod foo {
    pub struct Foo {
        pub alpha: u8,
        pub mut beta: u8,
    }

    // okay to mutate `alpha` here
    // okay to mutate `beta` here
}

// error to mutate `alpha` here
// okay to mutate `beta` here

My sense is that mut(self) actually communicates externally immut(!self). Perhaps this could be established via some mode switch, like pub pure struct Foo { ... } (or since this is a bikeshedding thread I should suggest pub 🦀 struct Foo { ... }).

3 Likes

edit: pub(as readonly)

This looks best to me. The public API of the struct is the primary concern here, and this directly says what the public API is. The problem with mut(self) is that from the API perspective it's actually communicating the negative: not mut(not in self), and negations are harder to reason about.

I don't think it's necessary to support fine-grained paths here. I haven't seen pub(in mod) syntax used in crates-io crates. It may be libstd's unique requirement. Other projects split into multiple crates, or just don't care that much, and for them pub(crate) is good enough.

2 Likes

Couldn't the AST concerns be addressed by introducing a "FieldVis" variant / matcher? Is there concern that that would require a bunch of changes in the compiler?

(Member of wg-grammar but acting on my own, not for the wg)

pub(readonly) is problematic as a contextual keyword for the same reason pub(in path) requires a real keyword introducer: it requires lookahead[1]. In this case it's bounded (LL(5)?) lookahead since it is a fixed length tokenstream to the deciding fork, but it's still important to consider.

The problematic case is tuple fields. Consider

struct readonly;
struct Type;

pub struct TupleStruct(
    pub ( readonly ) Type ,
    //  ^ ^        ^ ^
    pub ( readonly ) ,
    //  ^ ^        ^ ^
    pub ( readonly , ) Type ,
    //  ^ ^        ^ ^ ^
    pub ( readonly , ) ,
    //  ^ ^        ^ ^ ^
    pub ( readonly , Type ) ,
    //  ^ ^        ^ ^
    pub ( readonly , mut in crate ) Type ,
    //  ^ ^        ^ ^
);

Pointed at is the first unknown token (is this part of the restricted visibility or a tuple type?) through the deciding token.

This is an interesting case since it's LL(2) "fat token" lookahead (i.e. with a matched brackets token tree stream).

More problematic, though:

macro_rules! m {
    ($vis:vis(readonly)) => {}
}

m!(pub(readonly));

The $:vis macro matcher has a maximally permissive follow set, and as such any changes to the visibility grammar cannot be reflected in it without an edition change like was done for $:pat or being macro-breaking.


  1. As I suppose do most contextual keywords, to be fair. ↩︎

7 Likes

Fair. I was the one that did the actual removal from the compiler, so I'm aware of the history. If there is a shorter way that's just as clear, I'm all ears! I just don't think it's easy, particularly given the grammar concerns you noted.

This fundamentally changes what a field is.

That would have been great in my opinion. But unfortunately that ship has sailed, barring an edition change (which would be quite major).

For sure. I think I've only used pub(in mod) a few times myself, and even then it was quite niche. But limiting it to just crate/mod mutability, without the option of choosing the other? I at least want the choice between those two, even if nothing else.

What if pub(as readonly) was used to simplify the lookahead?

3 Likes

Ah, good point. We might be able to improve that with a non-contextual keyword in a future edition, and we also could just accept the lookahead requirement, but it's good to know that that's an issue.

I do still think pub(read) seems clearer than pub mut(self). But that does seem like a deliberate tradeoff we'd want to consider.

sorry for lately reply.

IMHO, it is motivations that decides the syntax.

if you just not want people modify your crate accidently, the unsafe keyword should be enough. If you ask people not to modify the code since you have the copyright, you should not provide the source code (since source code means "modify it when you want" directly) and thus no need to specific what is mut in crate and readonly outside it.

For other reason, (e.g., lazy-static, initialize once and never changes), maybe there was lots of discussion.