Make broadcastable treat Char like Number

[matthias314]

This is my very first PR, so I hope I'm doing things right. It's motivated by a thread on Discourse, see here. stevengj suggested that I file this issue on GitHub.

Starting point is the observation that Char seems to be the only plain-bits type for which broadcastable calls collect (the default method, meant for collecting iterables). stevengj mentioned on Discourse that Char once was a subtype of Number, so maybe it was just forgotten to align the treatment for Char with Number when Char ceased to be a subtype. This PR proposes to make this alignment.

It turns out that it's not enough to define broadcastable(x::Char) = x (same method as for Number). One also needs getindex(x, CartesianIndex()), which is not defined for Char (maybe again an omission). If one adds this, then everything seems to work. The entire test suite runs without problems. I've also added a test for the getindex call mentioned above. Note that there is already a @testset "broadcasting of Char" in test/char.jl.

[stevengj]

In particular, note that Char is already indexable and acts like a 0-dimensional container:

julia> ndims('x')
0

julia> 'x'[] == 'x'
true

so I think we should have a (::Char)[CartesianIndex()] method anyway for consistency. And once we have that, there should be no need to collect for broadcasting.

[stevengj]

Do we already have test coverage for broadcast operations with Char? e.g. 'x' .* ['y', 'z'] or similar?

[matthias314]

Do we already have test coverage for broadcast operations with Char?

I believe that currently the only test is @test identity.('a') == 'a' in test/char.jl. It fails if one only changes broadcastable without the new method for getindex.

[stevengj]

Would be good to add another test, just to be safe.

[matthias314]

Would be good to add another test

done

[simeonschaub]

Great first PR!

[jtrakk]

For me it's unintuitive that Char is a container. Having Char behave like a container makes it even easier than it already is to accidentally work on the wrong data type (a Char rather than a sequence of Chars). I would think indexability is mostly a historical artifact from its early origins as an Arraylike/Int, and it would be better to clean up the interface by removing the containerness in 2.0 rather than leaning into it by adding more containery features. (The same applies for numbers really.)

What do you think?

[JeffBezanson]

That comes up from time to time. It really is convenient to be able to treat a scalar like a 0-d array sometimes, so removing the methods strikes me as overly pedantic.

[jtrakk]

It really is convenient to be able to treat a scalar like a 0-d array sometimes

Agreed, but having some things and not others be arraylike complicates the semantics (what is an atom vs a collection? is this like APL or not?). We can have an answer that's clear and flexible: treating scalars as atoms by default and using Ref(x) to treat them as arrays.

[matthias314]

The tester_linux32 check failed. Is there anything I am supposed to do? I don't see how the problem is related to my PR.

[simeonschaub]

Yes, that failure can safely be ignored here.
I think this is a nice consistency improvement regardless of whether or not we want to make numbers non-indexable some time in the far future. Will merge later today if there are no further objections.

[stevengj]

Shouldn't this have been AbstractChar, not Char? The methods for getindex etcetera are for AbstractChar.