more conservative promote_rule for Pair and Some

[adienes]

closes #47704 which contains some painful examples

to quote @mbauman

I don't think the magic here is that Pair is a "wrapper" or "container." The bigger consideration to me is that it doesn't support mathematics in the first place. You can't do ("a"=>0.2) + ("b"=>2.0), so why should we bother promoting ("a"=>0.2) + ("b"=>2)? Mathematics aside, are there any "combining" operations that only work because Pair has promotion here?
This promotion rule was very intentionally added very long ago — #19171 — and was explicitly targeting promotion in vectors like this.
Ideally, I think we should remove this rule.

copies also the defensive convert that exists for Dict as observed here #12187 (comment)

[JeffBezanson]

I am OK with the promote_rule change; it doesn't make much sense for Pair to be different from Tuple here.

I don't see the need for the other check though. While Dict needs this check, there's no reason Pair itself does, and changing the promote rule makes it less likely the type of a Pair will change unpredictably. I guess the argument for doing the check is that if Dict{X,Y}(A=>B) would fail then Dict{X,Y}(Pair{X,Y}(A,B)) should also fail, but it's not obvious to me that identity needs to hold. You can also insert other explicit conversions that would change the behavior.

[adienes]

ok, makes sense. I am categorically a proponent of lossless conversion and error otherwise --- but no need to bundle that with the promote_rule change (which will probably be more impactful at fixing a larger set of real-world footguns)

[JeffBezanson]

Actually the right fix here is probably to just delete the method. To really make it work like Tuple we should fall back to typejoin like Tuple does. There are also ~no other methods of promote_rule for random container-ish types like this. I just found the one for Some, and it should probably be deleted too.

[adienes]

what should replace(Dict(1=>2, 3=>4), (1=>2) => (1.0=>2.0)) return? based on promote_type(Pair{Int, Int}, Pair{Float64, Float64}) I guess Dict{Any, Any} , as is demanded by the replace docstring ? but that seems likely to cause quite a lot of regressions

similarly: for v::Vector{<:Pair} does replace(Dict(v), p) need to be the same as Dict(replace(v, p)) ?

[JeffBezanson]

Very good catch. The Dict constructor doesn't promote, but replace does, which is annoying but we probably have to keep it since it's documented. And even as it is now, there is this discrepancy:

julia> Dict(replace(Any[1=>2, 2=>3], (1=>2) => (1=>2.0)))
Dict{Int64, Real} with 2 entries:
  2 => 3
  1 => 2.0

julia> replace(Dict(Any[1=>2, 2=>3]), (1=>2) => (1=>2.0))
Dict{Int64, Float64} with 2 entries:
  2 => 3.0
  1 => 2.0

If we remove this promote_rule method like I want to, that second case actually becomes an error no method matching similar(::Dict{Int64, Int64}, ::Type{Pair{Int64}}) because the joined pair type is too wide. So, we should consider adding an AbstractDict method for replace that calls promote on the key and value types instead of on the Pair type.

[JeffBezanson]

Data point: merge uses promote_type on the key and value types separately. The Dict constructor uses promote_typejoin on the key and value types. It looks unlikely that many places depend on Pair promotion recursively promoting the elements. DataStructures.jl and OrderedCollections.jl don't seem to use it, but maybe some packages do.

[adienes]

the current state of the PR would leave all replace behavior unchanged while deleting the rule

[adienes]

related-ish #16419

[LilithHafner]

Triage wants this but we still need a package eval.

[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

3 packages crashed only on the current version.

• An internal error was encountered: 1 packages
• GC corruption was detected: 1 packages
• A segmentation fault happened: 1 packages

1 packages crashed on the previous version too.

✖ Packages that failed

16 packages failed only on the current version.

• Package fails to precompile: 1 packages
• Package has test failures: 1 packages
• Package tests unexpectedly errored: 7 packages
• Tests became inactive: 1 packages
• Test duration exceeded the time limit: 6 packages

1531 packages failed on the previous version too.

✔ Packages that passed tests

13 packages passed tests only on the current version.

• Other: 13 packages

5209 packages passed tests on the previous version too.

~ Packages that at least loaded

4 packages successfully loaded only on the current version.

• Other: 4 packages

3023 packages successfully loaded on the previous version too.

➖ Packages that were skipped altogether

904 packages were skipped on the previous version too.

[adienes]

I created issues for the flagged failures. the remaining Meander.jl, PkgUtility.jl, EnergyExpressions.jl, seem to have not been updated in 3+ years, and TimeDag.jl is explicitly archived, so I let them be for now.