Fixes 165, Fixes 177.

[willow-ahrens]

A partial rewrite of splitarg and combinearg to fix #165 and #177.

The big change here is that arg_type can be nothing if no type is declared for an argument. I know that this is a change to the interface, but I also see no other way to fix the problem. While we are changing the interface, we may want to consider handling literal nothing as an argument default through the base Some and something interface. (discussed in #35).

[codecov-commenter]

Codecov Report

Merging #178 (f84996f) into master (a80be88) will increase coverage by 1.12%.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##           master     #178      +/-   ##
==========================================
+ Coverage   73.56%   74.68%   +1.12%     
==========================================
  Files           9        9              
  Lines         401      403       +2     
==========================================
+ Hits          295      301       +6     
+ Misses        106      102       -4     

Impacted Files	Coverage Δ
src/utils.jl	66.66% <75.00%> (+2.81%)	⬆️

---

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 a80be88...f84996f. Read the comment docs.

[cstjean]

That's a really nice PR, thanks!

The big change here is that arg_type can be nothing if no type is declared for an argument.

👍 It's an improvement.

While we are changing the interface, we may want to consider handling literal nothing as an argument default through the base Some and something interface. (discussed in #177).

Yes, that's unfortunately thorny. I don't see a way out of that change...

While we're at it, I wonder if we shouldn't return a namedtuple from both splitdef and splitarg. get(nt, :default, whatever) is somewhat nicer than tup[3] === nothing ? whatever : tup[3].

Also makes me want to reconsider #132... Now that MacroTools isn't super actively developed, maybe it'd make sense?

EDIT: Looking at ExprTools, it's still missing splitarg, so let's forget about that and focus on this PR...

[cstjean]

In any case, I'd like to think about it a bit before suggesting a particular change, but I agree that one set of breaking changes (and associated version bump, if necessary) is better than multiple ones, so we need to think well about the interface that's best...

[willow-ahrens]

Yes, let's think about it, but not for too long. A library like MacroTools is more useful when it can keep up to date with the syntax changes of Julia as they happen. If this issue takes two years to resolve, like #132 has, users are probably better off just writing their own splitarg as a utility file in their package and keeping it up to date themselves. splitarg and combinearg represent a combined total of under 30 lines of code, after all. Also, the pattern f(; kwargs...) is fairly common, so it would be great to maintain support.

If a discussion on the Julia Discourse or the Julia Slack would help everyone feel more confident about an interface change, let's start one.

I agree that the NamedTuple interface is better than the Tuple interface. However, it would not be type-stable (The type of the NamedTuple would change depending on whether or not arg_type is present), nor would it allow mutation (a common pattern is to mutate some field of the splitdef dictionary and pass it right back to combinedef).

We would get the same benefits of being able to call get if we used a Dict for splitarg, so perhaps we should change splitarg and combinearg to match splitdef and combinedef in returning and accepting Dict objects. This has the added benefit of leaving the interface to the more popular functions splitdef and combinedef unmodified. ExprTools also chose to use Dict.

Let me know if you'd like me to update this PR to return Dict instead of Tuple.

[willow-ahrens]

Since there are many users of MacroTools, and we're concerned about making a breaking change, we could also deprecate splitarg and combinearg and provide the new interface under a new set of names (splitargdef, combineargdef) to ease the transition.

[cstjean]

Yes, let's think about it, but not for too long. A library like MacroTools is more useful when it can keep up to date with the syntax changes of Julia as they happen.

True. Arrr, I don't know. You make a good point re. type stability.

Here's another proposal, which I hate for its complexity, but it kinda fills a lot of desiderata. Return a struct like:

mutable struct SplitArg
    name::Some
    type::Some
    default::Some
    splat::Bool
end

That would be type stable, and incur no particular compile-time cost. Then implement getindex(sd::SplitDef, f::Integer) to mimic the current Tuple behaviour, thus effectively making this a non-breaking change. We can overload getproperty/setproperty to support having default be set or not (sa.default would either return the default value, or throw error if there is none) and the user would use hasproperty to check if it has a default field.

In the short term, we could keep supporting combinearg(::SplitArg) and combinearg(::Tuple). Then we could deprecate it at some point if we cared.

🤮 I really don't like it.

I'm kinda worried about making a breaking release. How long does it take for all of these packages to upgrade to the latest version? I guess it's fine with CompatHelper, but...?

If a discussion on the Julia Discourse or the Julia Slack would help everyone feel more confident about an interface change, let's start one.

Not a bad idea, could you please start it? Basically, is it worth the transition pain making a point release when there is such a small interface change to a not-so-frequently used portion of MacroTools, given that it's so ubiquitous?

[cstjean]

We would get the same benefits of being able to call get if we used a Dict for splitarg, so perhaps we should change splitarg and combinearg to match splitdef and combinedef in returning and accepting Dict objects.

Yeah, I really like the simplicity of the Dict interface. A tuple of Some values is just so cumbersome. Even my code from above tup[3] === nothing ? whatever : tup[3] was missing the something, and I'm sure it'll be a frequent error.

Also, we could sneakily have both out[:default] = 25 and out[3] = 25, to provide something pretty close to backwards compatibility... 🤔 It'd be a fair bit ugly though. But if it saves us from a major version bump.....

[willow-ahrens]

I suspect that solutions which attempt to preserve the old interface (fancy getindex overloads, etc.) will still not preserve backward compatibility in many cases because of the unexpected new nothing value for arg_type instead of the old :Any.

Therefore, I advocate for the introduction of new functions named splitargdef and combineargdef that proudly use Dicts, and the deprecation of splitarg and combinearg. Anyone who notices deprecated splitarg no longer working on corner cases can upgrade to the new, unified Dict interface, and old code will still work as well as it possibly could with the old interface.

I don't think there's a way to get around the fact that users of splitarg need to rethink their code slightly because of this f(; kwargs...) debacle. The old interface didn't express enough information to handle things correctly. We can introduce a new function with new semantics for those who are ready to handle this case. No version bump necessary for deprecations, right?

[willow-ahrens]

A discourse post was made here.

[cstjean]

I finally had some time to download Julia 1.7 and check this out. I don't understand the default = nothing problem.

julia> fdef = :(foo(; x=1, y=nothing) = 2);

julia> splitdef(fdef)[:kwargs][2]
:($(Expr(:kw, :y, :nothing)))

julia> splitarg(splitdef(fdef)[:kwargs][2])
(:y, :Any, false, :nothing)

That looks fine to me. Where's the problem? Could you provide a broken test example?

[willow-ahrens]

My original post referenced the wrong issue when I intended to reference #35 (where the nothing problem was first noticed), apologies. You can get default to return nothing with a literal nothing in the input expr. This is a rare case.

splitarg(:(f(arg = $nothing) = 1).args[2])

[willow-ahrens]

More specifically, here are the two places where this is discussed in most detail:

#35 (comment)

#35 (comment)

[willow-ahrens]

An arguably less rare issue with nothing defaults occurs in combinearg. The user might want to specify nothing as a default, in the same way they might want to supply 1 or "hello". However, if they pass nothing to combinearg as a default, it would leave out the default entirely.

[willow-ahrens]

I've updated this PR to the minimal change suggested in #177, but I still think a dictionary-based interface would be nice to add under a separate name, perhaps in a separate PR.

[cstjean]

I still think a dictionary-based interface would be nice to add under a separate name, perhaps in a separate PR.

I like it too, but I agree with what Kristoffer said: we should try our best to avoid the need for a breaking release. Maybe if/when it's time for MacroTools 0.6 we can introduce it, but even then, it feels like a cosmetic change that'll break a bunch of code without providing much value at this point.

An arguably less rare issue with nothing defaults occurs in combinearg. The user might want to specify nothing as a default, in the same way they might want to supply 1 or "hello". However, if they pass nothing to combinearg as a default, it would leave out the default entirely.

Yeah... That's a gotcha, but it's not really a bug. You can just pass a QuoteExpr(x) if you want to pass a value as default, that might be nothing. Maybe it should be mentioned in the docstring...

[cstjean]

You can get default to return nothing with a literal nothing in the input expr.

Yes, and then the @assert from this PR makes a lot more sense! I thought that somehow a plain default value of nothing was broken. If it's to catch a rare case, then 💯

[willow-ahrens]

A thought, we could wrap literal nothing in a QuoteNode in splitarg. Something to consider.

[fingolfin]

if/when it's time for MacroTools 0.6 we can introduce it

If the API gets a breaking change, then isn't that by definition the time to up the version to 0.6? I.e. if this PR is merged then (as I understand the discussion) this would need to happen?

[willow-ahrens]

The PR has been updated since it was first submitted. The new plan is that if arg_type is :Any, is_splat is true, and arg_name is not nothing, combinearg will not include a typeassert (will return :(args...) rather than :(args::Any...)). We will still return :Any from splitarg when no argument type is specified. I don't like assuming what symbols will resolve to, but I don't think it's possible for :Any to resolve to anything other than Core.Any, so this is a rare case where it's safe.

[willow-ahrens]

I suspect that changing the output of combinearg to an equivalent expression with a different form is not enough of a change to justify a version bump. That said, it could break tests that compare expressions syntactically and not semantically.

[cstjean]

We will still return :Any from splitarg when no argument type is specified. I don't like assuming what symbols will resolve to, but I don't think it's possible for :Any to resolve to anything other than Core.Any, so this is a rare case where it's safe.

julia> let Any=Int
       Any
       end
Int64

🤔 It's very unlikely that someone will do something like that though, so we'll just leave it at that. But it's another argument for the Dict output.... One day...

[cstjean]

With these changes, it should be good to merge.

[willow-ahrens]

I think this one's ready to merge.

[cstjean]

Thanks!