Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider renaming @with for ScopedValues #52535

Closed
pdeffebach opened this issue Dec 14, 2023 · 22 comments · Fixed by #53004
Closed

Consider renaming @with for ScopedValues #52535

pdeffebach opened this issue Dec 14, 2023 · 22 comments · Fixed by #53004

Comments

@pdeffebach
Copy link
Contributor

PR #50958 introduces the @with macro along with dynamic scoping generally.

DataFramesMeta.jl, the data analysis package I maintain, currently exports our own @with and will have to re-name the macro before the Julia's v1.11 release.

@with is a very valuable name. It's short and clear. Considering this, having it's use-case for ScopedValues only seems unnecessarily niche to me.

Would it be possible to re-name the with and @with to something that doesn't use such a short name?

I'm not opposed to Julia Base taking over with and @with in the abstract. But this seems like such a unique use-case that the costs don't outweigh the benefits.

@jariji
Copy link
Contributor

jariji commented Dec 14, 2023

There are quite a few macro withs on GitHub.

@vchuravy
Copy link
Member

We do expect this to be widely used across the ecosystem once people can rely on it.

It has also been started to be used through the compatibility package ScopedValues.jl so changing it does have a non-zero cost attached to it.

@nalimilan
Copy link
Member

I also find with quite general, with many other possible meanings. For example, withenv could have been called with too, but it's good a more specific name was used.

@Seelengrab
Copy link
Contributor

Saying that a new package whose explicit purpose is to provide backwards compatibility for an as-of-yet unreleased feature of Julia should trump usages of existing packages feels a bit odd. Why shouldn't the uses of existing packages (who already have a userbase! One likely wider than ScopedValues.jl) trump Base instead? Them having to change their code and potentially slate a breaking release has a "non-zero cost" attached to it as well, and I'd wager that cost is larger than that of ScopedValues.jl.

@jariji
Copy link
Contributor

jariji commented Dec 14, 2023

I think the rule is that if you're using a name and you want a stability guarantee you need to import or qualify the name. It can be annoying for using P users, but packages getting to squeeze Base out of all the good names would be annoying too.

Anyway discussing the general policy of name priority should probably happen on Discourse rather than in the @with issue.

@pdeffebach
Copy link
Contributor Author

I think with is not the end of the world. However I want to emphasize

We do expect this to be widely used across the ecosystem once people can rely on it.

I really can't imagine that many users will be using the new with. It might gain reasonable adoption among developers, but it just seems to me unlikely that mass of people justifies taking over such a niche name. I hope next time when discussing naming the developers spend more time thinking about this trade-off.

@CameronBieganek
Copy link
Contributor

I think it is important that Base not choose an overly generic name for a specific operation. For example, to get the step size of a range, Base uses the name step. But step is a very generic name---Base should have used the name stepsize. The same principle probably applies to @with.

@CameronBieganek
Copy link
Contributor

CameronBieganek commented Dec 14, 2023

If package A depends on package B, then to avoid breakage when B adds new functions, package A should use the following pattern:

module A
using B: foo, bar
# ...
end

But is that even possible with Base? To my understanding, there is an implicit using Base when the REPL is started, so every exported name in Base is brought into scope. If that's the case, then the above pattern cannot protect a package from breakage when Base adds new names. Thus, Base should be cautious about adding very generic names like @with, especially if the name is more generic than the actual operation.

This general problem is a little depressing, because it means that if a package developer wants to follow semantic versioning, then, strictly speaking, they need to upper bound the version of Base julia, because any minor release of Base could break their package.

@jariji
Copy link
Contributor

jariji commented Dec 14, 2023

Yes it works fine in general:

julia> using ThreadsX: map

julia> parentmodule(map)
ThreadsX

But this issue is for @with.

@CameronBieganek
Copy link
Contributor

CameronBieganek commented Dec 14, 2023

I think you missed the point of my comment. The point is that packages cannot protect themselves from breakage when Base introduces new names. Thus Base should be more cautious about adding new names, especially overly generic ones. And @with seems overly generic for this use case.

@mbauman
Copy link
Member

mbauman commented Dec 14, 2023

I think @jariji's point is that it is theoretically possible to avoid the issue — even with new Base exports — but only if you and all your users happened to follow the proposed guidance in #42080. But of course, that's not merged.

Concretely, I count 14 registered packages that export with or @with (not counting Reexports): https://juliahub.com/ui/Search?q=with&type=symbols&u=define

@DilumAluthge
Copy link
Member

DilumAluthge commented Dec 14, 2023

The point is that packages cannot protect themselves from breakage when Base introduces new names.

I don't think this is true.

In a fresh REPL session:

