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

Allow @call to call private functions #8779

Closed
david-vanderson opened this issue May 15, 2021 · 18 comments
Closed

Allow @call to call private functions #8779

david-vanderson opened this issue May 15, 2021 · 18 comments
Labels
proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Milestone

Comments

@david-vanderson
Copy link
Contributor

Sometimes you find the exact function you need in the standard library, but it's not public. There should be a way to override that and call the function anyway.

In the zig project I'm working on, it works well to:

  1. std.net.tcpConnectToHost (blocking)
  2. std.os.setSockFlags to switch socket to nonblocking
  3. read/write to socket (nonblocking)

The problem is that std.os.setSockFlags is private. This sucks as a user. You either have to copy-paste the function into your project or edit the standard library file.

Options:

  1. Change @call to work on private functions as is
  2. Change @call to work on private functions with a new CallOptions modifier .override_private

This would balance the desire to have private functions with giving users an escape hatch and satisfy zig's commitment to "Communicate intent precisely" while serving users well.

What do you think?

@AssortedFantasy
Copy link

Seems like this would really only be useful for bandaid hacks. Like the underlying problem seems to be in the standard library design and not any kind of defficiency in@call or function scoping rules.

If you think some functions in the standard library are useful and should be public that could be done. Why not submit a PR and create an issue for that? It's not even a breaking change after all.

@david-vanderson
Copy link
Contributor Author

I agree, but then as a user you are stuck until the library is updated, which could be a substantial amount of time. Compared to the other options you have at that point (copying the code), this kind of bandaid is exactly what you need.

In a former life as a Java developer, this was a huge pain point. Zig can treat it's users better.

@kristoff-it
Copy link
Member

@david-vanderson I think you have a valid point when it comes to the fact that one should be able to exert control over how sockets are opened independently of the global io_mode setting. I have encountered this exact problem myself, but the reality is that std.net is not exposing the right interface. The entire stdlib is due a review and networking+async is in a particularly rough spot due to the fact that async is still considered experimental. I am confident your problem will be solved once we put more focus on impoving the stdlib.

That said, your proposal is IMO the wrong solution not just in this case, but also more in general because it would muddy the distinction between private and public API of not only the stdlib, but of any package, which in turn would affect negatively a library writer's ability to make backwards compatible changes to their code.

@andrewrk andrewrk added the proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. label May 16, 2021
@andrewrk andrewrk added this to the 0.9.0 milestone May 16, 2021
@david-vanderson
Copy link
Contributor Author

Thanks for the response! I totally agree that the std lib could be better in this instance, and I'm hoping to help in that effort.

I agree that this proposal is not about what the best solution is. I'm trying to make zig work for users who find themselves in a less-than-optimal situation. I don't think the right solution is to tell users to either copy the library code, or just wait until we update the library.

Is there another option for users at this point that I missed?

I also agree that the public/private distinction is important for library writers. That's why I suggested having to jump through @call, maybe with a specific CallOptions distinction. This gives the user an immediate workaround while clearly communicating that the library author does not support that. If the workaround breaks on a library update, I think users would understand.

I understand this is a tradeoff, especially as currently zig is moving very fast, so it's easy to update things. But also imagine the future where a user can't easily update the installed zig std lib (like in a corporate managed environment, or managed hosting).

@marler8997
Copy link
Contributor

When I have run into this in the past I copy the code. This nice thing about this "icky" solution is it's a marker/reminder that you're doing a bad thing by using something that's private. Another nice thing about your copy, is that if the private implementation changes, your copy will still remain the same and will continue to work. This solution doesn't break the library author's assumption that they are free to change their private code without breaking other people (which is what private is for). Making it easier to use private code is encouraging libraries to break this assumption which in turn encourages developers to write "brittle" code.

So, the current solution to copy private code provides a healthy amount of discouragement and ends up making the project more robust than it would be if it referenced the libraries private code directly. This is the sequence of events I would follow when I'm in this situation.

  1. copy the private code into your own codebase for now, mark it or put it in a place that will make it easy to remove later
  2. submit a change to the library to make the private code public
    • in this process, the author may want to tweak the interface for better public consumption, possibly refactor it a bit, maybe add some tests to ensure it doesn't break, and may want to add some documentation comments
  3. once your change is in, update your project by removing your copy and using the new shiny public code, this change will "feel" very nice :)

@david-vanderson
Copy link
Contributor Author

What do you do if the function you copy calls other private functions? Do you end up copying the whole library file?

Can other people respond with what they do in this situation? Does everybody copy the code?

@marler8997
Copy link
Contributor

What do you do if the function you copy calls other private functions? Do you end up copying the whole library file?

Yup, copy all the private stuff. Of course, if you had to copy the entire library that would mean the entire library was private, which wouldn't be a very useful library :)

@ghost
Copy link

ghost commented May 16, 2021

This is merely a personal opinion, but I've long felt that language designers put too much energy into policing safety features and interfaces, as opposed to merely checking them. The desire to make the language proof against bad programmers is understandable, but ultimately futile (just look at Java). That's why I'm generally in favor of giving the programmer complete control when asked for. If you in your capacity as programmer-in-charge decide that it's reasonable to call a private function in a library, then I don't see any problem with having @call(.{ .override_private = true }, foo, .{}); to do just that. Sure it's a hack, but probably less of a hack than forking the library just to use (or debug) that one function. That this should be used sparingly goes entirely without saying.

