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

Cannot register macro with same name #43

Open
jakobjpeters opened this issue Jun 27, 2024 · 13 comments · May be fixed by #63
Open

Cannot register macro with same name #43

jakobjpeters opened this issue Jun 27, 2024 · 13 comments · May be fixed by #63

Comments

@jakobjpeters
Copy link
Contributor

jakobjpeters commented Jun 27, 2024

The current design limits the extensibility of the package, and I argue that no macro should be given special treatment. While I'm not sure if this is an issue when a user depends on a package registering the same name, the predefined behavior intended for Distributed.@everywhere, Turing.@model, and MacroTools.@capture prevents anyone else from registering their own macro under one of those names. This applies to the Base and Core macros too, but one could argue that users shouldn't be shadowing those names anyways. While admittedly an edge case, this issue could increase in severity as DispatchDoctor.jl becomes more widely used or even moved to Core #40.

Possible solutions

I am personally in favor of #1 or #2, maybe #4, but not #3.

1. Compare macro equality at run-time (EDIT: or compile-time?)

I think that this can be implemented by storing both the module and name of each macro in MACRO_BEHAVIOR and returning an expression from get_macro_behavior and _stabilize_all. The expression would contain an if-statement for the union of behaviors of a given name in MACRO_BEHAVIOR and the default behavior.

EDIT: This might be able to be done at compile-time (at least, in the same way that LLVM can optimize it away), since macros have unique types.

Pros:

Cons:

  • Small increase in compile-time
    • Many macro expressions are small and the IncompatibleMacro case does not generate a new expression anyways
  • Small increase in run-time
    • At most an if-statement with three branches

2. Treat all macros as incompatible

Pros:

Cons:

  • Doesn't support docstrings
  • Handling macros is a useful feature
  • Increased false-negatives if a user writes unstable code in the macro

3. Make macro behavior opt-in only

The MACRO_BEHAVIOR dictionary would be empty by default, requiring users to register macros as needed.

Pros:

  • Same as #2 for macros that would otherwise be registered

