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

Deprecation for Uint directs user to UInt64 instead of UInt #13221

Closed
c42f opened this issue Sep 19, 2015 · 8 comments
Closed

Deprecation for Uint directs user to UInt64 instead of UInt #13221

c42f opened this issue Sep 19, 2015 · 8 comments

Comments

@c42f
Copy link
Member

c42f commented Sep 19, 2015

With version 0.5.0-dev+231:

julia> Uint
WARNING: Base.Uint is deprecated, use UInt64 instead.
UInt64

It looks like @deprecate_binding recommends the underlying type to the user rather than the typealias name. Clearly in this case it should be recommending UInt rather than the system specific UInt64.

@ViralBShah
Copy link
Member

The definition is correct in deprecated.jl, but the alias is probably getting resolved to the native type.

@c42f
Copy link
Member Author

c42f commented Sep 19, 2015

Right, it's not @deprecate_binding itself which is at fault, but a function which is called somewhere down the chain (looks to be in the C code?)

@c42f
Copy link
Member Author

c42f commented Sep 19, 2015

I did some digging. The code in jl_binding_deprecation_warning sure looks sensible. I didn't manage to find how type aliases are resolved, but if lookup to the underlying type happens when the binding is created, maybe there's no way to recover the intermediate type name without adding unwanted fields to the C data structures?

I'm happy to dig further if anyone has hints, but at this stage I think I'm stuck without doing a whole lot of reading to figure out how things work. I did notice a related bug in the process of looking around, but after a git pull I found it already fixed in 3e3f353 :-)

@yuyichao
Copy link
Contributor

This is expected given the implementation as Jeff already said in the commit message

This is a rather minimal implementation that only really works for types and functions, since it relies on printing those objects to show the replacement. Fortunately all existing deprecated bindings are for types.

I agree the printing is a little confusing and could be improved. You can probably try to pass in the name as a symbol and store it in the deprecated field of jl_binding_t (so make it a jl_sym_t*) and use that in the printing instead.

@c42f
Copy link
Member Author

c42f commented Sep 19, 2015

only really works for types and functions

as opposed to typealias I guess, and other things such as numeric constants?

Would it really be acceptable to burn an additional pointer in jl_binding_t to fix this (seems like a lot of bloat to me)? Ideally there'd be zero cost to pay for the deprecation machinery in the normal use case. Jeff has come really close to this already, using only a single bit to indicate binding deprecation.

How about storing deprecation information in a dictionary instead, keyed on the binding name and module? The lookup may be relatively expensive, but maybe that's acceptable. I'm not sure how it would work, but if the dictionary could be maintained on the julia side, there'd be much more flexibility in how to present deprecation messages.

@JeffBezanson
Copy link
Member

For now let's just hack this special case into jl_binding_deprecation_warning.

@c42f
Copy link
Member Author

c42f commented Sep 19, 2015

Sure, fair enough for 0.4 to hardcode a special case for something as common as Base.Uint. I'll create a disgusting special case fix.

@c42f
Copy link
Member Author

c42f commented Sep 19, 2015

Well, I created a workaround. Yes, I do feel appropriately bad about it.

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

No branches or pull requests

4 participants