Deprecate utf16_is_*, utf16_get_supplementary, is_utf8_*, add @unexportedwarning macro, add tests

[ScottPJones]

Although these were undocumented, unexported functions internal to the utf16.jl code, they got used in the JSON package.
(I will also submit a PR to fix JSON)

[pao]

I believe policy is not to add deprecations for unexported/undocumented internal functions, though I may be out of date on that.

[ScottPJones]

@pao I'm just doing what I was told to do!

[pao]

Yeah, I see that now--I hit this issue before I saw the comments on the commit. Carry on.

[pao]

But let's add a cf 9071f14#commitcomment-11953239

[ScottPJones]

If somebody can merge this quickly, I'd appreciate it, so it can make JSON happy again!

[tkelman]

Best to cross-reference everything since discussions happen in multiple places - merging #11551 broke JSON.jl.

I think the @deprecate macro causes the old name to be exported, so you could instead manually put the old method name and a depwarn without exporting it, not sure it matters too much.

[ScottPJones]

I wasn't aware of that technique, I've only used @deprecate before.

[ScottPJones]

@tkelman Is this what you were thinking of?

function utf16_is_surrogate(chr::UInt16)
    depwarn("utf16_is_surrogate was undocumented and unexported, and has been removed")
    Base.is_surrogate_codeunit(chr)
end
function utf16_get_supplementary(lead::UInt16, trail::UInt16)
    depwarn("utf16_get_supplementary was undocumented and unexported, and has been removed")
    Base.get_supplementary(lead, trail)
end

I have this ready to push to this PR, if you think it is better.
I also have another PR, JuliaIO/JSON.jl#111 to fix that as well.
If somebody could merge these in, I'd appreciate it.

[IainNZ]

@tkelman why would we deprecate these if they weren't exported? If packages need to touch these then I'd rather formally export an equivalent in 0.4, and make it work on 0.3 via Compat.

[ScottPJones]