Cons:

  • Extra work for users
    • Determining the behavior for macros
      • This may be done incorrectly, resulting in more bugs
    • Repeating macro registration across projects
  • If users share the same MACRO_BEHAVIOR as their dependencies (and I don't know if this is the case), then this can still result in non-extensibility

4. Make macro behavior toggle-able or customizable with preferences

  • Can be combined with previous options, and would share their respective pros and cons.
@jakobjpeters
Copy link
Contributor Author

jakobjpeters commented Jun 27, 2024

I just realized #2 wouldn't support docstrings, oops!

@stable begin
""" foo() """
foo() = 1
end

@MilesCranmer
Copy link
Owner

MilesCranmer commented Jun 27, 2024

For 1, would this mean having an if statement over all macros — at every occurrence of a macro — which only gets evaluated at runtime?

@MilesCranmer
Copy link
Owner

Maybe another option is to allow a per-module register_macro!? This is done in DynamicQuantities to allow per-module unit symbol mapping

@jakobjpeters
Copy link
Contributor Author

jakobjpeters commented Jun 27, 2024

For 1, would this mean having an if statement over all macros — at every occurrence of a macro — which only gets evaluated at runtime?

Unfortunately it might, but I'm not sure and am hoping to get it to compile away. I'm still working on a prototype, which is admittedly difficult. Returning an expression like I mentioned doesn't work. My next idea is to use downward callback functions to simulate modifying the expression without actually touching it so that the branch requiring the run-time macro can do it's thing without interfering. I believe this approach is similar to how Haskell handles impure functions by pretending to do work but evaluating it later.

Maybe another option is to allow a per-module register_macro!? This is done in DynamicQuantities to allow per-module unit symbol mapping

So the preferences file would additionally store the module and the macro would pass in __module__? Seems like a good idea!

@jakobjpeters
Copy link
Contributor Author

jakobjpeters commented Jun 27, 2024

Assuming the modules can be recognized and we store the macro in MACRO_BEHAVIORS with getproperty(parent_module, name), we might be able to dispatch on the macro with get_behavior(macro) itself, which is unique per module, and then I think the compiler will just toss out the unneeded methods?

EDIT: The methods would be generated from the table, whereas the dispatch might be able to occur at compile-time since macros can be returned as values from the expressions

julia> macro f() Symbol("@doc") end;

julia> typeof(@f)
Core.var"#@doc"

@MilesCranmer
Copy link
Owner

By the way, I’m not sure Preferences.jl makes sense here, because Preferences is more for compiler flag-like stuff. But declaring incompatible macros seems like it should be a constant and thus live in the code.

@MilesCranmer
Copy link
Owner

MilesCranmer commented Jun 27, 2024

@ven-k how is @register_unit in DynamicQuantities able to only affect the local module's unit registry? I never really understood why that works. I'd like to do something similar here with a @register_macro.

@jakobjpeters
Copy link
Contributor Author

jakobjpeters commented Jun 28, 2024

Am I doing this incorrectly? Maybe it could be done by having the macro check whether the table already exists in the current module and define it if not?

julia> module A
       using DynamicQuantities
       @register_unit MyVolt 1u"V"
       end;

julia> module B
       using DynamicQuantities
       @register_unit MyVolt 2u"V"
       end;
ERROR: LoadError: Unit `MyVolt` is already defined as `1.0 m² kg s⁻³ A⁻¹`

I'm working on #1; still trying to fix tests but it might be viable.
EDIT: The DontPropagateMacro method is incorrect here, but it shows the concept.

julia> Base.remove_linenums!(@macroexpand @stable @show 1)
┌ Warning: `@stable` found no compatible functions to stabilize
│   source_info = :(#= REPL[30]:1 =#)
│   calling_module = Main
└ @ DispatchDoctor._Stabilization ~/Documents/programming/forks/DispatchDoctor.jl/src/stabilization.jl:40
quote
    var"##435"(::DispatchDoctor._Interactions.IncompatibleMacro) = begin
            begin
                Base.println("1 = ", Base.repr(begin
                            local var"#196#value" = 1
                        end))
                var"#196#value"
            end
        end
    var"##435"(::DispatchDoctor._Interactions.CompatibleMacro) = begin
            begin
                Base.println("1 = ", Base.repr(begin
                            local var"#197#value" = 1
                        end))
                var"#197#value"
            end
        end
    var"##435"(::DispatchDoctor._Interactions.DontPropagateMacro) = begin
            1
        end
    var"##435"(DispatchDoctor._Interactions.CompatibleMacro())
end

@jakobjpeters
Copy link
Contributor Author

Option #1 only works for the return value of expressions, which is a pretty big limitation. But it was fun to try it out at least

@MilesCranmer
Copy link
Owner

Simple way to get this working: see

get_macro_behavior(ex::QuoteNode) = get_macro_behavior(ex.value)
.
Maybe what we need is a 2-arg method with both the module and the macro symbol, so a user’s registered macro would only have an effect if used within that same calling module.

@MilesCranmer
Copy link
Owner

MilesCranmer commented Aug 4, 2024

I think the best API would be:

@register_macro DontPropagateMacro @doc

Because @register_macro is a macro, it can access the module name where it is used.

We want the @doc macro (as an example) to go at the end, so that Julia doesn't parse it as applying to the other argument.

Then, get_macro_behavior takes an additional argument: the module it is used. Then, the MACRO_BEHAVIORS list can be something like:

const MACRO_BEHAVIOR = (;
    table=Dict([
        Symbol("@stable") => [(Main, IncompatibleMacro)],
        Symbol("@unstable") => [(Main, IncompatibleMacro)],
        Symbol("@doc") => [(Main, DontPropagateMacro)],
        Symbol("@assume_effects") => [(Main, IncompatibleMacro)],
    ]),
    lock=Threads.SpinLock()
)

Basically, the program will look up a macro symbol, and loop through the list of behaviors, and find the most specific behavior for the current module (where Main is the most generic).

So, if you have

module A
module mymacro(arg)
    return esc(arg)
end
@register_macro DontPropagateMacro @mymacro
end

Then it will create (or push to) the symbol in the macro table:

behaviors = get!(MACRO_BEHAVIOR.table, Symbol("@mymacro"), Tuple{Module,MacroBehavior}[])
# ^ This will automatically create the array if it does not exist

push!(behaviors, (Main.A, DontPropagateMacro))

And so whenever @stable is used within Main.A, it will prefer macro symbols defined in the table with that context, followed by symbols using Main.


However, I do feel like this is a bit complicated, and it would be better to take @ven-k's approach. I still don't quite understand how it works though and if it's possible to use it here.

@jakobjpeters jakobjpeters linked a pull request Aug 5, 2024 that will close this issue
@jakobjpeters
Copy link
Contributor Author

Maybe what we need is a 2-arg method with both the module and the macro symbol, so a user’s registered macro would only have an effect if used within that same calling module.

This fixes the problems I had trying to compare macros by identity! It is a bit more limited such that an external package cannot register defaults for arbitrary modules though.

@ven-k
Copy link

ven-k commented Aug 6, 2024

Ah, my non-org notifications are sent to an inactive mail; almost missed this thread.

So in an external package like ExternalUnitRegistration.jl the imported collections like ALL_SYMBOLS, UNIT_SYMBOLS are mutated by pushing the new registrations to it (Ex: MyWb).
These new values can be accessed and worked with within the scope of ExternalUnitReg module. This allows to work with u"MyWb" syntax and work just like other DynamicQ units. However, as these are imported collections, the changes don't live outside this module's scope.

We can still use that MyWb as a variable, outside the scope of ExternalUnitReg, because a new variable is explicitly declared in the "ExternalUnitRegistration" module here. As it is of the same DEFAULT_QUANTITY_TYPE type, the unit-arithmetic just works.
(While anything that depends on looking up the UNIT_SYMBOLS etc (ex: @u_str) don't work)

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