fix AbstractArrayStyle dimension promotion rule

[marius311]

The simple test I added,

struct CustomStyle{T} <: Broadcast.AbstractArrayStyle{1} end
Broadcast.BroadcastStyle(::CustomStyle{:A}, ::CustomStyle{:B}) = CustomStyle{:A}()
@test Broadcast.result_style(CustomStyle{:A}(), CustomStyle{:B}()) ==
      Broadcast.result_style(CustomStyle{:B}(), CustomStyle{:A}()) == CustomStyle{:A}() 

fails on current master because the function which I deleted in this PR is doing a bit of type piracy, especially since its using Base.typename which means its assuming if the typenames of your custom style are the same, they differ only in dimensions, for which I don't think there's justification. This then breaks BroadcastStyle rule resolution for your custom style if this is not the case.

In terms of Base, the offending part of the deleted function is not covered in the tests, and once you delete that, its equivalent to just deleting the whole thing.

I checked by hand if this impacts StaticArrays.jl as a representative user of this API, and it only breaks one test,

v1 = @SVector [2,4,6,8]
M = @SMatrix [1 2; 3 4; 5 6; 7 8]
broadcast(+, v1, M)

which now returns a SizedArray as opposed to previously an SArray. I think that can be easily fixed in StaticArrays, and it probably shouldn't have been relying on this hacky typename thing anyway. If you guys give the go ahead here, I can do the StaticArrays fix too (as well as any other comments you may have).

[marius311]

Hmm reading over the broadcasting API again maybe you can't just delete this function, its what provides the API described in the last paragraph there that lets you just define a Val(N) constructor.

Gonna mark this as draft and think about if there's a way to just do something smarter than typename which allows the above to test to pass while preserving the existing API. Suggestions welcome ofcourse.

[marius311]

I think I've got the right solution now, which fixes the test from my OP while simultaneously not breaking any Base or StaticArrays tests (any other packages I should check?), plus I think the logic makes a decent bit of sense.

Basically, instead of using Base.typename(A) === Base.typename(B) to try and guess whether to do the dimension promotion, only do it if promoting both A and B give the same result, otherwise require the user to provide a rule.

From what I can tell inference is fine and the entire logic is optimized away at compile time, so I don't think there's runtime performance concerns.

Ready for review from my end.

[mbauman]

Ah, this looks great! It's a good solution and huge bonus points for removing the certainly-erroneous @pure. It'd be good to just see what PkgEval thinks...

@nanosoldier runtests(ALL, vs = ":master")

[nanosoldier]

Your package evaluation job has completed - possible new issues were detected. A full report can be found here. cc @maleadt

[marius311]

Let me know if there's anything else that needs doing (I'm not really familiar with how to interpret the output of PkgEval).

[marius311]

Another friendly ping?

[marius311]

Hi guys, just rebased this on master. Can this please be merged? @tkf @mbauman

Not sure how many more times I have it in me to keep asking about this before I just give up and this gets lost.

[adienes]

I think this PR is actually still basically up to date. the conflict is cosmetic

[marius311]

Just did my bi-decadal rebase of this PR onto master, lmk if anything else is needed 😅

(but seriously, I appreciate this is a hard one since it touches sooo much, happy to help in whatever way I can)

I also checked StaticArrays, StructArrays, and ForwardDiff as some representative broadcast overloaders and didn't spot any new failures related to this PR (altho hard bc of other failures tbh)

[adienes]

@nanosoldier runtests()

[nanosoldier]

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

Report summary

❗ Packages that crashed

1 packages crashed only on the current version.

• A segmentation fault happened: 1 packages

2 packages crashed on the previous version too.

✖ Packages that failed

25 packages failed only on the current version.

• Package has test failures: 3 packages
• Package tests unexpectedly errored: 8 packages
• Test duration exceeded the time limit: 14 packages

1387 packages failed on the previous version too.

✔ Packages that passed tests

17 packages passed tests only on the current version.

• Other: 17 packages

5236 packages passed tests on the previous version too.

~ Packages that at least loaded

11 packages successfully loaded only on the current version.

• Other: 11 packages

2956 packages successfully loaded on the previous version too.

➖ Packages that were skipped altogether

2 packages were skipped only on the current version.

• Package could not be installed: 2 packages

920 packages were skipped on the previous version too.

[adienes]

pretty clean, I think the StrideArrays.jl issue is real, but I bet all the other failures are just downstream of that

MethodError: no method matching StrideArrays.CartesianStyle{Tuple{Int64}, 1}(::Val{1})

[marius311]

I'd bet they're somehow relying on the old broken API. I can take a look.

[marius311]

