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

setfield! does not call convert #16195

Closed
stevengj opened this issue May 4, 2016 · 10 comments · Fixed by #24960
Closed

setfield! does not call convert #16195

stevengj opened this issue May 4, 2016 · 10 comments · Fixed by #24960

Comments

@stevengj
Copy link
Member

stevengj commented May 4, 2016

type Foo; x::Float64; end
foo = Foo(0)
foo.x = 3
foo.(:x) = 4
setfield!(foo, :x, 5)

The last line throws TypeError: setfield!: expected Float64, got Int64.

@yuyichao
Copy link
Contributor

yuyichao commented May 4, 2016

This is expected since convert is inserted by the parser. Calling convert in the actual builtin sounds much more complicated (it needs to find the right one for example).

@stevengj
Copy link
Member Author

stevengj commented May 4, 2016

Since the foo.(:x) syntax may be going away (#6268 and/or #15032), it seems desirable to beef up setfield! a bit so that it is more usable.

@stevengj
Copy link
Member Author

stevengj commented May 4, 2016

The relevant code is in builtins.c. Since it knows the type that it wants at that point, it doesn't seem too hard to insert a call to Base.convert.

@vtjnash
Copy link
Member

vtjnash commented May 4, 2016

We can change it so that Base exports:

setfield!(x, i, v) = Core.setfield!(x, i, convert(fieldtype(x, i), v))

Otherwise, it's pretty hard to insert a call to convert directly from an Intrinsic, since Core.Intrinsics.convert is probably not what you ever intended to call (it's undefined) :P

stevengj added a commit to stevengj/julia that referenced this issue May 6, 2016
stevengj added a commit to stevengj/julia that referenced this issue May 27, 2016
stevengj added a commit to stevengj/julia that referenced this issue Aug 23, 2016
vtjnash added a commit that referenced this issue Dec 5, 2017
vtjnash added a commit that referenced this issue Dec 5, 2017
vtjnash added a commit that referenced this issue Dec 7, 2017
New functions `Base.getproperty` and `Base.setproperty!`
can be overloaded to change the behavior of `x.f` and `x.f = v`,
respectively.

fix #16226 (close #16195)
fix #1974
vtjnash added a commit that referenced this issue Dec 7, 2017
New functions `Base.getproperty` and `Base.setproperty!`
can be overloaded to change the behavior of `x.f` and `x.f = v`,
respectively.

fix #16226 (close #16195)
fix #1974
vtjnash added a commit that referenced this issue Dec 11, 2017
New functions `Base.getproperty` and `Base.setproperty!`
can be overloaded to change the behavior of `x.f` and `x.f = v`,
respectively.

fix #16226 (close #16195)
fix #1974
vtjnash added a commit that referenced this issue Dec 15, 2017
New functions `Base.getproperty` and `Base.setproperty!`
can be overloaded to change the behavior of `x.f` and `x.f = v`,
respectively.

fix #16226 (close #16195)
fix #1974
vtjnash added a commit that referenced this issue Dec 17, 2017
New functions `Base.getproperty` and `Base.setproperty!`
can be overloaded to change the behavior of `x.f` and `x.f = v`,
respectively.

fix #16226 (close #16195)
fix #1974
vtjnash added a commit that referenced this issue Dec 18, 2017
New functions `Base.getproperty` and `Base.setproperty!`
can be overloaded to change the behavior of `x.p` and `x.p = v`,
respectively.

This forces inference constant propagation through get/setproperty,
since it is very likely this method will yield better information after specializing on the field name
(even if `convert` is too big to make us want to inline the generic version and trigger the heuristic normally).

closes #16195 (and thus also closes #16226)
fix #1974
@tkf
Copy link
Member

tkf commented Apr 4, 2019

This issue seems to be not resolved yet. I can reproduce the issue in Julia 1.0, 1.1, and 1.2-DEV.

   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.0.3 (2018-12-18)
 _/ |\__'_|_|_|\__'_|  |  Official https://julialang.org/ release
|__/                   |

julia> mutable struct Foo x::Float64; end

julia> foo = Foo(0)
Foo(0.0)

julia> foo.x = 3
3

julia> setfield!(foo, :x, 5)
ERROR: TypeError: in setfield!, expected Float64, got Int64
Stacktrace:
 [1] top-level scope at none:0

cc @stevengj

ref JuliaPy/PyCall.jl#675

@stevengj stevengj reopened this Apr 4, 2019
@yuyichao
Copy link
Contributor

yuyichao commented Apr 4, 2019

julia> mutable struct Foo x::Float64; end

julia> foo = Foo(0)
Foo(0.0)

julia> foo.x = 3
3

julia> setproperty!(foo, :x, 5)
5.0

setfield! is the wrong function to call.

@yuyichao yuyichao closed this as completed Apr 4, 2019
@tkf
Copy link
Member

tkf commented Apr 4, 2019

setfield! is the right function to call if you override setproperty!. This is the case in JuliaPy/PyCall.jl#675. I think it is useful to have a uniform behavior for set-field operation. At least, I think the difference is better be documented to close this issue.

@yuyichao
Copy link
Contributor

yuyichao commented Apr 4, 2019

No, for a start, this was an issue because setfield! was documented as the lowering of a.b = c which is not the case. The solution is simple, creating a documented function that exactly corresponds to a.b = c and that's setproperty!. Having setfield! (i.e. the exact spelling) doing extra work was never the intent of the issue.

I think it is useful to have a uniform behavior for set-field operation

There isn't any non-uniform behavior of set-field operation. The dot syntax is always lowered to setproperty! which I believe is documented well.

setfield! is the right function to call if you override setproperty!

It is the right function to use only if its behavior is what you want. If, however, what you want is the behavior of the default setproperty! implementation, then you should call that instead. The generic way to do that is invoke, no different from any other overloadable functions.

@yuyichao
Copy link
Contributor

yuyichao commented Apr 4, 2019

I think the difference is better be documented

Also, please actually check the document of setfield!. It explicitly says,

The value must be mutable and x must be a subtype of fieldtype(typeof(value), name).

and also crossed linked to setproperty!.

Improving the doc is certainly always welcome but AFAICT there isn't anything missing for this issue so there's no need to keep an issue open for potential generic doc improvement.

@tkf
Copy link
Member

tkf commented Apr 5, 2019

Sorry, I have no idea why I missed that part. Thanks for the clarification.

Keno pushed a commit that referenced this issue Jun 5, 2024
New functions `Base.getproperty` and `Base.setproperty!`
can be overloaded to change the behavior of `x.p` and `x.p = v`,
respectively.

This forces inference constant propagation through get/setproperty,
since it is very likely this method will yield better information after specializing on the field name
(even if `convert` is too big to make us want to inline the generic version and trigger the heuristic normally).

closes #16195 (and thus also closes #16226)
fix #1974
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 a pull request may close this issue.

4 participants