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

Support import renaming through at-compat macro. #725

Merged
merged 1 commit into from
Oct 19, 2020
Merged

Conversation

fredrikekre
Copy link
Member

No description provided.

@fredrikekre fredrikekre force-pushed the fe/renaming branch 5 times, most recently from 1725126 to 8abf0f2 Compare October 13, 2020 11:52
@fredrikekre fredrikekre marked this pull request as ready for review October 13, 2020 11:56
@fredrikekre
Copy link
Member Author

fredrikekre commented Oct 13, 2020

@JeffBezanson do you mind reviewing this? The code is based on https://github.com/fredrikekre/ImportMacros.jl so it has been tested in the wild. The difference is that this is a string macro since using A: a as b does not parse on earlier versions.

@martinholters
Copy link
Member

The difference is that this is a string macro since using A: a as b does not parse on earlier versions.

Maybe we can still use a normal macro:

julia> VERSION
v"1.0.3"

julia> macro foo(ex...)
       end
@foo (macro with 1 method)

julia> @foo using A: a as b

julia> 

It parses as three expressions (using A: a, as, and b), so it does not work when parenthesized:

julia> @foo(using A: a as b)
ERROR: syntax: missing comma or ) in argument list

But that might be a tolerable restriction.

@fredrikekre
Copy link
Member Author

Thanks, not sure why I didn't try that, but it does not quite work if you import macros (?):

julia> macro foo(ex...)
           @show ex
       end
@foo (macro with 1 method)

julia> @foo using A: @a as @b, c as d
ex = (:(using A: @a),)
(:(using A: @a),)

@martinholters
Copy link
Member

Indeed. Seems to be some unfortunate parser quirk:

julia> :(using A: @a as @b)
:(using A: @a)

julia> using Base: @warn everything here seems to be lost

julia> :(using Base: @warn(everything, here, seems, to, be, lost))
:(using Base: @warn)

julia> 

Looks like this is parsed as a macrocall, and when it is found to be part of a using, the macroname is extracted and the arguments are ignored (silently assuming there are none). Anyway, nothing we can fix after the fact.

So looks like the options are:

  1. Keep using compat"..." for this.
  2. Switch to @compat and accept that this cannot be used for renaming macros, which may not be the most important feature.
  3. Provide @compat and compat"...", with only the latter supporting macro renaming.

My personal preference would be 2, with the option of extending to 3 in the future if the need arises. But I've never really missed import renaming in the first place, so my opinion might not be too relevant here.

@fredrikekre
Copy link
Member Author

I agree, I think this will be used mostly for modules names anyway and not on individual names.

@fredrikekre fredrikekre changed the title Support import renaming through compat"..." string macro. Support import renaming through at-compat macro. Oct 19, 2020
@fredrikekre
Copy link
Member Author

Updated.

@martinholters
Copy link
Member

There is an edge case where this behaves wrong:

julia> @compat import LinearAlgebra: BLAS.dot as bd

julia> bd
dot (generic function with 3 methods) # julia 1.6 (after #37396)
LinearAlgebra.BLAS # julia 1.5

Also, the following just errors on 1.5:

julia> @compat import LinearAlgebra: BLAS.dot as bd, BLAS.gemm as bg
ERROR: LoadError: MethodError: Cannot `convert` an object of type Expr to an object of type Symbol

Stumbled upon this because I expected the following to fail because it tries to import the two different foos into the same gensymed module and wanted to verify my suspicion:

julia> module M; module Foo; foo() = 1; end; module Bar; foo() = 2; end; end
Main.M

julia> @compat import M: Foo.foo as foo1, Bar.foo as foo2

But of course, it instead fails with the same error as the one above.

Not sure how tragic any of this is.

@fredrikekre
Copy link
Member Author

fredrikekre commented Oct 21, 2020

@compat import LinearAlgebra: BLAS.dot as bd

Ah, didnt realize this syntax was valid, I thought you had to do import LinearAlgebra.BLAS: dot as bd which is also the reason I opted for a single gensym module per invocation, rather than one per alias.

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