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

Add public keyword #50105

Merged
merged 69 commits into from
Sep 14, 2023
Merged

Add public keyword #50105

merged 69 commits into from
Sep 14, 2023

Conversation

LilithHafner
Copy link
Member

@LilithHafner LilithHafner commented Jun 7, 2023

NOTE: This PR is not a complete solution to the "public interfaces are typically not well specified in Julia" problem. We would need to implement much than this to get to that point. Work on that problem is ongoing in Base and packages and contributions are welcome.

This PR is extended by JuliaLang/Pkg.jl#3511 which defines the semantic interpretation of scoped export and should be merged or closed simultaneously.

This adds a mechanism for "scoped export", which marks a symbol as part of the public API without loading it into the global namespace on using.

Goals

  • "API markers" that show up in docstrings
  • The ability to query if something is an "API marked" thing
  • The ability to discover "API marked" things
  • Unambiguous definition of public API accessible from the REPL
  • Little to no effort required by package authors to adopt this slightly more formal API specification

first three goals originally posted by @dalum in #49973 (comment)

Changes this PR makes to achieve those goals

  • Add public, list, of, names
  • names(Module) reports all public names (including exports)
  • Base.isinternal(::Module, ::Symbol) is an internal method which determines if a symbol is internal.
  • ?my_symbol flags the docstrings of unexported symbols as internal.
  • The public API of Base becomes "behavior described in docstrings of public symbols (exported symbols are automatically public)"

Adoption

  • Internal unexported methods require no action. Internal exported methods shouldn't exist. Public exported methods require no action. Public unexported methods need to be "scoped export"ed to adopt this feature.
  • Until folks scoped export their public but unexported symbols, they will be flagged as internal in help?> mode.
  • Compat.jl should be able to provide @public so folks can adopt this without dropping support for earlier versions of Julia.

TODO

  • The current scoped export syntax is horrible export (scoped-true), list, of, names. This should be bikeshed and then implemented using JuliaSyntax.jl because @c42f says it's landing soon and I'd rather work with that than the scheme parser. [current syntax: public list, of, names]
  • Check that this doesn't lead to bad parsing error messages [none that I could find]
  • Need to mark as public all symbols in Base that are mentioned in the documentation
  • Need to mark as public all symbols in Stdlib that are mentioned in the documentation [this can be deferred to another PR before 1.11 lands because the docstring note is milder now].
  • The warning in internal docstrings is ugly—it was the first place I could put a warning, not how I think it should look. Again: bikeshed first, then implement the specifics.
  • Only tab-complete to public methods. (@mcabbott's suggestions) moved to Only tabcomplete public methods #50717
  • Update documentation to reflect public rather than export scoped=true.
  • Move the changes in Document how public API is defined (depends on scoped export) Pkg.jl#3511 to here.

This implements #38162. See also:

@LilithHafner LilithHafner added the docs This change adds or pertains to documentation label Jun 7, 2023
@Seelengrab
Copy link
Contributor

The current scoped export syntax is horrible export (scoped-true), list, of, names. This should be bikeshed

Hard agree on that! It's the same reason I opened an issue instead of a PR directly - I don't know a lick of scheme.

@c42f
Copy link
Member

c42f commented Jun 7, 2023

Very nice prototype! I think storing a scoped flag with each binding is the right semantics and we should do it.

For syntax I think there's two obvious options. One would be "properties on exports", as roughly implemented in this PR and mentioned in #38162

export scoped=true A, B, C

This is a little bit annoying to parse because module paths in the export list parse differently from normal Julia code. But I think we could use the presence of the = to disambiguate those cases after the first symbol is consumed.

The good thing about this syntax is it generalizes nicely if we want to add other properties to exported symbols. For example, @dalum mentioned marking experimental API in #49973 (comment) and with this syntax we can have

export scoped=true experimental=true A, B, C

# or exported experimental symbols ... though one could argue this is a bad idea
export experimental=true A, B, C

The other obvious option is a contextual keyword

scoped export A, B, C

IMO this reads really nicely - it's telling that it's the title of the PR! - and it matches with constructs such as mutable struct. But isn't as natural if multiple properties are combined, and the parser can't be forward compatible if we later decide to add more flags. experimental scoped export A, B, C doesn't look great.

@dalum
Copy link
Contributor

dalum commented Jun 7, 2023

This is really lovely! Thanks for making it ❤️ An argument for the export scoped=true ... over scoped export is that it could be made to seamlessly integrate with the @export macro from InlineExports.jl (shameless plug).

@adienes
Copy link
Contributor

adienes commented Jun 8, 2023

are there any other examples of kwarg flags for syntax in the language? If not then a contextual keyword seems more consistent to me

after all, it's mutable struct not struct mutable=true

@KristofferC
Copy link
Member

KristofferC commented Jun 8, 2023

FWIW, I think this name is not very good. It is very unclear what "scoped export" means. The definition of export is to put symbols into your namespace when you do using so having something that does not do that feels weird. It's like "vegetarian meat" or "dry water". Why is "export" even mentioned here at all?

Imo, it should be called some new adjective (public as an example), and export then also makes the object public.

@mcabbott
Copy link
Contributor

mcabbott commented Jun 8, 2023

will be flagged as internal in help?> mode.

Re how else to expose this information, one point I can't find mentioned is tab completion. At present CSV.<tab> shows all sorts of internals of CSV.jl. I think that ideally it would show only those which are exported and those marked as public without exporting (what this PR calls "scoped export", don't like the term).

@LilithHafner
Copy link
Member Author

Public sounds like a good name. Would the syntax be like this?

module Foo

export BAR
public foo, Baz

"adds one"
foo(x) = x+BAR

const BAR = 1

struct Baz end

end

Making public = 4 throw the way export = 4 currently does would be breaking, but public foo at the top-level is already a syntax error so we can use that.

@KristofferC
Copy link
Member

Could also be @public if you don't want to mess with the parser.

@Seelengrab
Copy link
Contributor

public is not a good name, because that has the connotation of access modifiers, as in many other programming languages. This is not an access modifier enforced by the compiler.

@KristofferC
Copy link
Member

KristofferC commented Jun 8, 2023

public is not a good name,

This is the adjective used to describe the feature here everywhere though. Yes, there is some baggage from the term in other (OO) languages but I am not sure that is a dealbreaker. But it could be @api or @publicAPI or something, my point is mostly that "scoped export" isn't great.

@dalum
Copy link
Contributor

dalum commented Jun 8, 2023

public in this context still means roughly the same. It's mostly the difference between whether you live in a universe where there are physical laws governing trespassing or not.

(Edit, to clarify: public here means "public use", public in the OO world is "public access", but in both cases, who the "public" is, is kinda the same thing)

@Seelengrab
Copy link
Contributor

Seelengrab commented Jun 8, 2023

If we have the option to reduce confusion for users coming from other contexts by simply calling it differently, why not do that? We don't have access modifiers and we can't get them until we make a breaking change. I really don't think it's worth having to answer questions like "Does julia have private/public?" just because we mean a slightly different thing, if we can at all avoid it.

Is it really that absurd to think of people that aren't already using julia and try to accomodate them at least a little bit?

@KristofferC
Copy link
Member

Is it really that absurd to think of people that aren't already using julia and try to accomodate them at least a little bit?

Of course not, I don't think anyone has indicated that something like that is absurd. It's a balancing act. Using the word that seems to be what we use to describe the feature gives it plus points, the fact that it can be confusing for people coming from another language gives it minus points. Other suggestions might come out on top.

@LilithHafner LilithHafner added the triage This should be discussed on a triage call label Jun 8, 2023
@dalum
Copy link
Contributor

dalum commented Jun 8, 2023

Here's a list of alternatives to "public" as an adjective from a thesaurus:

  • published
  • exposed
  • observable
  • visible
  • manifest
  • recognized
  • noticeable
  • revealed
  • unhidden
  • shared
  • known
  • accessible

I kinda like published, visible and exposed.

@LilithHafner
Copy link
Member Author

  • api
  • supported
  • stable

I kinda like stable based on the interpretation of this keyword according to JuliaLang/Pkg.jl#3511

@adienes
Copy link
Contributor

adienes commented Jun 8, 2023

I like verbs over adjectives; reveal is nice

@KristofferC
Copy link
Member

The only thing I see reasonable from the lists above is api tbh.

@mcabbott
Copy link
Contributor

mcabbott commented Jun 8, 2023

One issue with a new "public" keyword is whether it's obvious that exported names are already included (which seems desirable). In the example just above it might be surprising that you do not write

export BAR
public BAR, foo, Baz

One possibly crazy idea is this: The symbols after !export aren't actually exported, but do get all the other features we're discussing overloading onto export (i.e. marking as public).

export BAR
!export foo, Baz

@c42f
Copy link
Member

c42f commented Jun 8, 2023

are there any other examples of kwarg flags for syntax in the language

I think they'd be considered just as special in the export context as normal function kwargs. That is, on the parser side, we'd parse export scoped=1 experimental=foo A, B as something like

Expr(:export,
    Expr(:(=), :scoped, 1),
    Expr(:(=), :experimental, :foo),
    :A, :B)

Or maybe something like

Expr(:export,
    Expr(:properties,
        Expr(:(=), :scoped, 1),
        Expr(:(=), :experimental, :foo)),
    :A, :B)

Lowering (or eval in jl_toplevel_eval_flex) would then check that :scoped and :experimental are recognized and set the appropriate flags.

This is what I mean when I say the parsing of this would be forward compatible: it's a one-off change to the syntax and parser to support any number of future properties as needed.

@c42f
Copy link
Member

c42f commented Jun 8, 2023

The definition of export is to put symbols into your namespace when you do using so having something that does not do that feels weird

This is the current definition of export within Julia.

