avoid storing types in fields in Fix, ComposedFunction, Splat, Broadcasted

[nsajko]

This is:

• A minor change to the functionality of Fix, ComposedFunction, Splat, Broadcast.Broadcasted that ensures these types are zero-cost abstractions (which are abstractions that should not cause any run time overhead) even when their argument values are types.

  • The interpretation of the fields and the type parameters of Fix, ComposedFunction, Splat, Broadcast.Broadcasted is changed in some special cases. The intention is to prevent having a field type which is some subtype of Type.

  • The field x of Base.Fix is not affected by this change, only the field f. This is to prevent potential breakage.

• A feature addition, introducing the new TypeWrapper. While I feel awkward proposing a feature addition along with another kind of improvement, I feel the feature addition is necessary for completeness of the above redesign. This is because TypeWrapper becomes necessary to describe some subtypes of Fix, ComposedFunction, Splat, Broadcast.Broadcasted with this change.

xref #59619

[nsajko]

@nanosoldier runtests()

[nsajko]

@nanosoldier runtests()

[PatrickHaecker]

It's really inspiring and educative to follow your thought process and your work here. Thanks a lot for this!

What about Pair? Isn't it also affected?

[nsajko]

What about Pair? Isn't it also affected?

Pair is different than Fix, ComposedFunction and the other types touched in this PR. Compare the type parameters between Pair and ComposedFunction in this example, on current master:

julia> typeof(Float32 => Int)
Pair{DataType, DataType}

julia> typeof(Float32 ∘ Int)
ComposedFunction{Type{Float32}, Type{Int64}}

Perhaps modifying Pair should be considered in a follow up PR 🤷‍♂️

[vtjnash]

Adding maybe_unwrap appears to be a layering violation in the code, since it means the result is no longer a safe pass-through. Same goes for optimizing any of these functions using this shim (especially Fix and Returns), since they will cease to function exactly per their docstring, and will instead inject a different value that is type-equal, but which is not egal to the value the user expected.

[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 on the previous version too.

✖ Packages that failed

22 packages failed only on the current version.

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

1308 packages failed on the previous version too.

✔ Packages that passed tests

17 packages passed tests only on the current version.

• Other: 17 packages

5262 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

3459 packages successfully loaded on the previous version too.

➖ Packages that were skipped altogether

900 packages were skipped on the previous version too.

[nsajko]

inject a different value that is type-equal, but which is not egal to the value the user expected

True, haven't considered that. I notice the Returns doc string explicitly mentions ===, so clearly Returns is off-limits. However perhaps the PR might still pass as a minor change, now that Returns is no longer touched?

Your concern is basically that someone might have written a method something like this, where A == B, then passed A or B to Fix, ComposedFunction, Splat or Broadcasted:

function (a::Type{A})(args...; kwargs...)
    if a === B
        g(args...; kwargs...)
    else
        h(args...; kwargs...)
    end
end

This kind of usage would be broken by this PR because we would always have a === A (or always a === B, or a === C for some other type-equal type C). However, I think such a method is quite unlikely, so this PR could now pass as a minor change? Why would someone intentionally make two type-equal callable types behave significantly differently in their user code? And I do not see breakage like that in the completed PkgEval run.

Regarding the PkgEval run, I notice these real failures:

• The Compat test suite fails because a single test was testing not only what it was intended to test. PR to adjust the test: test: Fix: adjust "structs not in Base" test Compat.jl#842.

• The FunctionChains test suite fails because of a single test, which was arguably overly strict. Adjusting the package if this PR was to be merged, if necessary at all, should be easy: just add some methods for TypeWrapper to some package-internal functions when Base provides TypeWrapper. EDIT: it seems the current versions of the PR do not break the FunctionChains test suite any more.

• The EnforcedTypeSignatureCallables test suite fails, but it's my package (actually kind of the motivation for this PR) so I'll fix it if the PR gets merged. Fixing it would amount to adjusting the definition of a type alias depending on whether Base provides TypeWrapper. EDIT: the current versions of the PR should not break the EnforcedTypeSignatureCallables test suite any more.

• The PartialFuns test suite fails because the package used the _stable_typeof function, even though it is not public API. I've submitted a PR to fix the issue in PartialFuns, even though this PR does not currently affect _stable_typeof: bug fix: copy the impl instead of using a Base impl detail uniment/PartialFuns.jl#4.

[vtjnash]

I think ComposedFunction is okay to change, but not sure about Fix, since it means we could not use Fix1(===, T) anymore. This is commonly observed as a problem for apply_type mainly, since in the example of (a::Type{A})() where {A}, a{x} !== A{x} for types such as Pair or Dict

[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.

• GC corruption was detected: 1 packages

6 packages crashed on the previous version too.

✖ Packages that failed

32 packages failed only on the current version.

• Illegal method overwrites during precompilation: 1 packages
• Package has test failures: 5 packages
• Package tests unexpectedly errored: 4 packages
• Test duration exceeded the time limit: 22 packages

1300 packages failed on the previous version too.

✔ Packages that passed tests

6 packages passed tests only on the current version.

• Other: 6 packages

5275 packages passed tests on the previous version too.

~ Packages that at least loaded

6 packages successfully loaded only on the current version.

• Other: 6 packages

3460 packages successfully loaded on the previous version too.

➖ Packages that were skipped altogether

899 packages were skipped on the previous version too.

[vtjnash]

I think you need to find a way to eliminate the _maybe_unwrap_type overloads all over the place. I think you can potentially do it (by wrapping the function with the unwrap at the same time as you wrap the argument), but the overloads of getproperty also seem like they need to go away too.