Add documentation about failure cases of trig functions & return types

[Seelengrab]

This documents the failure cases of various trigonometric functions provided in Base, but is not exhaustive. In particular, there are still undocumented failure cases of the versions of these functions taking matrices, but this can be added at a later date. The failure cases where found with PropCheck.jl, checking if the property x -> f(x) isa T for the functions f in question and the types (Int8, Int16, Int32, Int64, Float16, Float32, Float64) holds (i.e., doesn't error and returns true). I can give a script reproducing the failures if requested.

I'd expect CI to be happy, unless someone somewhere is checking for the exact error message when a DomainError is thrown - some of the functions in here had a really bare bones error message, which I've also aligned with the formulation we use in other places.

I'm not 100% sure who best to review this, but since I know @oscardssmith and @stevengj have done plenty of numeric work on these functions in the past, have a ping :)

[Seelengrab]

The failures seem to be due to timeouts & some build cache failure, so should be unrelated to the doc change here.

[oscardssmith]

This looks good to me.

[ararslan]

I understand the rationale for writing it this way but IMO this is somewhat confusing to visually parse. I think something like

Suggested change

	sin(x::T) where T <: Number -> float(T)
	sin(x::Number) -> float(typeof(x))

would read a bit nicer.

[Seelengrab]

I wrote it this way for consistency with the other functions, as well as for using the same T in the description of NaN behavior. We do also explicitly document the float(T) type version.

[ararslan]

consistency with the other functions

Which other functions? AFAICT only getipaddr and getipaddrs have their method signature as shown in the docs written this way:

$ sift -e '\bwhere \w+\s*<:\s*\w+\s*->'
stdlib/Sockets/src/addrinfo.jl:246:    getipaddr(addr_type::Type{T}) where T<:IPAddr -> T
stdlib/Sockets/src/addrinfo.jl:280:    getipaddrs(addr_type::Type{T}=IPAddr; loopback::Bool=false) where T<:IPAddr -> Vector{T}

My original comment was intended to apply to all additions in this PR rather than just this particular one; apologies, I should have made that clear.

If the where is necessary then I think this minor reformatting would be easier would be easier to visually parse as it makes the logical grouping clearer:

Suggested change

	sin(x::T) where T <: Number -> float(T)
	sin(x::T) where {T<:Number} -> float(T)

[Seelengrab]

There's also givens and isascii, buit both of those use the curly braces version:

[sukera@tower julia]$ rg '\bwhere [\w\s<:{}]+->'
stdlib/Sockets/src/addrinfo.jl
246:    getipaddr(addr_type::Type{T}) where T<:IPAddr -> T
280:    getipaddrs(addr_type::Type{T}=IPAddr; loopback::Bool=false) where T<:IPAddr -> Vector{T}

stdlib/LinearAlgebra/src/givens.jl
270:    givens(f::T, g::T, i1::Integer, i2::Integer) where {T} -> (G::Givens, r::T)

base/strings/basic.jl
632:    isascii(cu::AbstractVector{CU}) where {CU <: Integer} -> Bool

I'll add the curly braces, so these are easier to visually distinguish from the intended return type.

[nalimilan]

I was going to make the same comment about the fact that the where syntax is quite heavy to read and hides the most useful information. I'd also find it better to use -> float(typeof(x)).

[Seelengrab]

Removing the T entirely will make it confusing where the T in T(NaN) came from further below in the docstring though.

[Seelengrab]

The one non-"Allowed to fail" failure was a timeout in apple. Seems unrelated, though still bad..

[Krastanov]

Is this good to merge?

[oscardssmith]

I think so

[Seelengrab]

I'd wait on feedback regarding the missing discussion above - it is a bit weird to only call it out for one method, though adding missing support for everything else in this PR too seems a bit orthogonal.

[Seelengrab]

Based on the thread above, I've removed the mention of missing from the one function I originally added it to, so that the missing behavior can be decided seperately from this doc improvement.

I've also rebased this onto the current master, so unless CI breaks things, please let me know if there is anything else that needs to be done.

[nsajko]

I think something went wrong with the rebase: the Web UI says that there are conflicts that must be resolved, and it seems most of the commits now don't really belong to this PR?

[Seelengrab]

Oh dear, that shouldn't have happened - let me fix that.

[Seelengrab]

Fixed, thank you @nsajko for pointing it out!

[Seelengrab]

This PR only touches docs, failures seem completely unrelated.

[Seelengrab]

Rebased, since there were conflicts with master. I think this is good to go as-is, but there are still some unresolved comments about whether or not T should be used in the docstrings. Once those are decided upon (@nalimilan or @ararslan, since you had comments), this should be good to merge.

[inkydragon]

Fix whitespace

[christiangnrd]

Was the 's' accidentally removed? The website works with https for me.

[Seelengrab]

Thanks for mentioning this! It did not work for me at the time of writing that commit; since that was more than half a year ago, things might have been fixed since then (an unfortunate sideeffect of PRs lingering). Note that the DOI link below still has its https protocol.

If I'm seeing it correctly, the http link for digizeitschriften.de redirects to the https version.

[Seelengrab]

Thanks for the merge!