@ikskuh
Copy link
Contributor

ikskuh commented May 16, 2021

Just a question that sparks my mind:

Are you all sure you're proposing the right language feature here? @call takes a function value or pointer whereever it might come from. Obtaining this pointer is not possible for non-pub symbols, so @call cannot change anything about that anyways (not 100% true technically, but orthogonality is a good thing). Also why should i only be allowed to access private functions, but not variables?

I feel that the original proposal should actually be

Add an explicit builtin that allows accessing types privates declarations from outside a module.
A possible variant would be:

fn @privateField(comptime T: type, comptime name: []const u8) [fieldreference]

@david-vanderson
Copy link
Contributor Author

@marler8997 - Haha! Do you run into many problems copying parts? I thought maybe a library with tight coupling between different functions/types would be hard to copy part of. Although I have found zig's explicit self pointer helps (as opposed to an implicit this pointer).

@batiati
Copy link

batiati commented May 16, 2021

It can be faced as using "undocumented APIs", that kind of practice is not so uncommon in the software industry. Of course, it's against best practices, you can tamper with the internal state and you have no guarantee regarding future compatibility, but at least I think it should be possible to do it in Zig.

As a C# developer, sometimes I needed to call private methods through reflection, and I was glad to have this resource, otherwise, it would be unviable using the library.

@david-vanderson
Copy link
Contributor Author

@zzyxyzz - Agree, and thanks for saying it!

@batiati - That makes sense, thanks for the experience from C#!

@MasterQ32 - No, I'm not sure I'm proposing the right thing. I thought all fields were public? Did that change? In that case, your proposal would be better. If you wanted to call a private function, would you do:
@call(.{}, @privateField(foo, "privateFunction"), .{}) ?
@call(.{}, @field(foo, @privateField(Foo, "privateFunction")), .{}) ?

@ikskuh
Copy link
Contributor

ikskuh commented May 16, 2021

@MasterQ32 - No, I'm not sure I'm proposing the right thing. I thought all fields were public? Did that change? In that case, your proposal would be better. If you wanted to call a private function, would you do:
@call(.{}, @privateField(foo, "privateFunction"), .{}) ?
@call(.{}, @field(foo, @privateField(Foo, "privateFunction")), .{}) ?

@call(.{}, @privateField(Foo, "privateFunction"), .{ foo });
@privateField(Foo, "privateVariable") = 10;

Maybe @privateDecl is better here

@emidoots
Copy link
Contributor

Not a huge fan of this, I feel it goes against the spirit of "no hidden control flow" somewhat (although maybe that's a stretch.)

In large systems with many complex inter-working dependencies, the ability to do this can often be detrimental to other parts of the system. For example, mutating private global state/options within a library having negative side effects in other libraries. This can often be non-trivial, because you've thrown out the contract consumers and producers agreed upon.

This can be something as non-trivial as:

  • Downstream library like a JSON parser doesn't expose a global option to ignore unrecognized fields, instead requiring you specify it each time you call a parse function.
  • Downstream libraries A, B, and C all use the JSON parser through its public contract that unrecognized fields will produce an error.
  • Another downstream library D uses the JSON parser, but "really wants ignoring unrecognized fields to be the default" and "there is an issue on the tracker to consider it, so there's no harm in forcing it today through a private method"
  • An HTTP client uses libraries A, B, and C and operates as expected.
  • Importing library D now has side effects that subtly break the HTTP client library.

@david-vanderson
Copy link
Contributor Author

@MasterQ32 - Thanks for the clarification!

@slimsag - Totally agree that using a library in a non-public way can be bad. That's why the proposed syntax clearly communicates to the user they are using the library in a non-intended way. When the user is in this situation they need some way of moving forward. Do you agree with @marler8997 that the user should copy the functions/library?

@marler8997
Copy link
Contributor

Haha! Do you run into many problems copying parts?

Not in the cases I've run into so far.

I thought maybe a library with tight coupling between different functions/types would be hard to copy part of.

It's possible but I can't think of any case at the moment. Seeing some examples that would make copying unreasonable might be useful. Do you have any or can you think of some?

@emidoots
Copy link
Contributor

@david-vanderson My point is that in this case, the affected user may not even be the one using the @call function. It might not even be clear to them that it is being used. A downstream package author can use @call and have a "it works for me" situation, while downstream consumers can find "I cannot upgrade this package" or "upgrading this package silently broke something for me"

I do agree that copying/forking the library is often the best choice in this situation, if an immediate workaround is required.

@david-vanderson
Copy link
Contributor Author

@marler8997 No I don't have any examples where copying was very difficult.

@slimsag Oh I see, I misunderstood. You are saying one library might screw up a different library through this @call backdoor. I hadn't thought of that.

Perhaps copying is the right solution in zig for this problem. If examples come up where copying is difficult then we can revisit this. I'll close this issue in a few days unless anyone speaks up otherwise.

Thank you very much everyone for thinking through this!

@andrewrk andrewrk modified the milestones: 0.9.0, 0.10.0 May 19, 2021
@andrewrk andrewrk modified the milestones: 0.10.0, 0.8.0 May 20, 2021
@ghost ghost mentioned this issue Oct 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Projects
None yet
Development

No branches or pull requests

8 participants