@IainNZ In the registered packages, only JSON.jl uses these, and they are used exactly once each,
however unregistered packages might be using them (if they built off of JSON.jl, for example.
Probably be safer to deprecate AND fix JSON.jl.

[tkelman]

Technically a deprecation isn't absolutely required if the breakage can be solved via Compat, or these look short enough that they're just pasted in as one-liners for the JSON use case. Let's see what else was using the deleted/renamed functions from #11551.

If we do the manual deprecation without exporting, then I would still want to keep the warning message like a conventional depwarn with a message about what to replace

julia/base/deprecated.jl

Lines 493 to 494 in e063f77

	depwarn(string("chol(a::AbstractMatrix, uplo::Symbol) is deprecated, ",
	"use chol(a::AbstractMatrix, uplo::Union{Val{:L},Val{:U}}) instead"), :chol)

[ScottPJones]

@tkelman, as far as I can tell by grepping, it's just those 2 occurrences in JSON.jl/src/Parser.jl, in all registered packages.

[ScottPJones]

I finally pushed the changes I'd made last week to use depwarn.
@tkelman, is this what you wanted, so that the definitions would not be exported by @deprecate?

[tkelman]

sure, but you didn't do

keep the warning message like a conventional depwarn with a message about what to replace

[IainNZ]

Still not seeing the point of doing this, feel like it should should just be allowed to break in whatever hypothetical user code out that there that is using this undocumented, unexported feature.

[tkelman]

Considering the important case of JSON has been fixed, I'm inclined to agree for now, did any of the PackageEvaluator results show anything that looked like it was caused by this?

[IainNZ]

I didn't see anything, but its a bit of a mess on 0.4 anyway that this is probably the least of anyones worries. Could put something NEWS.md if really paranoid?

[ScottPJones]

The problem there is, I didn't really think they should call the new functions (which are slightly different, to be able to handle UTF-16 stored in UTF-16 or in UTF-8 (i.e. CESU-8)).
I'll go ahead and put something in that gives them the new names, with a stern warning! 😀

It took me longer that I thought for, busy with kids. I see other people have commented.
I wanted to also have this deprecated, because it looked like there might be a number of unregistered forks of the JSON code, so better safe than sorry, and anyway, all this will disappear when 0.5 comes out, right?

[ScottPJones]

This now does depwarn as @tkelman wanted, and also adds tests (along with adding a new test/deprecated.jl file where people can add tests to make sure that the deprecations they added actually are working (and help us with coverage).

[ScottPJones]

This is mergeable again, and should help increase the testing coverage.

[tkelman]

no, there's a conflict, needs a rebase

[ScottPJones]

Darn, I had it all up-to-date before we went away this weekend! I feel like Job sometimes, continuously dealing with merge conflicts! It should be fine now (Jeff had added some deprecations, not sure why git couldn't handle the merge)

[ScottPJones]

Tests passed now, all ready again.

[ScottPJones]

Bump: this deprecates the usage of 6 undocumented, unexported functions in the UTF-8 and UTF-16 handling code, that have already been used in packages, that have been replaced by more generic functions. It also adds tests for all 6, and shows a way of adding coverage tests for the deprecations themselves (as I've seen occurrences of bugs getting introduced in the deprecation code itself).

[tkelman]

This is a lot of (almost) exactly repeated code. Should really use a for loop and a macro, at least for all the single-arg cases.

[ScottPJones]

OK, good point! Thanks! (I'm just not yet good at macros, but this is probably easy enough for me to learn how quickly)

[tkelman]

It might not be worth the trouble. Since JSON has been fixed, the need for this PR is questionable.

[ScottPJones]

I found other places, added the deprecations for is_utf8_start & is_utf8_continuation,
and I think it sets a good precedent for people to add unit tests for deprecations.

[ScottPJones]

Bump: This passes, all done with a macro which can be used for any future deprecations of unexported functions, and unit tests.

[ScottPJones]

Bump? I'd love to have this facility to easily deprecate functions that have been used outside of Base, which where unexported and undocumented (meant to be essentially private).

[StefanKarpinski]

It took me quite a while to figure out what this PR is even doing since it doesn't explain anywhere. It seems to be doing for non-exported names what the @deprecate macro does for exported names.

1. The implementation a wholesale copy-pasta of the definition of @deprecate which is not ok.
2. I don't want to get into maintaining deprecations of unexported things.

If someone used something in Base that wasn't exported and it gets removed, then their code will break and they can fix it.

[tknopp]

@StefanKarpinski: See 9071f14#commitcomment-11953239 for the origins of this.
I also do not see the point of deprecating unexported functions.

[quinnj]

Yeah, even though I originally whined about it, we definitely don't need to deprecate un-exported methods. As I suggested there, it's certainly a good faith effort to search existing packages and notify authors, but we definitely don't need to deprecate.

[ScottPJones]

we definitely don't need to deprecate un-exported methods.

This already bit me in a big way once, and the other deprecation I put in this PR is for another function that is used in at least one package. With this, I can remove unexported functions from base, without having to worry about causing lots of breakage.
Unless or until Julia gets some way of indicating that a function is not meant to be accessible outside the module it is defined in, this will be a constant problem (having people use functions that they stumble across in Base [or in other packages unexported functions]).
I've found many, many places just in the registered packages where unexported functions are being used, to say nothing of unregistered packages or people's unpublished source code.

Another thing is that just because something is not exported, doesn't mean that it is not meant to be used outside of the module (which is why the accessibility or private distinction is important).
I've seen a number of functions that are not exported, not because they are not meant to be used outside of the module, but to avoid name pollution.

A good counter-example is Base.compile.
If 0.4 had been frozen before we decided to rename Base.compile as Base.compilecache, then we would have needed to deprecate it.

Finally, deprecations only need to be maintained for one release cycle.
After that, they should really be removed, or changed to simply give a fatal error (with the explanation that the old function was deprecated).

About the copy-pasta of the @deprecate macro, that was simply the easiest way to implement it, after @tkelman said it should really be a macro, but I can refactor that tonight after I get home.

[tknopp]

@StefanKarpinski: Do you have objections closing this.

[StefanKarpinski]

No, fine by me. There's a pattern here.

[tknopp]

Discussion moved to #12647