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

@pydef improvements #473

Merged
merged 7 commits into from
Mar 23, 2018
Merged

@pydef improvements #473

merged 7 commits into from
Mar 23, 2018

Conversation

cstjean
Copy link
Collaborator

@cstjean cstjean commented Mar 17, 2018

I used MacroTools.splitdef to clean up the method definition code. This enables support for long-form function definitions (along with parametric types and return types, which might not get much use). I also now create types with a method dictionary. It will help #470.

(Sorry for the other PR; I had forgotten to branch)

@stevengj
Copy link
Member

What are all the 0.6 test failures?

@cstjean
Copy link
Collaborator Author

cstjean commented Mar 22, 2018

Fixed.

README.md Outdated
x2.set!(self, new_val) = (self[:x] = new_val / 2)
function x2.set!(self, new_val)
self[:x] = new_val / 2
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indent

@cstjean
Copy link
Collaborator Author

cstjean commented Mar 23, 2018

Fixed indentation.

@stevengj stevengj merged commit 74a2223 into JuliaPy:master Mar 23, 2018
@rdeits
Copy link

rdeits commented Mar 24, 2018

I think this is causing errors on v0.7 that are being masked by the fact that the 0.7 build was stalling out. Previously, the 0.7 build would hit JuliaLang/julia#26357 but now it errors out earlier: https://travis-ci.org/JuliaPy/PyCall.jl/jobs/357206714#L674

As I understand it, that means that the with_ioraise macro is being called with 0 arguments instead of the 1 it's expecting. It's not clear how that could be happening, but I guess macro parsing inside other macros is different in 0.7?

@cstjean
Copy link
Collaborator Author

cstjean commented Mar 24, 2018

Looks like some interaction with JuliaLang/julia#21746 ?

All macros receive an extra argument source::LineNumberNode which describes the parser location in the source file for the @ of the macro call. It can be accessed as a normal argument variable in the body of the macro. This is implemented by inserting an extra leading argument into the Expr(:macrocall, :@name, LineNumberNode(...), args...) surface syntax.

It shouldn't affect us, though. Maybe it's a bug?

This line may have to be changed, but it looks unrelated.

@rdeits
Copy link

rdeits commented Mar 24, 2018

I don't think it's the new line number argument. The error message is:

ERROR: LoadError: LoadError: LoadError: MethodError: no method matching @with_ioraise(::LineNumberNode, ::Module)
Closest candidates are:
  @with_ioraise(::LineNumberNode, ::Module, ::Any) at /home/travis/.julia/v0.7/PyCall/src/io.jl:22

which shows that the macro was defined to take the LineNumberNode, the Module, and then it's "actual" argument expr::Any, but was called with just the LineNumberNode and Module. If it were an issue of the missing LineNumberNode, then the first argument types wouldn't match.

@Keno
Copy link
Collaborator

Keno commented Apr 2, 2018

What's happening is that pydef now generates definitions like this:

    $(Expr(:escape, :(function ##fileno#1531(self; )::Any
      #= /home/keno/.julia/v0.7/MacroTools/src/utils.jl:260 =#
      begin
          @with_ioraise
      end

@Keno
Copy link
Collaborator

Keno commented Apr 2, 2018

I'll have a fix shortly.

@Keno
Copy link
Collaborator

Keno commented Apr 2, 2018

See FluxML/MacroTools.jl#68.

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.

4 participants