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

Enable cross-module initialization of private fields #145

Closed
bluenote10 opened this issue Apr 13, 2019 · 15 comments · Fixed by nim-lang/Nim#17706
Closed

Enable cross-module initialization of private fields #145

bluenote10 opened this issue Apr 13, 2019 · 15 comments · Fixed by nim-lang/Nim#17706

Comments

@bluenote10
Copy link

bluenote10 commented Apr 13, 2019

Nim doesn't have a straightforward solution for cross-module initialization of private fields. This has a negative impact on designing class hierarchies, especially when involving abstract interfaces. For example:

Library module:

# The library provides an abstract type, with base methods to be implemented by users.
type
  Component* = ref object of RootObj
    id: string # only relevant internally, not to be exposed to users

User module:

type
  UserComponent* = ref object of Component
    internal: SomeState

Nim prohibits to write a standard object constructor on user site, which initializes private fields on both library and user site like UserComponent(id: id, internal: internal). This often leads to suboptimal work-arounds like:

  • Changing fields from private to public only for the purpose of construction. Drawback: Due to the nature of ref objects, the field also becomes fully readable/writable (!) everywhere.
  • Partial construction: For instance, users may first construct UserComponent(internal: internal) and then have to call some setter to actually initialize private fields of the base class (see this forum thread for discussion of such work-arounds). Drawback: Prevents the compiler to verify complete construction (e.g. notnil feature) and is prone to leaving fields uninitialized.

Idea 1

Allow to opt-in for "object constructor only" visibility.

type
  Component* = ref object of RootObj
    id {.initializable.}: string

From other modules the field would stay fully hidden, except for the object constructor, i.e., UserComponent(id: id, internal: internal) is now allowed on user site.

Idea 2

One might argue that whereas hiding fields for read/write access is a good thing, excluding them from initialization actually encourages creating invalid object instances. From this perspective it would make sense to make them accessible in object constructors by default, and instead provide an explicit opt-out, if they really should be fully inaccessible from the outside:

type
  Component* = ref object of RootObj
    id: string # can be initialized
    fullyPrivate {.fullyPrivate.}: string # not even initializable

Idea 3

Usually libraries provide proc based constructors corresponding to a type, especially if the construction is non-trivial or requires input validation etc. For instance, with idea 1 + 2 it wouldn't be clear how the library can enforce a validation of the id field in the object constructor. An alternative solution is to enable forwarding of base class instances to object constructors of sub classes:

# library
proc newComponent*(id: string): Component =
  # optionally validate id
  Component(id: id)
# user
proc newUserComponent(): UserComponent =
  UserComponent(newComponent("myid"), internal: SomeState())

Here I made the assumption that the first argument of the object constructor is the base class instance. Alternatively, an existing keyword could be leveraged to keep the colon expression consistency, for example UserComponent(using: newComponent("myid"), internal: SomeState()).

@Araq
Copy link
Member

Araq commented Apr 14, 2019

This seems to assume that Nim's object construction syntax should be used directly in client code but Nim doesn't do that at all, see all the initT / newT procs everywhere in the stdlib. Thanks to default parameters and names parameters constructor procs work much better than the object construction syntax.

@bluenote10
Copy link
Author

bluenote10 commented Apr 14, 2019

This seems to assume that Nim's object construction syntax should be used directly in client code [...]

No, not at all, this still assumes that client code introduces initT / newT procs. The thing is that in the end these procs still have to invoke the object constructor internally. Or how should I write newUserComponent without using the object constructor UserComponent?

The problem is that if the types are defined in different modules we cannot properly construct the user object:

# Currently that's all what Nim allows, leaving field `id` uninitialized :(
proc newUserComponent(): UserComponent =
  UserComponent(internal: generateState())

Idea 1 + 2 would enable to at least fully initialize the user object, but it would "bypass" constructor procs defined for the base type:

proc newUserComponent(): UserComponent =
  UserComponent(id: generatedId(), internal: generateState())

Idea 3 would be my favorite, because it would allow to compose constructor procs:

proc newUserComponent(): UserComponent =
  # newComponent() is the constructor proc of the base type, which initializes the `id` field.
  UserComponent(using: newComponent(), internal: generateState())

However, idea 3 may be the most difficult to implement.

@Clyybber
Copy link

@bluenote10 The client defined initT / newT of the UserComponent should invoke the initT / newT of Component, which should be defined by the library.

@bluenote10
Copy link
Author

bluenote10 commented Apr 14, 2019

@Clyybber That is what idea 3 is about. To my knowledge, there is currently no way of composing the construction of a subclass with the initT/newT of its parent class.

@Clyybber
Copy link

@bluenote10 I was thinking of something like this:

Library module:

# The library provides an abstract type, with base methods to be implemented by users.
type
  Component* = ref object of RootObj
    id: string

proc newComponent*(id: string): Component = Component(id: id)

User module:

type
  UserComponent* = ref object of Component
    internal: SomeState

proc newUserComponent(somearg: SomeState): UserComponent =
  result = UserComponent(newComponent("test"))
  result.internal = SomeState

