ensure binding type is set consistently for consts #44232
Conversation
simeonschaub
added
backport 1.8
Previously, `get_binding_type` would usually return `Any` for constants. I think always setting it to the type of the actual value is probably useful.
simeonschaub
force-pushed
the
sds/const_binding_type
branch
from
5758e80 to
5db09a7
Compare
| if (b->constp) { | ||
| jl_atomic_cmpswap_relaxed(&b->ty, &old_ty, jl_typeof(rhs)); |
There was a problem hiding this comment.
A bit fussy perhaps, but I think it would make sense to move this inside the if (jl_atomic_cmpswap for the value.
There was a problem hiding this comment.
I'm pretty sure right now we always assume that a non-null value implies that the binding type is also set, while setting a type for an uninitialized binding is perfectly fine. In case of a race we might have set the wrong type, but this should then be caught and error appropriately before we set the value to something inconsistent. Or is there another reason for doing it the other way around?
There was a problem hiding this comment.
I forgot to add another check of the binding type here though, I can add that. (Although declaring constants isn't really thread-safe right now anyways.)
There was a problem hiding this comment.
I was just thinking of making setting the value and type more atomic, but I guess the behavior ends up being the same either way.
|
What's the status here? |
|
It currently doesn't work and I'm not 100% sure anymore we actually want to do this. Regarding the release of 1.8 at least, I don't think this needs to be a blocker |
|
Yeah, I'm wondering whether we should just special case constants in |
|
Closing in favor of #44632 |
|
Yes, I'd like it to be |
|
I assume you're aware of #40985 (comment)? I can try to get this to work instead. |
|
Well, DataTypes with free typevars aren't really first class. I'm fine if this returns |
Previously,
get_binding_typewould usually returnAnyfor constants.I think always setting it to the type of the actual value is probably
useful.