Ok, submitted a PR to fix that package, the problem was indeed they we're using the API incorrectly but the bug fixed here was hiding that.

[adienes]

awesome!

I think my initial summary was wrong, and it looks like these are also some sources, so they're not all only from StridedArrays unfortunately. I've created the relevant issues for:

Tensorial.jl#250
ClimaCore.jl#2329
Modulo2.jl#8
StandardPacked.jl#2
RecursiveArrayTools.jl#449

now these are of course not all necessarily your responsibility to fix, it would probably be polite to notify the authors before "breaking" code, so it would be reasonable give it a few days to see if any of the package authors bite on the issue. the original waiting for author label I just meant to signify that you were checking if that failure was a downstream user error or if the updated PkgEval had exposed something about this PR

if you are feeling generous, it might also be nice to add also a bit more docs about how exactly the BroadcastStyle API is supposed to be used 😬 as the fact that so many authors (incl. very experienced ones) got it wrong independently points to an insufficiency in the docs

[matthias314]

I'm the author of one of the packages mentioned above (Modulo2.jl). What about updating the Julia documentation as part of this PR? I believe the current documentation doesn't mention that style constructors have to accept argument, see the example in the docstring for BroadcastStyle. And if they have to accept a Val argument, what are they supposed to do? The patch for StridedArrays.jl assumes that the Val argument is equal to the dimension of the style and skips it,

https://github.com/JuliaSIMD/StrideArrays.jl/blob/a2dd2e6c3a938ab79b2b1c9b495c6ee640aacc5f/src/broadcast.jl#L10-L11

In contrast, the code in Base allows to set and change the dimension,

julia/base/broadcast.jl

Lines 98 to 99 in 18d5ccd

	DefaultArrayStyle(::Val{N}) where N = DefaultArrayStyle{N}()
	DefaultArrayStyle{M}(::Val{N}) where {N,M} = DefaultArrayStyle{N}()

A clarification would be most helpful.

[marius311]

The Val argument stuff is described in the AbstractArrayStyle docstring, those are the only kind of styles that assume this functionally is present, with some further clarification here: https://docs.julialang.org/en/v1/manual/interfaces/#writing-binary-broadcasting-rules

I haven't looked at your package yet so not sure if that answers everything.

In terms of the StrideArrays PR, I only did the trivial rule where the dimensionally is the same since that was all that was needed to fix the tests and I wasn't familiar enough with the package so just decided to be safe.

[marius311]

I suppose this PR does add a requirement that the trivial rule is defined where previously there was no requirement. I wonder if all those packages can simply be fixed by that? I can look. It may make sense to just define that one generically.

[matthias314]

@marius311 Thanks for the pointer to the documentation! I'm going to fix my package.

[adienes]

given

julia> struct MyStyle{N} <: Broadcast.AbstractArrayStyle{N} end

I think this PR now requires a constructor on the parameterized type

MyStyle{N}(::Val{N}) where {N} = MyStyle{N}()

where some packages had been previously defining (note the lack of {N})

MyStyle(::Val{N}) where {N} = MyStyle{N}()

This is at least what looks to be the case for RecursiveArrayTools.jl. is that an intentional change?

[marius311]

Neither master nor this PR will ever call:

MyStyle(::Val{N}) where {N} = MyStyle{N}()

They would only call this one:

MyStyle{N}(::Val{N}) where {N} = MyStyle{N}()

If package authors had implemented the first one, I think it was just a misreading of the docs or maybe they were using it themselves explicilty elsewhere.

This PR does require the second form to be defined in more cases than before, but at the benefit of making the logic correct.

[matthias314]

I think it was just a misreading of the docs

The docstring for Broadcast.AbstractArrayStyle explicitly suggests

(::Type{<:MyArrayStyleDim})(::Val{N}) where N = MyArrayStyleDim{N}()

which includes the MyArrayStyleDim(::Val{N}) constructor. If that is not needed, then maybe one can adjust the docstring.

[marius311]

Yea I would support changing that docstring to:

MyArrayStyleDim{M}(::Val{N}) where {N,M} = MyArrayStyleDim{N}()

I think it makes the docs clearer and also mirrors how DefaultArrayStyle itself is defined:

julia/base/broadcast.jl

Line 99 in f03e9c3

DefaultArrayStyle{M}(::Val{N}) where {N,M} = DefaultArrayStyle{N}()

Although confusingly DefaultArrayStyle also expclitily defines the non-concrete version just above that line, but that definition I don't see how is ever called, at the very least its confirmed not covered by Julia's test, as expected.

[adienes]

so how to progress? should we try removing DefaultArrayStyle(::Val{N}) where N = DefaultArrayStyle{N}() (and updating that docstring) and rerun pkgeval?