Skip to content

Conversation

oxinabox
Copy link
Contributor

Before:

julia> whereis(@which sum([1]; dims=1))
("none", 0)

julia> definition(String, @which sum([1]; dims=1))
ERROR: SystemError: opening file "none": No such file or directory

After, both of these return nothing.

# kwargs that result in not finding the file
m = @which sum([1]; dims=1)
loc = whereis(m)
@test loc != ("none", 0) # old incorrect behavour.
Copy link
Member

Choose a reason for hiding this comment

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

This test is redundant with the one below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is indeed, but it serves to illustrate what was wrong before.
It can be removed, but I thought it might be helpful in future changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think it should be removed?

Suggested change
@test loc != ("none", 0) # old incorrect behavour.

Return a string with the code that defines `method`. Also return the first line of the
definition, including the signature (which may not be the same line number returned
by `whereis`).
If the method could not be found for some reason, returns `nothing`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
If the method could not be found for some reason, returns `nothing`.
If the method could not be found for some reason, then return `nothing`.

Co-Authored-By: Kristoffer Carlsson <kristoffer.carlsson@chalmers.se>
@codecov
Copy link

codecov bot commented Aug 22, 2019

Codecov Report

Merging #48 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@         Coverage Diff          @@
##           master   #48   +/-   ##
====================================
  Coverage       0%    0%           
====================================
  Files           3     3           
  Lines         165   169    +4     
====================================
- Misses        165   169    +4
Impacted Files Coverage Δ
src/CodeTracking.jl 0% <0%> (ø) ⬆️

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 3df13a7...4642055. Read the comment docs.

Co-Authored-By: Kristoffer Carlsson <kristoffer.carlsson@chalmers.se>
@timholy
Copy link
Member

timholy commented Aug 22, 2019

The better approach is to fix the underlying problem. In this case the issue is

julia> m
(::getfield(Base, Symbol("#kw##sum")))(::Any, ::typeof(sum), a::AbstractArray) in Base

julia> m.file
:none

which makes it impractical to find it. However, with a suitably-fixed version of bodymethod (in LoweredCodeUtils), we have

julia> mbody = LoweredCodeUtils.bodymethod(m)
#sum#558(dims, ::typeof(sum), a::AbstractArray) in Base at /home/tim/src/julia-1/base/reducedim.jl:652

and voila we have it.

I'll submit the PRs (LCU, CodeTracking, and Revise) needed to make this work.

I'm not against this change, but I am about to invalidate your test 😄.

@oxinabox
Copy link
Contributor Author

oxinabox commented Aug 23, 2019

@timholy great to have this fixed properly.
What happens for the case when Revise isn't loaded?
I'ld like to fail gracefully then. For what ever gracefully means.

@timholy
Copy link
Member

timholy commented Aug 23, 2019

I'd call this graceful:

julia> whereis(@which sum([1]; dims=1))
("none", 0)

@oxinabox
Copy link
Contributor Author

I would not call that graceful, because potentially
"none" could be a valid filename.

Further definition(String, @which sum([1]; dims=1) will still error right?
Because it tries to open that path.

@timholy
Copy link
Member

timholy commented Aug 23, 2019

But no actual method has a line number of 0. (Internally, Julia checks this kind of thing fairly frequently.) You can also use isfile to see if "none" really is a file. I'm not completely against returning nothing, but whereis is kind of the more powerful/accurate variant of checking m.file and m.line. And you can always do that and get something back.

The definition example is perhaps more problematic. What should it do besides error? Return nothing?

@oxinabox
Copy link
Contributor Author

but whereis is kind of the more powerful/accurate variant of checking m.file and m.line.

That sounds like a reasonable argument for the current behavour.
So I am sold.

The definition example is perhaps more problematic. What should it do besides error? Return nothing?

I think so.
Sometimes already definition{Expr, method) returns nothing. (#45)
So it makes sense to me that sometimes definition(String, method) might return nothing.

timholy added a commit that referenced this pull request Aug 23, 2019
timholy added a commit that referenced this pull request Aug 23, 2019
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.

3 participants