AdminisatorsMBP:Downloads dilum$ julia
               _
   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.11.0-DEV.1102 (2023-12-14)
 _/ |\__'_|_|_|\__'_|  |  Commit e9b0fa11c99 (0 days old master)
|__/                   |

julia> using DataFramesMeta: @with

help?> @with
  @with(d, expr)

  @with allows DataFrame columns keys to be referenced as symbols.

In another fresh REPL session:

AdminisatorsMBP:Downloads dilum$ julia
               _
   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.11.0-DEV.1102 (2023-12-14)
 _/ |\__'_|_|_|\__'_|  |  Commit e9b0fa11c99 (0 days old master)
|__/                   |

julia> module Foo
       using DataFramesMeta: @with
       end
Main.Foo

help?> Foo.@with
  │ Warning

  │  The following bindings may be internal; they may change or be removed in
  │  future versions:

  │    •  Main.Foo.@with

  @with(d, expr)

  @with allows DataFrame columns keys to be referenced as symbols.

Pkg status:

julia> import Pkg; Pkg.status()
Status `~/.julia/environments/v1.11/Project.toml`
  [1313f7d8] DataFramesMeta v0.14.1

@CameronBieganek
Copy link
Contributor

CameronBieganek commented Dec 15, 2023

In a fresh REPL session:

julia> using DataFramesMeta: @with

That's not the package protecting itself from breakage, that's the package user protecting themselves from breakage. The current status quo is that using DataFramesMeta is allowed (even encouraged by the Julia manual). And if you have a compatible set of packages installed by the package manager, e.g. DataFramesMeta version 0.14.1 on Julia version 1.11, then the following should work, because supposedly the package manager installed a compatible set of packages:

using DataFrames, DataFramesMeta
df = DataFrame(x = 1:2);
@with(df, :x .+ 1)

Anyhow, I opened up an issue for the more general issue of package breakage from new exported names in minor releases of Base Julia: #52540.

@pdeffebach
Copy link
Contributor Author

DataFramesMeta.jl is meant to be a REPL-package for data cleaning and data exploration, so even if Julia recommends against using for packages, the @with export would still be a problem in this case.

@Seelengrab
Copy link
Contributor

Irrespective of the wider discussion around the policy in Base, I want to thank @mbauman for digging up the concrete list of affected packages. Seeing as that is quite a number of packages, I think choosing a different name for @with is prudent.

To give a concrete suggestion: How about @withval?

@CameronBieganek
Copy link
Contributor

If package A depends on package B, then to avoid breakage when B adds new functions, package A should use the following pattern:

module A
using B: foo, bar
# ...
end

But is that even possible with Base?

Disregard this. I had a misunderstanding about how name ambiguities occur. The problem arises with code like using A; using B; foo(), which could occur in a script or inside a package. (And the problem also affects code like using A; foo() due to the implicit using Base at the beginning.)

That being said, I still think that Base should be cautious about adding new names. And new names should be added at the appropriate level of genericness. In my opinion, the name with is too generic for this use case.

KristofferC added a commit that referenced this issue Mar 6, 2024
fixes #52535

---------

Co-authored-by: Kristoffer Carlsson <kcarlsson89@gmail.com>
KristofferC pushed a commit that referenced this issue Mar 6, 2024
fixes #52535

---------

Co-authored-by: Kristoffer Carlsson <kcarlsson89@gmail.com>
(cherry picked from commit 6335386)
@sgaure
Copy link

sgaure commented Mar 12, 2024

Shouldn't with, @with and ScopedValue be declared public?

@KristofferC
Copy link
Member

That makes sense, yes.

@CameronBieganek
Copy link
Contributor

CameronBieganek commented Mar 12, 2024

Shouldn't with, @with and ScopedValue be declared public?

Cross posting from the PR #53707:

I don't think this is what we are looking for. ScopedValue, @with, and with are already exported from the Base.ScopedValues module. So if you want to use them, you do this:

using Base.ScopedValues
x = ScopedValue(1)

It's the same pattern as the Base.Iterators module.

In #53628 (not merged yet) I did add a public declaration for ScopedValues.get.

@CameronBieganek
Copy link
Contributor

I believe with, @with, and ScopedValue are imported into Base because they are used by Base, not because you are expected to write Base.with, Base.@with, or Base.ScopedValue in your code.

@sgaure
Copy link

sgaure commented Mar 12, 2024

Then the docs must be updated. It says,

@CameronBieganek
Copy link
Contributor

Yes, I already updated that in PR #53628, but it hasn't been merged yet.

mkitti pushed a commit to mkitti/julia that referenced this issue Apr 13, 2024
fixes JuliaLang#52535

---------

Co-authored-by: Kristoffer Carlsson <kcarlsson89@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants