search

FluxML / MacroTools.jl

MacroTools provides a library of tools for working with Julia code and expressions.
https://fluxml.ai/MacroTools.jl/stable/
Other
308 stars 77 forks source link

splitarg incompatible with kwargs on 1.7 #177

Closed willow-ahrens closed 2 years ago

willow-ahrens commented 2 years ago

Therefore, it is no longer safe to do funcdef[:kwargs] = map(arg->combinearg(splitarg(arg)...), funcdef[:kwargs], since this would transform a definition like f(;kwargs...) to f(;kwargs::Any...) which is no longer allowed in Julia 1.7. I don't know why that's no longer allowed, but from the REPL:

ERROR: syntax: "x::Any" is not a valid function argument name around REPL[3]:1
Stacktrace:
 [1] top-level scope
   @ REPL[3]:1
cstjean commented 2 years ago

MWE, for convenience:

julia> f(args::Any...) = 10
f (generic function with 2 methods)

julia> f(;args::Any...) = 10
ERROR: syntax: "args::Any" is not a valid function argument name around REPL[24]:1
Stacktrace:
 [1] top-level scope
   @ REPL[24]:1

Man, that sucks.

First things first, can we identify the Julia PR that changed this? I'm not sure that this isn't simply an unintended change, that could potentially be reverted in a julia patch release.

Second... There's another solution, being that IF slurp is true AND type is :Any, THEN do not output the type. It's not pretty, but I find it hard to believe that it'll break anybody's code...

cstjean commented 2 years ago

https://github.com/JuliaLang/julia/issues/43625. The more I think about it, the more I believe that this was unintended. The error message certainly doesn't sound like a carefully thought deprecation!

willow-ahrens commented 2 years ago

I'm not sure it's even possible for the symbol :Any to evaluate to anything other than Any, so I think it's always correct to omit a typassert on :Any. I've updated #178 accordingly.

cstjean commented 2 years ago

Let's wait for Julia's feedback. Julia 1.7 introduced a special case (all args can be typed except...), which forces us to introduce another special-case, which might well force other libraries downstream to special-case Any too. It's not a good situation at all.

cstjean commented 2 years ago

Update from https://github.com/JuliaLang/julia/issues/43625: Jeff said that the new behaviour wasn't exactly intentional, but that it's in a sense more correct than the old, so we'll go ahead with #178

willow-ahrens commented 2 years ago

This is fixed by #178 and a better, additional, solution is described in #179