Skip to content

Conversation

@tkf
Copy link
Member

@tkf tkf commented Feb 25, 2022

Many publicly registered packages use Core.Compiler.return_type. Given the popularity of its usage, keeping it as an internal just doesn't reflect the current reality. Unfortunately, it is used often in a way that makes their behavior depend on subtle implementation details in the compiler. Of course, there are valid ways to use Core.Compiler.return_type for optimizations while making the packages' API-level behavior robust against possible other typocalypses. However, I don't believe the usual caution "it's an internal function so be careful" is enough. Furthermore, keeping it internal makes it hard to document the correct usage and explain the subtle nuances. So, I suggest adding Core.Compiler.return_type to the public API and explaining exactly what guarantees it provides. Importantly, this can be done without sacrificing any future improvement possibilities in the compiler.

I think the main difficulty of using Core.Compiler.return_type correctly is that you need to do reasoning with "inequality" rather than "equality." 1 This of course interacts with what you exactly guarantee in your package. So, I tried to clarify these aspects. Are they enough? Are there other nuances that I miss?

I created a markdown file so that it is easy to review. For rendered version, please see: https://github.com/tkf/julia/blob/return_type/return_type.md

I posted this in the draft mode since it has to be turned into a docstring (using the technique @jw3126 used in #36834) before it is ready to merge. But other than that, I think it's ready for review.

Footnotes

  1. This is akin to the difficulty of university math compared to high school math. The rigor and flexibility brought through the reasoning with inequalities (e.g., epsilon-delta arguments) do require some changes in the thought process.

@tkf tkf added the docs This change adds or pertains to documentation label Feb 25, 2022
@tkf tkf changed the title Public API guarantee for Core.Compiler.return_type RFC: Public API guarantee for Core.Compiler.return_type Feb 25, 2022
@N5N3
Copy link
Member

N5N3 commented Feb 25, 2022

Maybe devdoc is a better place to put this .md? (we can mention it in docstring.)

@chriselrod
Copy link
Contributor

Note of course that we already have Base.promote_op, which is documented

julia/base/promotion.jl

Lines 443 to 454 in 9b8253c

"""
promote_op(f, argtypes...)
Guess what an appropriate container eltype would be for storing results of
`f(::argtypes...)`. The guess is in part based on type inference, so can change any time.
!!! warning
Due to its fragility, use of `promote_op` should be avoided. It is preferable to base
the container eltype on the type of the actual elements. Only in the absence of any
elements (for an empty result container), it may be unavoidable to call `promote_op`.
"""
promote_op(f, S::Type...) = _return_type(f, Tuple{S...})

With the documentation/warning being relevant to invalid_usecase1.

@tkf
Copy link
Member Author

tkf commented Feb 26, 2022

Maybe devdoc is a better place

If you mean that it's too long, I think we need to find a location in the manual or the API docs. Since devdoc documents a lot of internal details, I think it blurs what is public. Ideally, it should be possible to write correct and efficient Julia program without ever reading devdoc.

Note of course that we already have Base.promote_op

Thanks for bringing it up. I should've probably discussed prmote_op in the OP explicitly. But let me walk through the reason why prmote_op is not enough and why it is implied by the logic in the OP (that is, if you buy it, of course). To see this, please observe that promote_op is severely under-specified. As I discussed in the OP, comment like "Due to its fragility" is not enough to clarify the condition on which it can be used safely (i.e., the package's API guarantee is independent of the change in the compiler) because it doesn't clarify that the inferred type is always an upper bound (unless the compiler has a bug). There is some hints in Output-type computation section for when and why prmote_op can be used. However, an example is not a specification. Furthermore, it is advocating a rather questionable (but unfortunately common) usage pattern 1 that may break upon a typocalypse. In this sense, the phrase "an appropriate container eltype" in the docstring is incorrect (but, luckily, we can remove this phrase rather easily since promote_op is not included in the manual).

Also, another (albeit non-fundamental) reason why prmote_op is not ideal is that the name and the majority of the use cases are suggesting that it is primarily for element type of array. A name like return_type or upper_bound_return_type communicates more directly that this API is a general-purpose construct. If we make return_type public, I think it's better to hide it from the documentation and then clarify what new function should be used in the docstring.

Footnotes

  1. It is questionable to advocate that it's OK to use the inference API for empty array. Empty array is not special. The consumer of the value of the API may have some dispatch depending on the element type of the array which is, I think, safe to assume public by default. Of course, it is free for the API to declare that the element type is unspecified. This technical possibility is mentioned in the documentation in the PR.

@martinholters
Copy link
Member

The inference of return_type itself is also problematic. To allow the kind of code described here to be inferred to a concrete type, we pretend the result of return_type is constant, which it only is most of the time. I think we have an issue open where this is the root cause for some severe brokeness, but of course, I cannot find it right now. But note that:

julia/base/compiler/tfuncs.jl

Lines 1976 to 1979 in 685d905

# TODO: this function is a very buggy and poor model of the return_type function
# since abstract_call_gf_by_type is a very inaccurate model of _method and of typeinf_type,
# while this assumes that it is an absolutely precise and accurate and exact model of both
function return_type_tfunc(interp::AbstractInterpreter, argtypes::Vector{Any}, sv::InferenceState)

So it's still best if you can avoid using return_type.

@KristofferC
Copy link
Member

I think we have an issue open where this is the root cause for some severe brokeness

Maybe #40302 (comment)?

@tkf
Copy link
Member Author

tkf commented Mar 1, 2022

To allow the kind of code described here to be inferred to a concrete type

My suggestion in this PR is to make absolutely no guarantee at the API level about how efficient a program written using return_type is, including when return_type itself can be inferred to the tightest possible return value. So, if the examples are optimal or not with respect to the current implementation of the compiler is not the primary issue. The main point is to show users how to specify the API guarantee for their package and make it independent of the compiler implementation.

That said, this does look scary:

But note that:

julia/base/compiler/tfuncs.jl

Lines 1976 to 1979 in 685d905

# TODO: this function is a very buggy and poor model of the return_type function
# since abstract_call_gf_by_type is a very inaccurate model of _method and of typeinf_type,
# while this assumes that it is an absolutely precise and accurate and exact model of both
function return_type_tfunc(interp::AbstractInterpreter, argtypes::Vector{Any}, sv::InferenceState)

So it's still best if you can avoid using return_type.

I was aware of the comment I have no expertise for judging how severe it is. I hoped that the bug is rare enough, given that return_type is used in fundamental interfaces in Base and stdlib. But I may be naive. So, I genuinely am curious about the opinions of you and other type inference gurus if it is safer to:

  1. hide it as an internal and keep telling not to use it or
  2. explicitly document presumably theoretically correctly implementable API and document the current limitation (can produce incorrect code? or just inefficient code?)

I think going with the first option is totally reasonable if you folks are uncomfortable with the possibility of a further increase in the use of return_type. Even so, I think it may make sense to add something like this PR in the documentation as some form of discussion. But, in this case, I think devdoc (as @N5N3 suggested) is a more appropriate place.

One conservative option may be to define a public API (the name is TBD)

public_api_return_type(args...) = Core.Compiler.return_type(inferencebarrier(args)...)

for now as a "safer" version of return_type. I believe there are a lot of situations where this is already useful (e.g., GPU) and we can decouple the discussions on the specification of return_type API and the implementation of it.

@vtjnash vtjnash changed the title RFC: Public API guarantee for Core.Compiler.return_type Some extended documentation for Core.Compiler.return_type usage as promote_op Oct 30, 2023
@vtjnash vtjnash marked this pull request as ready for review October 30, 2023 16:45
@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Oct 30, 2023
@vtjnash vtjnash requested a review from vchuravy October 30, 2023 16:45
responsibility to ensure that the API guarantees provided by the package do not depend on
the exact type `R` obtained from `promote_op`.**
Additionally, the result may depend upon return overly exact types, such as `DataType`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may depend upon return?

@IanButterworth IanButterworth merged commit 2253cdf into JuliaLang:master Oct 31, 2023
@IanButterworth IanButterworth removed the merge me PR is reviewed. Merge when all tests are passing label Oct 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs This change adds or pertains to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants