rofinn / FilePaths.jl

A type based approach to working with filesystem paths in julia
Other
82 stars 14 forks source link

Correct module lookup in compat macro #39

Closed oxinabox closed 5 years ago

oxinabox commented 5 years ago

@__MODULE__ in the body a macro expands to that macro's module. What one actually wants is the QuoteNode(__module__) which expand to the calling code's module.

module Bar
    macro get_module_wrong()
        mod = @__MODULE__
        :(string($(esc(mod))))
    end

    macro get_module_right()
        mod =  QuoteNode(__module__)
        :(string($(mod)))
    end
end

module Foo
    using ..Bar: @get_module_right, @get_module_wrong

    println("wrong: ", @get_module_wrong())
    println("right: ", @get_module_right())

end;

output:

wrong: Main.Bar
right: Main.Foo
rofinn commented 5 years ago

So this is passing tests because FilePaths has the types we're working with, but this would error for 3rd party types? I'm not entirely sure how we get the Module from this because we need that at macro time rather than when the return expression is evaluated.

oxinabox commented 5 years ago

So this is passing tests because FilePaths has the types we're working with, but this would error for 3rd party types?

Correct

I'm not entirely sure how we get the Module from this because we need that at macro time rather than when the return expression is evaluated.

Oh bother, I forgot that QuoteNode(__module__) does infact return aQuoteNode until it is interpolated.

I think mod = QuoteNode(__module__).value works

oxinabox commented 5 years ago
julia> module Bar
           macro get_module_wrong()
               mod = @__MODULE__
               :(string($(esc(mod))))
           end

           macro get_module_right()
               mod = QuoteNode(__module__).value
               :(string($(esc((mod, typeof(mod))))))
               end
               end
WARNING: replacing module Bar.
Main.Bar

julia> module Foo
           using ..Bar: @get_module_right, @get_module_wrong

           println("wrong: ", @get_module_wrong())
           println("right: ", @get_module_right())

       end;
WARNING: replacing module Foo.
wrong: Main.Bar
right: (Main.Foo, Module)
rofinn commented 5 years ago

I'm a little nervous of using this undocumented language feature.... 😨

codecov-io commented 5 years ago

Codecov Report

Merging #39 into master will not change coverage. The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #39   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           3      3           
  Lines          60     60           
=====================================
  Hits           60     60
Impacted Files Coverage Δ
src/compat.jl 100% <100%> (ø) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 9a5eeb4...5a22bad. Read the comment docs.

rofinn commented 5 years ago

Would you mind adding a test case for a 3rd party path type?

Maybe something like this?

using FilePathsBase

struct MyPath <: AbstractPath
    x::String
end

__init__() = register(MyPath)
FilePathsBase.ispathtype(::Type{MyPath}, str) = startswith(str, "mypath://")
...
FilePaths.@compat mypath(path::MyPath) = path.x

And then add this test:

str = "mypath://foo/bar.txt"
p = MyPath(str)
@test TestPkg.mypath(p) == TestPkg.mypath(str)
oxinabox commented 5 years ago

I'm a little nervous of using this undocumented language feature.... 😨

It is documented, it is just poorly documented and kinda obsure.

https://docs.julialang.org/en/v1/manual/metaprogramming/index.html#Macro-invocation-1

oxinabox commented 5 years ago

Test added.

Without my fix, one gets the following errror

FilePaths: Error During Test at /Users/oxinabox/JuliaEnvs/FilePaths.jl/test/runtests.jl:4
  Got exception outside of a @test
  LoadError: LoadError: UndefVarError: MyPath not defined
  Stacktrace:
   [1] _ispath at /Users/oxinabox/JuliaEnvs/FilePaths.jl/src/compat.jl:144 [inlined]
   [2] (::getfield(FilePaths, Symbol("#parse_arg#2")){Module,Array{Symbol,1},Array{Symbol,1}})(::Expr) at /Users/ox
ePaths.jl/src/compat.jl:64
   [3] compat_exp(::Module, ::Expr) at /Users/oxinabox/JuliaEnvs/FilePaths.jl/src/compat.jl:97
   [4] @compat(::LineNumberNode, ::Module, ::Any) at /Users/oxinabox/JuliaEnvs/FilePaths.jl/src/compat.jl:44

With my fix, all things work as expected.

rofinn commented 5 years ago

Awesome, thanks for taking the time to fix this!