Normalize indices in promote_shape error messages.

[tpapp]

Seeing implementation like Base.OneTo in error messages may be confusing to some users (cf discussion in #39242,
discourse).

This PR turns

julia> ones(2, 3) + ones(3, 2)
ERROR: DimensionMismatch("dimensions must match: a has dims (Base.OneTo(2), Base.OneTo(3)), b has dims (Base.OneTo(3), Base.OneTo(2)), mismatch at 1")

into

julia> ones(2, 3) + ones(3, 2)
ERROR: DimensionMismatch("dimensions must match: a has axes (1:2, 1:3), b has axes (1:3, 1:2), mismatch at 1")

Fixes #40118.

[Cvikli]

But why not using dims when the programmer needs dims.

I mean, this is still confusing for me as an error message. Any programmer that used python or C++ would expect dims to be numbers instead of ranges. Also design pattern we should follow Jacob Law (https://lawsofux.com/jakobs-law/). I mean I am a CEO of a company and any time we design the product and created a new paradigm sooner or later we faced with problem that 99% of the users are getting used to the other pattern that is used on everyone else's platform. Many advisor I meet suggested the same and some of them were leader of 1000 employee big companies.

I would suggest to use this one:
ERROR: DimensionMismatch("dimensions must match: a has dims (2, 3), b has dims (3, 2), mismatch at 1")

That is why I proposed the solution on the other issue. promote_shape dimension mismatch prompt

I don't understand why do you force the other way. Can you argue why you want this and not the other one?

[tpapp]

I would suggest to use this one:
ERROR: DimensionMismatch("dimensions must match: a has dims (2, 3), b has dims (3, 2), mismatch at 1")

I guess we could specialize for Base.OneTo, but I find the proposed solution consistent for custom indexing.

I don't understand why do you force the other way.

You misunderstand the process — this is a pull request, not yet merged. I am not forcing anything, comments are welcome.

[Cvikli]

Base.OneTo is nice thing for some general version of the Array and the printing is cleaner as you described for general Arrays which is nice for advanced usage of for example: OffsetArrays where there is a scenario as I understood when it isn't a dim it is already a range.

What I would propose is in case of concrete Array types, to use a cleaner version. For example one possible way is just extend the promote_shape:

import Base: promote_shape
function promote_shape(a::Array{A}, b::Array{B}) where {A,B}
    promote_shape(size(a), size(b))
end

Which solve the whole error prompt and you get nice and clean:
ERROR: DimensionMismatch("dimensions must match: a has dims (2, 3), b has dims (3, 2), mismatch at 1")

This uses common pattern that is understandable for everyone coming from other languages.

[jishnub]

Perhaps the error message might be altered here, as ultimately the check is for the indices of the arrays and not the sizes. Something along the lines of "axes must match: a has indices (1:2, 1:3), b has indices (1:3, 1:2), mismatch at 1" might make it clearer that ultimately it's the indices that must be identical for the operation to succeed. This also removes the possible ambiguity in the meaning of "dims".

Although consistency might be harder to obtain in general, as StaticArrays checks the sizes and ignores the indices. For example, why does this work

julia> ones(2:4,2) + SMatrix{3,2}(ones(3,2))
3×2 SArray{Tuple{3,2},Float64,2,6} with indices SOneTo(3)×SOneTo(2):
 2.0  2.0
 2.0  2.0
 2.0  2.0

while this doesn't:

julia> ones(2:4,2) + ones(3,2)
ERROR: DimensionMismatch("dimensions must match: a has dims (OffsetArrays.IdOffsetRange(2:4), OffsetArrays.IdOffsetRange(1:2)), b has dims (Base.OneTo(3), Base.OneTo(2)), mismatch at 1")

The indices are clearly different in the first case, but that doesn't seem to matter. The broader issue of indices vs. size might need some work across the ecosystem to fix.

[tpapp]

@jishnub:

why does this work
ones(2:4,2) + SMatrix{3,2}(ones(3,2))

Because StaticArrays defines a method for + which ultimately dispatches to something that skips the shape check (other than sizes).

The broader issue of indices vs. size might need some work across the ecosystem to fix.

My understanding is that promote_shape is somewhat vestigial at this point, a remnant of earlier code that is mostly superseded by broadcasting (@mbauman, is this correct?). It is used only in a few places, called with Ints by zip and axes by + and - (for arrays). It is not something the ecosystem should really rely on any more.

[tpapp]

@jishnub, thanks for the suggestion. I went with "axes" and "size", since the two contexts that call Base.promote_shape are array operations and iteration.

[Cvikli]

Does _has_axes = T <: AbstractUnitRange evaluate at compilation time? It look nicer if it is splitted by function type?

[tpapp]

@Cvikli: yes, this information should be known at compile time. But for the function which just throws an error that should be largely irrelevant. In fact, I was considering using @nospecialize, but it should be no big deal either way.

It look nicer if it is splitted by function type?

I don't understand what you mean here.

[tpapp]

Closing and opening to trigger CI.

It would be great if someone could review this. @kshyatt, since you modified this code the last time, your comments/review would be appreciated.

[mbauman]

Yeah, we don't really need to worry about performance in an error message — if anything I'd lean towards pessimizing the performance to reduce compile time.

What about further simplifying this to use the size version for OneTo axes?

(Also this isn't a minor change as it doesn't even change the error type; we're free to change printing from version to version)

[tpapp]

@mbauman: I am not sure why we would special-case Base.OneTo here. Consider axes Base.OneTo(4) and 1:4:
my understanding from #39242 is that they should print the same, as Base.OneTo is an implementation detail.

Also, consistently printing something that axes could return may be good practice.

[tpapp]

Friendly ping: I am wondering if someone could review this so it could get merged. It is a small improvement fixing an error message.

[KristofferC]

CI seems to fail but not sure it is related. Perhaps you could rebase on master.

[vtjnash]

The CI failure is related: it is noting that you are requesting it to assign a value to T in the where clause, but that the value might be undefined.

[tpapp]

Made a mess of rebasing, will cherry-pick into another PR.