make creation of inputs to propagation consistently hyginic

[oxinabox]

Turns out that we were not escaping the inputs correctly in the pullbacks generated by @scalar rule.
And in the case of multi-input functions that actually ended up creating and assigining to a global variable during the input destructuring.
Which must be some edge case of the compiler, as normally it errors if you try to do that.

julia> module FooMod
       bar((FooMod.x, FooMod.y)) = 1
       @eval FooMod  bar((1,2))
       end
ERROR: cannot assign variables in other modules
Stacktrace:
 [1] setproperty! at ./Base.jl:27 [inlined]
 [2] bar(::Tuple{Int64,Int64}) at ./REPL[11]:2
 [3] top-level scope at none:1
 [4] eval(::Module, ::Any) at ./boot.jl:331
 [5] top-level scope at REPL[11]:3

Anyway that in turn was making the sincos 's rule in ChainRules.jl type-unstable because it was accessing this global variable.
Testing this is really hard. I could make a regression test for exactly that symptom if you think that is a good idea.z

Before

julia> Base.remove_linenums!(@macroexpand @scalar_rule sincos(x) @setup((sinx, cosx) = Ω) cosx -sinx )
...
function sincos_pullback((ChainRulesCore.Δ1, ChainRulesCore.Δ2))
    $(Expr(:meta, :inline))
    return (ChainRulesCore.NO_FIELDS, ChainRulesCore.muladd.(ChainRulesCore.conj(-sinx), ChainRulesCore.Δ2, ChainRulesCore.:*.(ChainRulesCore.conj(cosx), ChainRulesCore.Δ1)))
end
...

After

julia> Base.remove_linenums!(@macroexpand @scalar_rule sincos(x) @setup((sinx, cosx) = Ω) cosx -sinx )
...
function sincos_pullback((var"##Δ1#264", var"##Δ2#265"))
    $(Expr(:meta, :inline))
    return (ChainRulesCore.NO_FIELDS, ChainRulesCore.muladd.(ChainRulesCore.conj(-sinx), var"##Δ2#265", ChainRulesCore.:*.(ChainRulesCore.conj(cosx), var"##Δ1#264")))
end
...

[nickrobinson251]

yikes. Not sure how to test this / if it's worth it.

I could make a regression test for exactly that symptom if you think that is a good idea

How easy is that to do? I think it might be worth it if the test code is not too complex or hard to maintain?

Also, is it maybe worth adding a comment linking to this PR, to make it prominent if/when someone comes to edit this code? it's quite a sneaky gotcha.

[mzgubic]

Why do we need the gensym?

[oxinabox]

gensym gives it a name that is certain not to clash with anything.
it is a name like ##Δ2#991

So just incase the person had named a variable Δ2 and written something like

@scalar_rule foobar(x) (Δ2=10.0; Δ2*2),  x

using gensym makes sure we don't clash into that variable.

[mzgubic]

ah yes thanks esc doesn't help us with that

[oxinabox]

The regression test was not that hard to add.
I have verified that that fails on current master

[codecov-io]

Codecov Report

Merging #276 (f2b0b7f) into master (937981c) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #276      +/-   ##
==========================================
+ Coverage   87.79%   87.82%   +0.02%     
==========================================
  Files          13       13              
  Lines         418      419       +1     
==========================================
+ Hits          367      368       +1     
  Misses         51       51              

Impacted Files	Coverage Δ
src/rule_definition_tools.jl	93.57% <100.00%> (+0.05%)	⬆️

---

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 937981c...f2b0b7f. Read the comment docs.