However the verb "export" has a much broader meaning in English (https://en.wiktionary.org/wiki/export#Verb) and it's good to go back to the roots of the word to decide whether attaching adverbs to it make sense. In this case I think it's fine.

@KristofferC
Copy link
Member

KristofferC commented Jun 8, 2023

What is the reason to piggyback on export (and change its meaning) anyway? It seems there is no gain from it. And you would also have to come up with a new term for what export means now. Or do you want to say a "nonscoped export" all the time or something like that?

However the verb "export" has a much broader meaning in English (https://en.wiktionary.org/wiki/export#Verb)

Which of those were you particularly thinking about that makes sense to use in the context here of denoting something as public API?

@oscardssmith
Copy link
Member

triage generally likes this but thinks that it should be named public and the names probably shouldn't show them by default.

@Seelengrab
Copy link
Contributor

I'd like to add that triage was not unanimous about calling it public.

@LilithHafner
Copy link
Member Author

Returning these things from names by default might lead to warnings in ModuleInterfaceTools.jl (https://github.com/JuliaString/ModuleInterfaceTools.jl/blob/cbc5ac69fc97dbcbf47e95041343144b3ce553f7/src/ModuleInterfaceTools.jl#L392) but we have yet to find any concrete example of where this change would cause problems. I'm still optimistic that the names change might qualify as a minor change, but I'd reconsider if an example where this would break something comes up.

@LilithHafner
Copy link
Member Author

you would also have to come up with a new term for what export means now. Or do you want to say a "nonscoped export" all the time or something like that?

I think that many existing uses of export should stay as is, referring to both full exports and scoped exports. For example, I think it would be reasonable for names to continue to return all exported symbols (both full and scoped).

For referring to unscoped exports, I think the phrase "full export" is reasonable.

What is the reason to piggyback on export (and change its meaning) anyway

It makes it clear that export automatically marks things as public.

(I'm also fine with public or whatever; I care 3x that this exists and 1x what it's called)

@Pangoraw
Copy link
Contributor

I'm still optimistic that the names change might qualify as a minor change, but I'd reconsider if an example where this would break something comes up.

As a data point, in pluto, names is used to discover the symbol exported by a given module and therefore available in scope after using the module. I think this PR changes the behavior and it now has to be filter!(s -> isexported(mod, s), names(mod, all=true, imported=true)).

@LilithHafner
Copy link
Member Author

We ran PkgEval to try to detect this sort of thing, and it came up with no real failures. However, I see that PkgEval skips Pluto.jl, so we quite plausibly could have missed that.

I searched for uses of names in Pluto.jl ag "[^_a-z]names\(" and found two results:
1:

function completions_exported(cs::Vector{<:Completion})
    completed_modules = (c.parent for c in cs if c isa ModuleCompletion)
    completed_modules_exports = Dict(m => string.(names(m, all=is_pluto_workspace(m), imported=true)) for m in completed_modules)

    map(cs) do c
        if c isa ModuleCompletion
            c.mod  completed_modules_exports[c.parent]
        else
            true
        end
    end
end

Which appears to be gatekeeping autocompletion based on whether or not a symbol is in names. This seems to want the notion of "public", not "exported". IIUC, the change this PR makes is good for this snippet.

2:

...
old_names = names(old_workspace, all=true, imported=true)
...

Which this PR does not affect the behavior of at all (all=true means that publicity and export status are irrelevant).

@Pangoraw, was that a hypothetical failure, or can you point out the specific error that you are seeing in Pluto?

@Pangoraw
Copy link
Contributor

As you pointed out, these two instances should be ok (or improved!). There is actually a third one which is used to re-run cells when using a package based on its list of exports (see fonsp/Pluto.jl#1541) and I guess this will run for unavailable symbols without the isexported filtering (similar to #51331). I just wanted to point out a case where there is reliance on symbols from names being exported, as we can of course include the filtering for VERSION >= 1.11.

@LilithHafner
Copy link
Member Author

IIUC that will result in increased runtime in an edge case, but not cause errors. Unfortunate, but neither dire nor difficult to fix. IMO that is worth pointing out (thank you for sharing), but not grounds to revert the minor behavior change to names.

@LilithHafner
Copy link
Member Author

News: #51322

@LilithHafner LilithHafner removed the needs news A NEWS entry is required for this change label Sep 23, 2023
Pangoraw added a commit to fonsp/Pluto.jl that referenced this pull request Oct 7, 2023
Pangoraw added a commit to fonsp/Pluto.jl that referenced this pull request Oct 7, 2023
Pangoraw added a commit to fonsp/Pluto.jl that referenced this pull request Oct 7, 2023
* Filter on exported symbols in 1.11

See
JuliaLang/julia#50105 (comment)
for context

* Update PlutoRunner.jl
KristofferC pushed a commit that referenced this pull request Feb 8, 2024
Fixes #52472, which was caused by `names` being changed to also return
public, unexported symbols in #50105. Note that this restores previous
behavior. A case could be made to instead add the public, unexported
bindings as suggestions with the appropriate qualification.

Not entirely sure how to test this so I'd welcome any suggestions.

---------

Co-authored-by: Jameson Nash <vtjnash@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs This change adds or pertains to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.