but it seems like this is not working. It compiles fine, but upon actually calling newUserComponent it fails with:

Error: unhandled exception: invalid object conversion [ObjectConversionError]

@bluenote10
Copy link
Author

Yes, type conversion doesn't work in that direction. You can make it work by using this trick from the forum thread, but it still has obvious downsides:

  • The pattern is too tedious for such a fundamental problem (syntactically verbose).
  • Requires incremental initialization, which goes against the idea to statically proof initialization (see RFC Enforce initialization in more contexts / everywhere / up to debate #49). In particular, {.requiresInit.} and not nil cannot be used, and fields will be set twice (default(T) followed by actual initialization).

@Araq
Copy link
Member

Araq commented Apr 14, 2019

Or how should I write newUserComponent without using the object constructor UserComponent?

Ok, I see, thanks for the clarification.

@krux02
Copy link
Contributor

krux02 commented May 8, 2019

So you want protected from Java/C++/languageX in Nim as well, am I correct?

@bluenote10
Copy link
Author

bluenote10 commented May 8, 2019

So you want protected from Java/C++/languageX in Nim as well, am I correct?

No, not at all.

All I want is to correctly initialize private fields, which is a fairly big design flaw in my opinion. Look at how many libraries out there make their fields public just for that reason.

@krux02
Copy link
Contributor

krux02 commented May 11, 2019

All I want is to correctly initialize private fields, which is a fairly big design flaw in my opinion

Well, nim isn't perfect. But if a type from a module has private fields, that module should be responsible to initialize it, not the user of the module. So yea you might be correct that it is a design flaw that you want to initialize that private field.

Look at how many libraries out there make their fields public just for that reason.

Can you name me one?

@krux02
Copy link
Contributor

krux02 commented May 11, 2019

I don't see a reason to change anything here, this is how it should be done:

var state: int

type
  Component* = ref object of RootObj
    id: string # only relevant internally, not to be exposed to users

proc init*(self: Component) =
  # maybe some synchronization
  self.id = $state
  state += 1

user module:

import component

type
  UserComponent* = ref object of Component
    internal: int

proc init(self: UserComponent) =
  init(Component(self))
  self.internal = 13

proc newUserComponent(interal: int): UserComponent =
  result.new
  result.init

let uc : UserComponent = newUserComponent(13)

echo uc[]

@bluenote10
Copy link
Author

The RFC & forum thread already mention this workaround, with details why it's not a general solution. To clarify: How do you ensure that Component and UserComponent are always constructed via constructor procs so that nothing can violate class invariants by calling plain object constructors Component() / UserComponent() directly? Nim's answer is the requiresInit pragma. However it cannot be used here due to the incremental construction. The same applies for not nil fields. Note that the third proposal would solve all these issues nicely.

Regarding examples: I'm traveling right now, so I can't do a thorough research, but I was facing the private/public problem in many of my packages and I remember @mratsim struggling with it as well.

@krux02
Copy link
Contributor

krux02 commented May 15, 2019

A. This is a pattern, not a workaround.
B. Please provide a like if you reference something from the forum, otherwise I can't validate what you say.

How do you ensure that Component and UserComponent are always constructed via constructor procs so that nothing can violate class invariants by calling plain object constructors Component() / UserComponent() directly?

I usually document it. The existence of private fields and a constructor like newObjName is already a very big hint that the object should probably not be used raw. Only if this is proven to not be sufficient, then I see a reason to do more here.

At the moment I think it might be a good idea to emit warning when plain object constructors are used on an object that has private fields and is declared in another module.

@bluenote10
Copy link
Author

A. This is a pattern, not a workaround.

Call it whatever you like, it is ugly (bunch of statements instead of a single expression), potentially inefficient (unnecessary default(T) initialization), and doesn't work in general. For instance in the component library example, if the base type enforces this pattern, it prevents all users to ever use a not-nil field in their components, which sucks. See also @Araq's comment here on statement-based vs expression-based initialization -- I just noticed that it alludes to something which looks like proposal 3.

However, the biggest problem with the pattern is that people typically don't use it and go for public fields instead.

B. Please provide a like if you reference something from the forum, otherwise I can't validate what you say.

The link was in the RFC and I cross-posted a link on the forum thread. It would be nice if you could read the entire RFC before criticizing ;).

At the moment I think it might be a good idea to emit warning when plain object constructors are used on an object that has private fields and is declared in another module.

That goes in the right direction, and I thought about it as well before. The problem is that there are cases where default(T) initialization is perfectly fine, so it is not something that needs a warning in general. It would be rather annoying having to turn off the warning in those cases which should allow default(T) initialization and those that don't. {.requiresInit.} exactly serves the purpose of differentiating these use case and thus I think it is better to go for a solution which plays nicely with this existing mechanism in Nim.

@Araq
Copy link
Member

Araq commented May 20, 2019

However, the biggest problem with the pattern is that people typically don't use it and go for public fields instead.

Well if you put it this way, we need to educate more people about the pattern then before we try to add yet-another feature to Nim which might backfire.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants