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

RFC: audit Core and Base exports #25802

Merged
merged 1 commit into from
Feb 6, 2018
Merged

RFC: audit Core and Base exports #25802

merged 1 commit into from
Feb 6, 2018

Conversation

JeffBezanson
Copy link
Member

@JeffBezanson JeffBezanson commented Jan 29, 2018

I took a pass through to find Core and Base exports that should be removed. This is based on frequency of use in Base and stdlib, and my own judgment about which things people should generally not use or rely on. Up for debate though.

Un-exported from Core:

TypeName, SimpleVector, CodeInfo, MethodTable, TypeMapEntry, TypeMapLevel,
GotoNode, LabelNode, NewvarNode, SSAValue, Slot, SlotNumber, TypedSlot

I kept QuoteNode and LineNumberNode since those appear in ASTs and can be passed to macros. GlobalRef could go either way. But I believe it does not appear in surface ASTs, only expanded macros and later. I added a module Core.IR that exports the types used in the IR.

Un-exported from Base:

BufferStream, EachLine, Enumerate, RangeIndex, StackTrace, StackFrame

Opinions welcome.

@JeffBezanson JeffBezanson added breaking This change will break code deprecation This change introduces or involves a deprecation labels Jan 29, 2018
@ararslan
Copy link
Member

Having to qualify the sorting algorithms I think will be mildly annoying, but +1 for everything else.

@StefanKarpinski
Copy link
Member

I agree with @ararslan. I also don't think we're likely to delete or change the meaning of the sorting algorithms, so I don't much mind exporting them. The other potential approach would be to re-export them from SortingAlgorithms and make the official way to pass even the algorithms defined in Base to be via that package. That would potentially argue for its inclusion in the stdlib.

@iamed2
Copy link
Contributor

iamed2 commented Jan 31, 2018

You could still do using Base.Sort to get them

@JeffBezanson JeffBezanson force-pushed the jb/exportaudit branch 2 times, most recently from bb0de2d to 9da98c5 Compare January 31, 2018 23:35
@StefanKarpinski
Copy link
Member

You could still do using Base.Sort to get them

I think we're going to want to consider anything that involves reaching into Base to get some functionality to be potentially breaking. Now that almost all of the public modules that used to be part of Base are in stdlib, you know you can safely do using DelimitedFiles and as long as you lock down a version of DelimitedFiles in a Manifest.toml file for your project, you're going to get the same behavior. For sorting, I think we either need to export these from Base and consider the stable or expose them from somewhere outside of Base which can be versioned in a manifest. However, that does not necessarily need to happen right now, so we could unexport them and then sort out where they live "officially" in the future (e.g. export them from SortingAlgorithms moved to stdlib).

@iamed2
Copy link
Contributor

iamed2 commented Feb 3, 2018

I think we're going to want to consider anything that involves reaching into Base to get some functionality to be potentially breaking.

Base.Iterators is still a thing, and I don't see how that could be decoupled or the functions exported.

@StefanKarpinski
Copy link
Member

StefanKarpinski commented Feb 3, 2018

Yes, that's true. We should probably consider moving Iterators into stdlib and adding more iterators from the IterTools package as they are made sufficiently efficient and robust.

@iamed2
Copy link
Contributor

iamed2 commented Feb 5, 2018

IIRC the iterators from Iterators are actually used fairly centrally in Base. Perhaps it's a good idea to have those ones in Base but unexported and export them from an Iterators stdlib package?

@JeffBezanson JeffBezanson merged commit 251a501 into master Feb 6, 2018
@JeffBezanson JeffBezanson deleted the jb/exportaudit branch February 6, 2018 20:23
@samoconnor
Copy link
Contributor

Why BufferStream?

It seems to me that an ::IO that can be written to from one task and read from another is a fairly generally useful thing. When I proposed adding that ability to IOBuffer(), the feeling was that BufferStream already provides that and that adding a Condition to IOBuffer() was too expensive: #23391 (comment)

HTTP.jl uses BufferStream to unit-test code that is intended work with a TCPSocket or SSLContext.

@JeffBezanson
Copy link
Member Author

I agree that it seems useful in theory, but it doesn't seem to be widely used in practice. Do you have uses for it other than testing?

@EricForgy
Copy link

Could it be the lack of popularity has to do with lack of awareness / documentation? I never knew it existed until seeing this (although I don't have a use case in mind at the moment) :)

If HTTP.jl becomes the defacto for Julia web apps (like I think it will) and BufferStreams are used there, it might increase their profile. I don't have a strong feeling either way though :)

@samoconnor
Copy link
Contributor

I suspect it isn't widely used because it is almost undocumented.
Some have rolled their own: https://github.com/JuliaWeb/HTTP.jl/blob/master/src/fifobuffer.jl (although in that case, it turns out that the extra layer of buffering can be avoided completely). Part of the perceived need for doing that are the ambiguities in the IO interface #24526.

Aside from testing, the only place I'm using it is in Base.parse(HTTP.Message, str) where the message to be parsed is written to BufferStream and then processed by the usual HTTP stack as if it were a TCPSocket. I can't use IOBuffer here because the edge cases discussed in #24526.

Perhaps the solution is to have a kw arg to IOBuffer() that causes it to return a BufferStream (i.e. an IO, backed by a local buffer, that supports tasks). That would allow BufferStream to be unexported.

@iamed2
Copy link
Contributor

iamed2 commented Feb 14, 2018

Sam is right, we've been looking for a buffer stream type for a while but haven't found anything documented.

@JeffBezanson
Copy link
Member Author

Ok, I'm certainly willing to re-export it and document it. Or maybe putting it in its own stdlib package would help improve discoverability?

fredrikekre added a commit to JuliaDocs/DocStringExtensions.jl that referenced this pull request Mar 21, 2018
fredrikekre added a commit to JuliaDocs/DocStringExtensions.jl that referenced this pull request Mar 21, 2018
* contains(haystack, needle) -> occursin(needle, haystack)

* import Compat.LibGit2 to silence some warnings

* MethodTable no longer exported from Core to Base, see JuliaLang/julia/pull/25802
mortenpi pushed a commit to JuliaDocs/DocStringExtensions.jl that referenced this pull request Mar 26, 2018
* contains(haystack, needle) -> occursin(needle, haystack)

* import Compat.LibGit2 to silence some warnings

* MethodTable no longer exported from Core to Base, see JuliaLang/julia/pull/25802

(cherry picked from commit 3fb4c3d)
Keno pushed a commit that referenced this pull request Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This change will break code deprecation This change introduces or involves a deprecation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants