Skip to content

improved function partial application design#56518

Closed
nsajko wants to merge 1 commit into
JuliaLang:masterfrom
nsajko:partial_application
Closed

improved function partial application design#56518
nsajko wants to merge 1 commit into
JuliaLang:masterfrom
nsajko:partial_application

Conversation

@nsajko

@nsajko nsajko commented Nov 10, 2024

Copy link
Copy Markdown
Member

This replaces Fix (xref #54653) with fix. The usage is similar: use fix(i)(f, x) instead of Fix{i}(f, x).

Benefits:

  • Improved type safety: creating an invalid type such as Fix{:some_symbol} or Fix{-7} is not possible.
  • The design should be friendlier to future extensions. E.g., suppose that publicly-facing functionality for fixing a keyword (instead of positional) argument was desired, it could be achieved by adding a new method to fix taking a Symbol, instead of adding new public names.

Lots of changes are shared with PR #56425, if one of them gets merged the other will be greatly simplified.

@nsajko
nsajko force-pushed the partial_application branch from 83d2606 to ac0a69e Compare November 10, 2024 16:42
@nsajko nsajko added design Design of APIs or of the language itself feature Indicates new feature / enhancement requests labels Nov 10, 2024
@nsajko
nsajko force-pushed the partial_application branch 3 times, most recently from 3549ca3 to 0ea9693 Compare November 10, 2024 19:46
@adienes

adienes commented Nov 10, 2024

Copy link
Copy Markdown
Member

look, I'm not trying to be the bad guy on all your PRs. but I feel like you are misrepresenting the status quo

Improved type safety: creating an invalid type such as Fix{:some_symbol} or Fix{-7} is not possible.

this is already not possible with Fix. the constructor will throw an error as implemented in this block

        if !(N isa Int)
            throw(ArgumentError(LazyString("expected type parameter in `Fix` to be `Int`, but got `", N, "::", typeof(N), "`")))
        elseif N < 1
            throw(ArgumentError(LazyString("expected `N` in `Fix{N}` to be integer greater than 0, but got ", N)))
        end

@nsajko

nsajko commented Nov 10, 2024

Copy link
Copy Markdown
Member Author

I'm not misrepresenting anything, this is the (embarrassing) behavior on master:

julia> Base.Fix{-1}
Base.Fix{-1}

@adienes

adienes commented Nov 10, 2024

Copy link
Copy Markdown
Member

try constructing it?

@nsajko

nsajko commented Nov 10, 2024

Copy link
Copy Markdown
Member Author

Could you please take your off-topic chatting to Discourse or wherever?

@adienes

adienes commented Nov 10, 2024

Copy link
Copy Markdown
Member

it was a direct and objective refute to one of the primary motivations you provided in the commit message. it feels pretty on topic to me.

that being said, you clearly don't want my input, so I'll stop providing it (as, I suspect, will everyone else)

This replaces `Fix` (xref JuliaLang#54653) with `fix`. The usage is similar: use
`fix(i)(f, x)` instead of `Fix{i}(f, x)`.

Benefits:
* Improved type safety: creating an invalid type such as
  `Fix{:some_symbol}` or `Fix{-7}` is not possible.
* The design should be friendlier to future extensions. E.g., suppose
  that publicly-facing functionality for fixing a keyword (instead of
  positional) argument was desired, it could be achieved by adding a
  new method to `fix` taking a `Symbol`, instead of adding new public
  names.

Lots of changes are shared with PR JuliaLang#56425, if one of them gets merged
the other will be greatly simplified.
@nsajko
nsajko force-pushed the partial_application branch from 0ea9693 to 8b2e9ef Compare November 10, 2024 23:14
@KristofferC

Copy link
Copy Markdown
Member

Why does every PR now try to push in this "typed domain numbers" package into Base..?

As has been said regarding

Improved type safety: creating an invalid type such as Fix{:some_symbol} or Fix{-7} is not possible.

julia> Base.Fix{1}(*, "1")
(::Base.Fix1{typeof(*), String}) (generic function with 2 methods)

julia> Base.Fix{-1}(*, "1")
ERROR: ArgumentError: expected `N` in `Fix{N}` to be integer greater than 0, but got -1
Stacktrace:
 [1] (Base.Fix{-1})(f::typeof(*), x::String)
   @ Base ./operators.jl:1181
 [2] top-level scope
   @ REPL[10]:1

julia> Base.Fix{:some_symbol}(*, "1")
ERROR: ArgumentError: expected type parameter in `Fix` to be `Int`, but got `some_symbol::Symbol`
Stacktrace:
 [1] (Base.Fix{:some_symbol})(f::typeof(*), x::String)
   @ Base ./operators.jl:1179
 [2] top-level scope
   @ REPL[11]:1

so it is unclear what is being improved here.

I don't really understand what the other benefit means.

@martinholters

Copy link
Copy Markdown
Member

From what I can tell, this PR comprises three changes that are related, but could be considered independently:

  1. Introduce a function fix that creates the required type to wrap the function and fixed argument.
  2. Rename that type from Fix to PartiallyAppliedFunction.
  3. Change the N type parameter from an integer to a type-domain integer.

I agree that 1. might make future extensions to e.g. fixing keyword arguments more seamless, but could just be introduced when the time is ripe. If we expect the type to change in the future, this function might give the needed indirection to make that less breaking, but that is quite speculative.
I like 2., but it's clearly a matter of taste, and I'm not sure it's worth the trouble renaming now.
I see no benefit in 3.

@nsajko

nsajko commented Nov 21, 2024

Copy link
Copy Markdown
Member Author

The benefit of 3. is type safety, as mentioned in the OP. I'm confused by the apparently wide disregard for correctness.

@DilumAluthge

Copy link
Copy Markdown
Member

If I understand correctly, currently Julia does check for correctness (as shown above), but currently that check is done at runtime, and the idea of this PR (specifically, change 3) is to move that check from runtime to compile-time?

What are the advantages and disadvantages of doing this check at compile-time vs runtime?

@nsajko nsajko added the DO NOT MERGE Do not merge this PR! label Nov 22, 2024
@nsajko

nsajko commented Nov 22, 2024

Copy link
Copy Markdown
Member Author

The benefit is type safety for the first type parameter (fixed argument position) of the Fix/PartiallyAppliedFunction type, i.e., an invalid type application would throw. So the existence of an invalid UnionAll value/constructor, such as Fix{-7} with the code currently on the Git master branch, would be impossible. Practically this means bugs getting caught sooner.

The drawback is more code in Base.

IMO the unnecessary lack of type safety in the current version of Fix could be embarrassing if released, leading newcomers to incorrectly conclude that Julia isn't able to provide the type safety there. Given the bad reputation regarding "correctness" that Julia seems to have among users of other programming languages, I'd expect more sensitivity for such issues.

PS: I labeled with do-not-merge because this PR currently fails show-related tests. Not sure if I should bother with looking into it given the strongly negative reactions to the PR.

@nsajko

nsajko commented Nov 22, 2024

Copy link
Copy Markdown
Member Author

Also, to respond to @adienes and @KristofferC directly, clearly error checks in a constructor are irrelevant for type safety, which is why I called the discussion off-topic.

@martinholters

Copy link
Copy Markdown
Member

So the existence of an invalid UnionAll value/constructor, such as Fix{-7} with the code currently on the Git master branch, would be impossible.

Hm, but PartiallyAppliedFunction{PositiveInteger, typeof(<), Int} would be possible AFAICT and is similarly invalid. (Well, instead of specifying an invalid argument number it leaves the argument number unspecified, so it's another kind of invalidity, but still.)

We also have

julia> Array{Float64,-7}
Array{Float64, -7}

and I don't think that has caused much grief, has it? Maybe even worse, we also allow Array{-7, Float64}. Has that caused any real harm?

@KristofferC

Copy link
Copy Markdown
Member

@nsajko, what do you mean when you say "type safety" w.r.t Julia? How will a user be happier?

@nsajko nsajko closed this Jan 28, 2025
@nsajko
nsajko deleted the partial_application branch January 28, 2025 22:40
nsajko added a commit to nsajko/julia that referenced this pull request Feb 8, 2025
Disallows any type `Fix{x}` where `!(x isa Int) || (x < 0)`.

Follows up on PR JuliaLang#54653.

The goals are similar as in the closed PR JuliaLang#56518, but less ambitious.

I hope it's not too late in the release cycle to consider this change
as it seems not to be invasive or disruptive.
nsajko added a commit to nsajko/julia that referenced this pull request Feb 8, 2025
Disallows any type `Fix{x}` where `!(x isa Int) || (x < 0)`.

Follows up on PR JuliaLang#54653.

The goals are similar as in the closed PR JuliaLang#56518, but less ambitious.

I hope it's not too late in the release cycle to consider this change
as it seems not to be invasive or disruptive.
@nsajko nsajko mentioned this pull request Feb 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

design Design of APIs or of the language itself DO NOT MERGE Do not merge this PR! feature Indicates new feature / enhancement requests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants