I think it’s totally sureal to have to check each and every function in a PR to see if it’s documented/exported/in-the-manual/etc.
Think of something like VSCode, Slack, etc - anything that can be extended by plugins and integrations and any module/plugin having access to any other module/plugin.
Anyway, I don’t think we can reach an agreement beyond this point as we’re literally going in circles. It’s been a good discussion and it’s good to have this here in the open. I appreciate all the input and respect all the opinions, even if they can’t be reconciled.
Most modules follow the convention that exported symbols are public, so you would only have to check ones that were specifically imported or are accessed with ..
While I agree that some tooling would be nice for this, this is usually a small part of reviewing a PR, so apparently this is not a key issue for most people. For example, the commit where you introduced this to Genie has added 4 lines of code. Personally, I don’t find checking this much code surreal, YMMV.
Still, in this particular case, @kwdef is a) very useful and b) often utilized. Just by looking at other people’s code, one will likely discover it. The fact that it has a very nice docstring (with example!) could make one believe that it is save to use without checking the manual first. Of course one might argue in the case of breakage that the user neglected their due diligence, but I can’t help but feel that’s not entirely warranted either.
Personally I quite like and try to use this convention. Yes, it’s arguably a bit less “pretty” to have to begin private functions with _ but that’s also I think what makes it so effective.
Base seems to maybe partially (?) use this convention (or at least, there are _ methods in base and I certainly make sure to never rely on them). Would it be infeasible to adopt this more consistently throughout Base? (maybe for 2.0…)
Then there’s absolutely no ambiguity as to what is private: does it begin with _ ? yes : no. Don’t necessarily even need tooling – though you could certainly add it easily enough, and would not need to worry about annotating things with a special @public/@private macros.
As a convention, this solves the problem from one side: as a consumer of an API I can tell at a glance (and I can check programatically reasonably easy) if an API is public or not. Assuming everybody will (afford to) adopt this convention. Because when making a function public or private with a modifier it’s an issue of simply changing it at the place of the function declaration. But marking a function as public/private in an existing codebase, requires replacing all the uses of that function throughout all the codebases.
However, the underscore convention does not cover the other side: a developer that wants to ensure that their internal code can not be utilised outside the boundaries of their own class/module/type/etc hierarchy.
A lightweight way to improve on what already exists would be:
Exported-public - anything exported is deemed public
Private - anything not exported is deemed private by default
Not-exported public - need a keyword or macro to mark entities as not exported but are part of the API.
That means the degree of change necessary to implement is restricted to methods you want to be public but not exported, which should be relatively few.
Tooling can warn if private entities are used from another module, rather than have any enforcement by the compiler. Have another keyword or macro that allows it to be suppressed, either for a particular line, block of code, or whole module.
No need to enforce a naming convention that would require ecosystem-wide refactoring of all internal code.
Python at least has more of a culture and tooling (linters, etc.) to support the idea of private methods and attributes. Just one example: Some linters will require you to document a method unless it starts with _.
The same convention is sometimes used in Julia. But, it’s much less widespread, and more poorly supported by tooling.
One thing I can fully agree on is that it sucks when someone contributes code and your library breaks a year later because they snuck in an undocumented internal Base method to make something work but didn’t make a note or add a check to the tests. It makes sense to want some guard against that beyond proofreading code at 100% efficiency.
It just doesn’t make sense to appropriate OOP access modifiers for this, Julia doesn’t have classes for access modifiers to encapsulate. Roughly speaking, OOP encapsulation designates what components of a class are accessible by other classes (public methods and few fields) and what are confined implementation details (private fields, methods, even nested classes). It’s not the same, but a non-OOP encapsulation is accomplished by modules separating global scopes, exporting select names, or importing select names. That’s why exported names came up in the thread as a hint of public API (though still not as authoritative as documentation).
The talk about access modifiers is suggesting that we should apply to multimethods in modules something designed for members in classes. There’s a lot wrong with this:
Internals aren’t confined to their home module, they’re free to be exported within the package to the convenience of the developer and out of the users’ sight.
The unit in which an internal operates is not a module, it’s the package. So you could invent access modifiers on a package level…if you’re prepared to sacrifice the ability to inspect and test internals easily. There’s also a complicated unsettled debate on how to develop and test (or not) private members.
Unlike a singly dispatched method confined to a class, a multimethod’s definition can be scattered, even across packages. What happens when some methods are marked @private and the others are marked @public? This straightforward inspiration is just a footgun.
What about global variables and macros? Macros aren’t even variables or instances.
There’s been some talk about informal conventions like leading underscores, but I’m not sure it’ll solve the issue at hand. It saves someone from checking documentation or exports to know if something is public vs internal, but a proofreader can easily overlook it anyway. Without printing warnings or throwing errors, this’ll only make a very rare mistake somewhat easier to spot. And remember that all our internals would have leading underscores, so when you search the text ._, most of it would be your internals. And if we want to make an internal public in the next version? Time to find-replace-all every file.
No need for opt in. Last I checked, it was either an issue in your repo, or a revert of the offending change in Base, provided it was an accidental API breakage, not a change to an internal function that doesn’t have stability guarantees. Other cases are when you test for exact numerical reproducibility of e.g. the default RNG number stream or numerical results based on that.
Possibly, but note that in this particular case, @essenciary himself introduced this code. To some extent, this whole discussion is not unlike complaining about missing toes after purchasing, loading, and firing a footgun, that had FOOTGUN printed on it in large block letters.
Again, I am not necessarily disagreeing — if we had a good technical solution here, it would make sense to apply it. Just want to point out that this is a relatively benign bug, because
it can be easily tested for ex ante (@assert isdefined(SomeModule, :internals_I_used)), and caught by CI,
if it is nevertheless missed, things break immediately and visibly, instead of some silent error that corrupted something and you discover a year later,
it can be tracked down super-quickly (top of the stack trace).
Unfortunately, 99%+ of bugs are nastier than this, because of one reason or another. This is why people do use internals from other modules in Julia as a workaround, in an admittedly rather informal way, and also why there is no great pressure to fix this — it is relatively innocuous as bugs go.
@Tamas_Papp as already mentioned, it’s a generic discussion about methodology and the general way of doing things. The issues I raised stand, regardless of the exact specific of this incident. I would appreciate it if you’d keep the discussion at technical topics and abstain from comments about people.
Proposed breaking changes to the language’s design aside, what does Base.modules_warned_for even do? The most I could figure out is Base.modules_warned_for == Set{Base.PkgId}(). Any chance the removed code is simple enough to be repurposed for your package’s own internal code? Seems a bit like plagiarism but maybe this is fair game for internal code? Just seems like a faster option than contributing to the language standard.
