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

Scoped Values documentation #53471

Closed
CameronBieganek opened this issue Feb 26, 2024 · 5 comments · Fixed by #53628
Closed

Scoped Values documentation #53471

CameronBieganek opened this issue Feb 26, 2024 · 5 comments · Fixed by #53628

Comments

@CameronBieganek
Copy link
Contributor

CameronBieganek commented Feb 26, 2024

I've started reading the Scoped Values page in the manual (v1.12-dev). It seems a bit rough around the edges.

1.

Since Julia uses lexical scope the variable x is bound within the function f to the global scope and entering a let scope does not change the value f observes.

This sentence seems to refer to the code block that immediately proceeds it, but it actually refers to the next code block. So it should probably say something like

In the following example, the variable x is bound within ...

I don't want to propose an exact wording, I just want to point out that it's pretty confusing the way that it is written now.

2.

This example does not appear to be a good example of dynamic scoping:

const scoped_val = ScopedValue(1)
const scoped_val2 = ScopedValue(0)

# Enter a new dynamic scope and set value
@show scoped_val[] # 1
@show scoped_val2[] # 0
with(scoped_val => 2) do
    @show scoped_val[] # 2
    @show scoped_val2[] # 0
    with(scoped_val => 3, scoped_val2 => 5) do
        @show scoped_val[] # 3
        @show scoped_val2[] # 5
    end
    @show scoped_val[] # 2
    @show scoped_val2[] # 0
end
@show scoped_val[] # 1
@show scoped_val2[] # 0

The exact same behavior can be achieved with let blocks:

const scoped_val = 1
const scoped_val2 = 0

@show scoped_val # 1
@show scoped_val2 # 0
let scoped_val = 2
    @show scoped_val # 2
    @show scoped_val2 # 0
    let scoped_val = 3, scoped_val2 = 5
        @show scoped_val # 3
        @show scoped_val2 # 5
    end
    @show scoped_val # 2
    @show scoped_val2 # 0
end
@show scoped_val # 1
@show scoped_val2 # 0

(They print exactly the same thing to the REPL.)

3.

Since with requires a closure or a function and creates another call-frame, it can sometimes be beneficial to use the macro form.

const STATE = ScopedValue{State}()
with_state(f, state::State) = @with(STATE => state, f())

That sentence could use some elaboration, and I have no idea what the purpose of the example is (there is no explanation given).

4.

If I'm not mistaken, the @with in this example,

Base.@kwdef struct Configuration
    color::Bool = false
    verbose::Bool = false
end

const CONFIG = ScopedValue(Configuration())

@with CONFIG => Configuration(CONFIG[], color=true) begin
    @show CONFIG[].color # true
    @show CONFIG[].verbose # false
end

should actually be this:

@with CONFIG => Configuration(color=true) begin
    @show CONFIG[].color # true
    @show CONFIG[].verbose # false
end

or maybe this:

@with CONFIG => Configuration(verbose=CONFIG[].verbose, color=true) begin
    @show CONFIG[].color # true
    @show CONFIG[].verbose # false
end

5.

In the "Example" section, there is this sentence:

Other alternatives like task-local storage and global variables are not well suited for this kind of propagation; our only alternative would have been to thread a value through the entire call-chain.

Threading a value through the entire call-chain doesn't seem so bad to me. It's explicit (explicit is better than implicit) and it does not require a new language feature. Maybe you can make a better case in the manual for why ScopedValues are needed?

@vchuravy
Copy link
Member

I don't want to propose an exact wording, I just want to point out that it's pretty confusing the way that it is written now.

Nonetheless I would encourage you to open a pull-request.

This example does not appear to be a good example of dynamic scoping:

I see how that can be confusing. It would probably make sense to introduce:

f() = @show scoped_val[]
h() = @show scoped_val2[]

const scoped_val = ScopedValue(1)
const scoped_val2 = ScopedValue(0)

# Enter a new dynamic scope and set value
@show f() # 1
@show h() # 0
with(scoped_val => 2) do
    f() # 2
    h() # 0
    with(scoped_val => 3, scoped_val2 => 5) do
        f() # 3
        h() # 5
    end
    f() # 2
    h() # 0
end
f() # 1
h() # 0

But the example was not meant as a contrast against let and rather an introduction into how scoped values behave.

and I have no idea what the purpose of the example is

To simply show the usage of the macro form?

Regarding 4 you are correct, I thought @kwdef added a copy constructor.

julia> Base.@kwdef struct Configuration
           color::Bool = false
           verbose::Bool = false
       end
Configuration

julia> Configuration(Configuration(), color = true)
ERROR: MethodError: no method matching Configuration(::Configuration; color::Bool)

Threading a value through the entire call-chain doesn't seem so bad to me.

That assumes you have control of the entire call-chain (which you often don't) and doesn't handle propagation to tasks cleanly.

@CameronBieganek
Copy link
Contributor Author

Nonetheless I would encourage you to open a pull-request.

I don't feel I know enough about ScopedValues to be writing the docs for them. 😅

and I have no idea what the purpose of the example is

To simply show the usage of the macro form?

I still think that could use some more explanation. How does the macro avoid the creation of another call-frame? Does the @with macro not just lower to with? Maybe I'm dumb, but I have no idea how @with(STATE => state, f()) is useful or how it avoids a call-frame or how it is semantically different from with(f, STATE => state).

@CameronBieganek
Copy link
Contributor Author

CameronBieganek commented Feb 26, 2024

Does the @with macro not just lower to with?

Looking at the code, it looks like with is defined in terms of @with. In the original PR, with had its own try-finally block.

I'm used to thinking of @foo(...) as lowering to foo(...) as a general design principle, but I guess Base sometimes does more exotic things with macros, like inserting compiler hints into Expr objects that have no surface level syntax equivalent. (I guess inserting compiler hints is a sneaky way to avoid adding keywords like inline or inbounds to the language.)

@CameronBieganek
Copy link
Contributor Author

I might be able to work on a PR this evening.

@mnemnion
Copy link

I'd also like to draw attention to this post and the two proceeding it (this isn't about the name of ScopedValues, I promise). Specifically, I read the example with scopes and spawned threads wrong, when accompanied by the sentence about "the usual caveats for global variables apply in the context of concurrent programming".

I'd suggest spawning the threads and then introducing the scopes, and clarifying that the "usual caveats" is about the case where a ScopedValue points to mutable data.

KristofferC pushed a commit that referenced this issue Mar 12, 2024
Fixes #53471.

One thing to note, I changed the signature in the `with` docstring from
this:

```julia
with(f, (var::ScopedValue{T} => val::T)...)
```

to this:

```julia
with(f, (var::ScopedValue{T} => val)...)
```

...since the original signature in the docstring was too strict. I also
added this sentence to the docstring:

```julia
`val` will be converted to type `T`.
```

I added a couple tests that verify the conversion behavior.
KristofferC pushed a commit that referenced this issue Mar 15, 2024
Fixes #53471.

One thing to note, I changed the signature in the `with` docstring from
this:

```julia
with(f, (var::ScopedValue{T} => val::T)...)
```

to this:

```julia
with(f, (var::ScopedValue{T} => val)...)
```

...since the original signature in the docstring was too strict. I also
added this sentence to the docstring:

```julia
`val` will be converted to type `T`.
```

I added a couple tests that verify the conversion behavior.

(cherry picked from commit 7613c69)
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.

3 participants