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

Functional record update: private fields should not throw errors if not explicitly used in literals #70564

Open
RReverser opened this issue Mar 30, 2020 · 12 comments
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@RReverser
Copy link
Contributor

RReverser commented Mar 30, 2020

I tried this code:

mod module {
    #[derive(Default)]
    pub struct S {
        pub public_field: u32,
        _private_field: u32,
    }
}

use module::S;

pub fn f() -> S {
    S {
        public_field: 42,
        ..S::default()
    }
}

I expected to see this happen: code compiles successfully.

Instead, this happened: rustc throws an error

error[E0451]: field `_private_field` of struct `module::S` is private

  --> <source>:14:11

   |

14 |         ..S::default()

   |           ^^^^^^^^^^^^ field `_private_field` is private

As a user, I field this confusing, because I'm not trying to access or set the private field explicitly anywhere in the literal.

Meta

Applies to all known rustc versions.

I understand why this happens in principle, if the literal is desugared literally (no pun intended) to:

let s_rest = S::default();
let s = S {
  public_field: 42,
  _private_field: s_rest.private_field,
};

However, it seems that this code could be equally expanded to:

let mut s = S::default();
s.public_field = 42;
let s = s;

This way an immutable literal could be used with many more kinds of structures without users manually resorting to the temporarily-mutable variable as in the 2nd example.

Do I miss some other difference between the two that would break some existing usecases if the implementation is changed?

@RReverser RReverser added the C-bug Category: This is a bug. label Mar 30, 2020
@jonas-schievink jonas-schievink added C-feature-request Category: A feature request, i.e: not implemented / a PR. T-lang Relevant to the language team, which will review and decide on the PR/issue. and removed C-bug Category: This is a bug. labels Mar 30, 2020
@petrochenkov
Copy link
Contributor

Do I miss some other difference between the two that would break some existing usecases if the implementation is changed?

The desugaring change would break existing use cases, yes.
See https://internals.rust-lang.org/t/proto-rfc-expanded-functional-record-update/3077/2.

@RReverser
Copy link
Contributor Author

RReverser commented Mar 30, 2020

@petrochenkov Interesting. It's hard to understand from the branch code, but is desugaring in your experiment same as I have shown above? I don't really understand why Drop would be an issue here, since I'm not trying to move any fields out.

@RReverser
Copy link
Contributor Author

I don't really understand why Drop would be an issue here, since I'm not trying to move any fields out.

Tried to add Drop to my S and the desugaring still works as expected. Could you please provide an example code that would break?

@RalfJung
Copy link
Member

RalfJung commented Mar 30, 2020

This discussion also recently came up on IRLO again: https://internals.rust-lang.org/t/pre-rfc-relaxed-non-exhaustive-structs/11977

Tried to add Drop to my S and the desugaring still works as expected. Could you please provide an example code that would break?

As the linked comment says: "FRU from borrowed contexts, you can’t move the whole value, but can copy individual Copy fields"
So, try a situation where the thing after the .. is borrowed, not owned. This commit has some examples.

@RalfJung RalfJung changed the title Private fields should not throw errors if not explicitly used in literals Functional record update: private fields should not throw errors if not explicitly used in literals Mar 30, 2020
@RReverser
Copy link
Contributor Author

@RalfJung Ah, I see, haven't realised it's possible. It could be interesting to enable this at least for owned case, but I suppose that's what makes feature request more nuanced (which explains previous forum discussions).

@jonhoo
Copy link
Contributor

jonhoo commented Jun 20, 2020

I'll just add to this that if we could use the alternative desugaring, this would also make functional record update syntax work with #[non_exhaustive] types as well, which would be really nice.

jonhoo added a commit to jonhoo/inferno that referenced this issue Jun 20, 2020
This is a breaking change.

See also
rust-lang/rust#70564 (comment)
for why this change is a little sad.
jonhoo added a commit to jonhoo/inferno that referenced this issue Jun 20, 2020
This is a breaking change.

See also
rust-lang/rust#70564 (comment)
for why this change is a little sad.
@jplatte
Copy link
Contributor

jplatte commented Aug 5, 2020

Would it be feasible to make this work without fundamentally changing the desugaring? I.e. somehow "ignore" the non-exhaustiveness of the struct (and privacy of implicitly-copied / -moved fields) when it is used with FRU?

@jplatte
Copy link
Contributor

jplatte commented Aug 5, 2020

Also either this or #63538 should probably be closed as a duplicate.

@Rua
Copy link
Contributor

Rua commented Feb 6, 2022

There are several threads on Rust internals discussing this problem:

There is mention of an RFC to introduce a new syntax, so that existing uses of non_exhaustive don't break, but it seems that it never went anywhere. Is there someone that can get the proposal going again? I would do it myself but I have no understanding at all of Rust internals so I'd just make a mess of it.

@starkat99 do you think you could create the RFC you proposed back then?

@sffc
Copy link

sffc commented Sep 16, 2022

Another option suggested by @cchiw is to allow the normal functional record update syntax on non_exhaustive structs and structs with private fields if they implement Copy. That gets around the issues involving destructors mentioned earlier in this thread. This seems like a no-brainer since it is forwards compatible and doesn't affect any syntax.

Two other solutions involving syntax are:

  1. #[non_exhaustive(pub)] that declares that the struct won't gain private fields in the future. Seems like a workable solution except for explaining what #[non_exhaustive(pub)] means and why you'd want to use it.
  2. A new FRU operator that desugars to the code in the OP. This means there are two similar but incompatible FRU operators in Rust, which is a little unfortunate, but I think it could be worth the cost.

CC @Manishearth

@ebkalderon
Copy link
Contributor

ebkalderon commented Jan 31, 2023

I like the idea of introducing a minimal version of this feature which only allows the syntax on non-exhaustive structs with private fields if they are all Copy, at the very least. Besides being forward-compatible and taking the least amount of effort to implement, it would go a long way to make constructing non-exhaustive Config structs much smoother, for example.

// Present
let mut config = Config::default();
config.a = 123;
use(config);

// Future
use(Config { a: 123, ..Default::default());

The real question is, though, is how intuitive and, conversely, how surprising such semantics would be to an average user. I could imagine such a user running into the Copy restriction while working with some external crate and thinking: "Why can this non-exhaustive struct be constructed with ..Default::default() but not this other non-exhaustive struct? Hmm, the error message claims the fields of the first one are all Copy, but those of the second one aren't. Wait, why does that matter?" Perhaps an explanatory help message would be good to have here.

@jayhf
Copy link

jayhf commented Feb 18, 2023

Could this be a use case for supporting !Drop referenced in #68318? FRU could initially be implemented for Copy types only and later be expanded to anything !Drop, which Copy should imply

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. 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

10 participants