External initialization of private struct fields should be disallowed

[dlangBugzillaToGithub]

Max Samukha (@maxsamukha) reported this on 2019-11-06T07:05:22Z

Transferred from https://issues.dlang.org/show_bug.cgi?id=20358

CC List

• RazvanN
• Simen Kjaeraas

Description

s.d:

struct S {
    private int x;
}


test.d:

import s;

void main() {
    S s = {1}; // should fail to compile
    auto s2 = S(1); // should fail to compile
}

[dlangBugzillaToGithub]

razvan.nitu1305 commented on 2019-11-06T09:28:40Z

This issue is invalid. Default constructor is allowed to initialize private fields, otherwise you would end up with partially initialized objects. If this behavior does not suit you, you can disable the default constructor.

I suggest closing this as invalid.

[dlangBugzillaToGithub]

simen.kjaras commented on 2019-11-06T10:24:34Z

Supporting Max's argument: test.d's author should have no knowledge of S's private fields and their allowed values. A change in S's internals would lead to breakage in unrelated modules.

The solution here would be to define the default constructor as private when a struct has private members. Inside s.d this would allow the exact same usage as now, while not giving other modules access to internals that arguably shouldn't be available to them. Constructors would need to be defined explicitly to initialize private fields when called from other modules.

I started out agreeing with RazvanN on this, but I think the above is the best solution.

[dlangBugzillaToGithub]

maxsamukha commented on 2019-11-06T11:07:44Z

(In reply to Simen Kjaeraas from comment #2)

> The solution here would be to define the default constructor as private when
> a struct has private members. 

That would be better, but simply making the default constructor private would outlaw other valid use cases:

struct S {
   int x;
   private int y;
   int z;
}

// partial initialization should still be allowed
S s = {1};
S s2 = {1, z: 2}; // etc

In other words, private should be the respective parameters of the default constructor.

[dlangBugzillaToGithub]

b2.temp commented on 2019-11-06T15:48:12Z

I vote for closing as invalid. 

Default constructors should be seen as real constructors so the private fields are not written directly but rather copied from the constructor parameters to the fields (conceptually). Finally explicit constructors can be used in a way that prevents initialization of private fields, if required.

[dlangBugzillaToGithub]

maxsamukha commented on 2019-11-07T07:38:02Z

I disagree. Implicit constructors should not magically give public assess to private members. That breaks encapsulation. I bet you wouldn't like it if the compiler implicitly generated public setters for private fields. Mutating private fields with an external initializer is analogous to that.

[dlangBugzillaToGithub]

simen.kjaras commented on 2019-11-07T09:57:42Z

In favor of closing as invalid: If your type has an invariant that requires private fields to have specific values, you should define a constructor that establishes said invariant. If you don't, you've essentially told the world you'll accept any and all values.

If we were designing D from the start, I'd go with the private constructors I described above*. As it is, changing this would break code for relatively small benefit.

Even if default constructors are thought of the way Basile indicates in comment #4, that's no excuse for S s = {x: 1};, though - that should be disallowed.


* Curly bracket initialization could be handled separately by respecting field visibility rules, or simply disallowed - it's a blunt tool for simple types, and more complex types with invariants can simply define a constructor. Yes, there are valid use cases, but they can easily be handled by explicitly defined constructors.

[dlangBugzillaToGithub]

maxsamukha commented on 2019-11-07T15:38:36Z

(In reply to Simen Kjaeraas from comment #6)
> In favor of closing as invalid: If your type has an invariant that requires
> private fields to have specific values, you should define a constructor that
> establishes said invariant. If you don't, you've essentially told the world
> you'll accept any and all values.

But there is also lazy initialization:

// invariant is x == 1.
// neither disabled default ctor nor non-default ctors are wanted
struct S {
    private int x;
    void foo() {
        if (x == 0) x = 1;
    }
}

auto s = S(2); // problem

I agree that the issue is probably minor, because most structs with private fields will have at least one constructor defined or disabled anyway. I disagree that the issue is invalid.