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

Fix Tricks.jl to work for closures #30

Merged
merged 7 commits into from
Sep 26, 2021
Merged

Conversation

NHDaly
Copy link
Collaborator

@NHDaly NHDaly commented Aug 26, 2021

Before this PR, the static functions wouldn't work for closures, because their type doesn't have a singleton type.instance defined (since there can be more than one instance!):

julia> make_add_n(n) = x->x+n
make_add_n (generic function with 1 method)

julia> func = make_add_n(2)
#3 (generic function with 1 method)

julia> typeof(func)
var"#3#4"{Int64}

julia> typeof(func).instance
ERROR: UndefRefError: access to undefined reference
Stacktrace:
 [1] getproperty(x::Type, f::Symbol)
   @ Base ./Base.jl:28
 [2] top-level scope
   @ REPL[13]:1

julia> hasmethod(func, Tuple{Int})
true

julia> static_hasmethod(func, Tuple{Int})
ERROR: UndefRefError: access to undefined reference
Stacktrace:
 [1] getproperty(x::Type, f::Symbol)
   @ Base ./Base.jl:28
 [2] #s1#1
   @ ~/.julia/dev/Tricks/src/Tricks.jl:20 [inlined]
 [3] var"#s1#1"(T::Any, ::Any, f::Any, t::Any)
   @ Tricks ./none:0
 [4] (::Core.GeneratedFunctionStub)(::Any, ::Vararg{Any, N} where N)
   @ Core ./boot.jl:571
 [5] top-level scope
   @ REPL[16]:1

After this PR, it works correctly. 😊

I had to copy some functionality over from Base, because they don't provide easy utility functions that work on function-types, only on function instances, so we had to build that ourselves (since we have only function-types, thanks to the generated function).

Copy link
Contributor

@MasonProtter MasonProtter left a comment

Choose a reason for hiding this comment

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

Looks good to me! Just a couple legibility suggestions based on my personal taste. Feel free to ignore them though if you disagree.

src/Tricks.jl Outdated Show resolved Hide resolved
src/Tricks.jl Outdated Show resolved Hide resolved
@NHDaly
Copy link
Collaborator Author

NHDaly commented Aug 26, 2021

Thanks @MasonProtter - have another look if you can? :) i don't have the power to re-request a review, unfortunately.

Copy link
Contributor

@MasonProtter MasonProtter left a comment

Choose a reason for hiding this comment

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

The implementation is good so far as I can tell.

The only thing I can think of is maybe also add a test with a closure that captures a Box'd value or a Ref? I know it shouldn't matter, but sometimes weird things can be different when a value isn't inline.

We should also think about just dropping support for versions less than v"1.3". IMO it is dangerous and incorrect that these functions used to not support closures.

@NHDaly
Copy link
Collaborator Author

NHDaly commented Aug 26, 2021

dangerous

well, thankfully they just threw an error, so i don't think it's dangerous?

but yeah, i was surprised to see support for julia < 1.3 as well.

@NHDaly
Copy link
Collaborator Author

NHDaly commented Aug 26, 2021

The only thing I can think of is maybe also add a test with a closure that captures a Box'd value or a Ref? I know it shouldn't matter, but sometimes weird things can be different when a value isn't inline.

can you suggest a test? It'd be easier than me trying to guess at what you have in mind i think! ❤️

@oxinabox
Copy link
Owner

Urg, looks like this repo doesn't have GHA setup.
I am loath to merge this if I can't check CI.
Can I leave this open til I find time to add CI?

@oxinabox
Copy link
Owner

I have given you merge rights.
And I have added CI to master.
Rebase and merge when CI passes?

@NHDaly
Copy link
Collaborator Author

NHDaly commented Aug 27, 2021

Awesome - thanks!! :)

@codecov-commenter
Copy link

codecov-commenter commented Aug 27, 2021

Codecov Report

Merging #30 (88f65df) into master (7349d24) will decrease coverage by 1.56%.
The diff coverage is 95.83%.

Impacted file tree graph

@@             Coverage Diff             @@
##            master      #30      +/-   ##
===========================================
- Coverage   100.00%   98.43%   -1.57%     
===========================================
  Files            1        1              
  Lines           45       64      +19     
===========================================
+ Hits            45       63      +18     
- Misses           0        1       +1     
Impacted Files Coverage Δ
src/Tricks.jl 98.43% <95.83%> (-1.57%) ⬇️

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 7349d24...88f65df. Read the comment docs.

@NHDaly
Copy link
Collaborator Author

NHDaly commented Aug 27, 2021

Cool, i'm glad you added CI, since this wasn't working on julia 1.3 (or any versions < v1.6).

I've fixed it now, but it's even uglier 🙈
I do wonder if there's some higher-level public interfaces that I missed here, that we should have been calling instead?

If you don't mind, I'll leave this PR open for another round of reviews from you both to see if maybe you can see some cleaner APIs to call that I missed? 🙏 thanks!

@NHDaly NHDaly requested a review from oxinabox August 27, 2021 14:33
@coveralls
Copy link

Pull Request Test Coverage Report for Build 1174594970

  • 0 of 16 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-2.3%) to 97.674%

Totals Coverage Status
Change from base Build 1173795341: -2.3%
Covered Lines: 42
Relevant Lines: 43

💛 - Coveralls

@coveralls
Copy link

Pull Request Test Coverage Report for Build 1174594970

  • 16 of 16 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-2.3%) to 97.674%

Totals Coverage Status
Change from base Build 1173795341: -2.3%
Covered Lines: 42
Relevant Lines: 43

💛 - Coveralls

@coveralls
Copy link

coveralls commented Aug 27, 2021

Pull Request Test Coverage Report for Build 1275872199

  • 18 of 18 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 1222338592: 0.0%
Covered Lines: 51
Relevant Lines: 51

💛 - Coveralls

Copy link
Owner

@oxinabox oxinabox left a comment

Choose a reason for hiding this comment

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

let's do it, can always clean up later

Copy link
Contributor

@MasonProtter MasonProtter left a comment

Choose a reason for hiding this comment

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

LGTM

@NHDaly
Copy link
Collaborator Author

NHDaly commented Sep 26, 2021

Alright - sorry for my delay. :) Merging now!

@NHDaly NHDaly merged commit bab6a7a into oxinabox:master Sep 26, 2021
@NHDaly NHDaly deleted the nhd-closures branch September 26, 2021 20:03
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.

5 participants