Skip to content

Fix Pack.@asdf references #30

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

Merged
merged 3 commits into from
Jun 26, 2025
Merged

Fix Pack.@asdf references #30

merged 3 commits into from
Jun 26, 2025

Conversation

ederag
Copy link
Contributor

@ederag ederag commented Jun 22, 2025

Using the Pack.@asdf example already present in tests (but only for symbol state, not ReactiveNode).

Before

julia> ex = :(Pack.@asdf);

julia> compute_reactive_node(ex).references
Set{Symbol} with 1 element:
  Symbol("Pack.@asdf")

This sometimes broke reactivity in Pluto because the :(import Pack) cell definitions were :Pack,
which does not match the Pack.@asdf in references.

With this PR

julia> compute_reactive_node(ex).references
Set{Symbol} with 1 element:
  :Pack

The Pluto tests pass with these changes (with EE dev'ed),
but please check thoroughly, as I'm very new to this stuff.

@ederag ederag marked this pull request as draft June 22, 2025 12:23
@ederag ederag marked this pull request as ready for review June 22, 2025 17:34
@fonsp
Copy link
Member

fonsp commented Jun 22, 2025

Thanks for looking into this!

Indeed your example should have Pack inlcuded in the references, but Pack.@asdf should be there as well. This matches what we do for Pack.asdf(123) (except for the macrocalls field of course):

Before this PR:

image

Also check Path.SubPath.func(123) and compare.

@ederag
Copy link
Contributor Author

ederag commented Jun 23, 2025

OK, no objection to keeping it as well.
But just to check my current understanding,
the joined reference (e.g. Path.SubPath.func) is not used for reactivity, right ?

@fonsp
Copy link
Member

fonsp commented Jun 24, 2025

Yes, it's used to find the link between

Example.func(x::MyType) = "asdf"

and

Example.func(MyType())

image

@ederag
Copy link
Contributor Author

ederag commented Jun 24, 2025

Thanks, your explanations were very interesting.
I overlooked that definitions could be added to a module from within Pluto.

But it does not seem possible for macros.

# ╔═╡ 67c2e8fe-67d3-4757-b649-5de0d82735b5
import Pluto.PlutoRunner

# ╔═╡ cd07dcd1-bc1f-43b9-bc8c-9e9893ff89f3
PlutoRunner.bar = 1  # needs an existing name in the module

# ╔═╡ b8f2a0b2-b751-46c7-b676-431c5018e4e9
macro PlutoRunner.bar(x)
	esc(x)
end

yields

syntax: invalid macro definition

So I'm not sure f74002e is adequate now,
but it does not hurt either ?

@fonsp fonsp merged commit 6076b93 into JuliaPluto:main Jun 26, 2025
@fonsp
Copy link
Member

fonsp commented Jun 26, 2025

Thanks, this is great!

About your example – I think we would want to make this reactive:

Package.@hello 123
# adding a new method to an existing macro
Package.@hello(x::Int64) = x + 1

But I'm pretty sure we would need some other changes in Pluto to support this. But for the correctness of ExpressionExplorer this is a nice change! And it fixes your issue, which one was it again?

@ederag
Copy link
Contributor Author

ederag commented Jun 27, 2025

The issue was not reported per se, just discovered while working on fonsp/Pluto.jl#3263.

By the way, the core of that Pluto issue is clear now;
need to think again, and probably write a summary to get advises for a fix.

@ederag ederag deleted the macro_references branch June 27, 2025 12:11
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 this pull request may close these issues.

2 participants