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

implement getfield overloading #24960

Merged
merged 3 commits into from
Dec 18, 2017
Merged

implement getfield overloading #24960

merged 3 commits into from
Dec 18, 2017

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented 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. It seemed like about time to turn this on. (Also it means I get to close another 4-digit issue.)

fix #16226 (close #16195)
fix #1974

TODO:

  • Docs & News
  • Nanosoldier run

@ararslan ararslan added needs docs Documentation for this change is required needs news A NEWS entry is required for this change labels Dec 7, 2017
@KristofferC
Copy link
Member

If I would want to overload only certain fields, is this the goto way?

julia> struct S
       x::Int
       y::Float64
       z::Float64
       end

julia> s = S(1, 2.0, 3.0)
S(1, 2.0, 3.0)

julia> @inline Base.getproperty(s::S, v::Symbol) = _getproperty(s, Val(v))

julia> @inline _getproperty(s::S, v::Val{V}) where {V} = Core.getfield(s, V)
_getproperty (generic function with 1 method)

julia> @inline _getproperty(s::S, v::Val{:z}) = 2 * Core.getfield(s, :z)
_getproperty (generic function with 2 methods)

julia> f(s) = s.z
f (generic function with 1 method)

julia> f(s)
6.0

julia> @code_warntype f(s)
Variables:
  s::S
  px<optimized out>
  py<optimized out>
  R<optimized out>

Body:
  begin
      return (Base.mul_float)(2.0, (Core.getfield)(s::S, :z)::Float64)::Float64
  end::Float64

@quinnj
Copy link
Member

quinnj commented Dec 7, 2017

With constant prop, you don't need to make Vals anymore.

@KristofferC
Copy link
Member

Lifting from value domain to type domain like above would be type unstable without IPO anyway, so the reason for writing it like this is not type stability. It is just that it is a convenient way of defining methods for fields.

Of course you could also do something like:

function Base.getproperty(s::S, v::Symbol)
    if v == :x
        return _getproperty_x(s)
    elseif v == :y
        return _getproperty_y(s)
     .
     .
     else
         return Core.getfield(s, v)
    end
end

But that is imo uglier.

@StefanKarpinski
Copy link
Member

Python uses the name getattr and Ruby also refers to these as "attributes" frequently with the "attr" abbreviation, so how about calling this getattr and setattr! instead?

@vtjnash
Copy link
Member Author

vtjnash commented Dec 7, 2017

Python uses the name getattr

Python also calls them properties. (The equivalent function in python would be __getattribute__, not __getattr__).

@@ -90,7 +90,7 @@ const _llvmtypes = Dict{DataType, String}(
"""
return quote
Base.@_inline_meta
Base.llvmcall($exp, Vec{$N, $T}, Tuple{Vec{$N, $T}, Vec{$N, $T}}, x, y)
Core.getfield(Base, :llvmcall)($exp, Vec{$N, $T}, Tuple{Vec{$N, $T}, Vec{$N, $T}}, x, y)
Copy link
Member

@vchuravy vchuravy Dec 7, 2017

Choose a reason for hiding this comment

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

Why is this necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because Core.llvmcall is actually special syntax and not a valid function. It also works as just llvmcall, as more commonly written.

@stevengj
Copy link
Member

stevengj commented Dec 7, 2017

Great to see this finally happening!!

Why can't they just be called getfield and setfield!? Is it because the latter are used to get "real" fields or something? We could just use Core.getfield for those in the rare cases where you need to distinguish?

@vtjnash vtjnash force-pushed the jn/getfield-overloading branch from 74146c9 to 2a9c500 Compare December 7, 2017 19:35
@vtjnash
Copy link
Member Author

vtjnash commented Dec 7, 2017

Why can't they just be called getfield and setfield!?

I tried that initially, and it was just really annoying to fix all existing calls to these functions. I found that almost always, where there were direct calls to these functions, the user wanted to reflect on the actual field value. Picking a new name just worked much better.

@tknopp
Copy link
Contributor

tknopp commented Dec 7, 2017

I like Base.getproperty and Base.setproperty! a lot since this seems to be a common term for this functionality. C# makes heavy use of them. getfield is kind of wrong since this is not really a field access anymore.

Here is a citation from the C# documentation:

Properties behave like fields when they are accessed. However, unlike fields, properties are implemented with accessors that define the statements executed when a property is accessed or assigned.

https://docs.microsoft.com/en-us/dotnet/csharp/properties

@tknopp
Copy link
Contributor

tknopp commented Dec 7, 2017

I still stand in awe of this feature since it has the potential to change how we think about interfaces. I don't want to start this discussion here again, but it would be essential to form some guidelines on this. Wanting properties but not wanting array.length is from my perspective still a contradiction. So I am +1 on this feature, but only if we fully exploit it. Otherwise I think its too dangerous.

@vtjnash
Copy link
Member Author

vtjnash commented Dec 7, 2017

I have nothing personally against array.length. However, it's important to note that my array.length might not have anything to do with your file.length, since there is no name-spacing. But my Base.length(array) is the interface for getting the number of elements in an enumerable container. But in the end, it's always your choice whether you want your code to compose well with others or not.

@StefanKarpinski
Copy link
Member

StefanKarpinski commented Dec 7, 2017

Wikipedia is very helpful here :trollface::

In computing, an attribute is a specification that defines a property of an object, element, or file.

@stevengj
Copy link
Member

stevengj commented Dec 7, 2017

See also this stackoverflow discussion and this one. Most people seem to think "property" refers to custom getter/setter functionality.

@StefanKarpinski
Copy link
Member

Seems like that SO thread would indicate "property" as the right term, at least coming from Java.

@andyferris
Copy link
Member

I like the "property" terminology.

I think the current system without getfield overloading has encouraged people to think about generic & composable interfaces through overloaded functions. I'm slightly worried how much effort will be need to convince people to continue such thinking. We'll see, I guess!

Other than that, I'm sure this may be very useful, in those situations where it can be used well.

@ararslan
Copy link
Member

ararslan commented Dec 8, 2017

I'm slightly worried how much effort will be need to convince people to continue such thinking.

I absolutely agree with this. By enabling this we're inviting people who come from languages like Python and Ruby to implement interfaces via getfield overloading because it's just what they're used to, rather than encouraging them to familiarize themselves with what makes Julia special, which would allow them to create generic, Julian APIs. It's been an enormous benefit that the ecosystem has been as consistent as it has been, both with the Base library and with other packages. If we go through with this, we're running the real risk of fracturing the ecosystem, right before 1.0.

@KristofferC
Copy link
Member

If we go through with this, we're running the real risk of fracturing the ecosystem, right before 1.0.

This sounds very excessive to me. Any feature can be misused. The whole type system in Julia can be overused and we even have a section in the manual with warnings about this. We can do the same for this feature. I have many cases where this would significantly clean up the interface and it is quite annoying to me when someone says that a feature shouldn't be implemented because someone might use it wrong.

@tknopp
Copy link
Contributor

tknopp commented Dec 8, 2017

I share @ararslan's view although I am a little bit more positive. The point is: If we introduce this feature, we will effectively introduce a new way to get/set properties of an object. This will require differentiating between what is a function and what is a property of an object. If we do this and have some well defined guideline this can work out pretty well. But if we don't have a clear guideline this can become chaos.

My vote here is to get the feature in as experimental. And then have a discussion on how to use it. This will allow the obvious use cases (language bindings) to use it and we can discuss the general usage without haste (post 1.0). I am not so much concerned that an exotic package will use it. Consistency is reached through base and the "highly used" standard packages. If we have a well defined guideline there should not be a consistency decrease.

More important in the context of this PR is if the feature is technically ok. Here my question is if there are any performance issues associated with this? Will a trivial getter be inlined leading to the same assembly code?

@KristofferC
Copy link
Member

we will effectively introduce a new way to get/set properties of an object.

I would rephrase this as, "it is now actually possible to have a property of an object be part of a public API". Previously, accessing a property of an object has always been subject to breaking when the this field happens to be renamed or change e.g. its type. This is for example how all (most) of the packages that depended on Optim broke, when it updated some internals.

Having to write gettters and setters for every field is so tedious that most just use direct field access anyway. This will actually make that possible without locking that code to the internals.

@mauro3
Copy link
Contributor

mauro3 commented Dec 8, 2017

I get first dibs with OO.jl:

using OO

# OO-ify Array
# note that this is not possible to do by default for all types as
# redefining Base.getproperty(self, p::Symbol) seg-faults Julia.
@OO Array
[1,2].length # ->2
[1,2].sum # ->3

@OO abstract type Animal end
_getproperty(self::Animal, ::Val{:eat}) = food -> println("$self eats $food")
@method Animal.eat(self, food) = println("$self eats $food")
@classmethod Animal.notsurewhat(arg) = println("This is a class-method saying: $arg")

@OO struct Dog <: Animal
    name
    breed
    color
    sound
end
Base.show(io::IO, d::Dog) = println(io, "Dog $(d.name)")
@method Dog.bark(self) = println("$(self.sound)")
@method Dog.bite(self, other) = println("Dog $(self.name) bites $other")

maclary = Dog("Hairy Maclary", "Terrier", "Black", "Yep yep yep yep")
maclary.bark() # Yep yep yep yep 
maclary.bite("me") # Dog Hairy Maclary bites me
maclary.eat("dog food") # Dog Hairy Maclary eats dog food
maclary.notsurewhat("Hi MacLary") # This is a class-method saying: Hi MacLary

I also worry a bit. In particular thin wrappers to Python libs will just be as they are in Python and leave the wrong impression on how Julia is "supposed" to be. However, I suspect Pandora's box is open and there is no way back.

@vtjnash
Copy link
Member Author

vtjnash commented Dec 8, 2017

That seems a bit overcomplicated. Here's a sketch of how I would do it:

struct Dog <: Animal
    name
    breed
    color
    sound
end
Base.show(io::IO, d::Dog) = println(io, "Dog $(d.name)")
bark(self::Dog, self) = println("$(self.sound)")

# Now add a method with `Dog` specified in a different place:
@method Dog.bite(self, other) = println("Dog $(self.name) bites $other")
# ^ defines:
# bite(self::Dog, other) = ...

# Now make it possible to call all methods on Dog (defined in Dog's module) using OO-syntax:
@OOify Dog
# ^ defines:
# function Base.getproperty(d::Dog, n::Symbol)
#     n in fieldnames(Dog) && return getfield(d, n)
#     func = getfield(@__MODULE__, n)
#     return (args...) -> func(d, args...)
# end

This will actually make that possible without locking that code to the internals

Not entirely. OO-style code will still have the global name-spacing problem that it has always had. What this will allow is defining deprecations, and in some limited cases dealing with cases were accessors are a useful API when defining some Abstract API (e.g. requiring that AbstractRange has a first field – I can't find a link to the discussion where it was observed recently that FloatRanges don't have that field for accuracy reasons, but it is easy to compute and could be a property.)

Here my question is if there are any performance issues associated with this? Will a trivial getter be inlined leading to the same assembly code?

No. Yes. @nanosoldier runbenchmarks(ALL, vs = ":master")

@vtjnash
Copy link
Member Author

vtjnash commented Dec 8, 2017

This will require differentiating between what is a function and what is a property of an object. If we do this and have some well defined guideline this can work out pretty well. But if we don't have a clear guideline this can become chaos.

https://news.ycombinator.com/item?id=1730411 :)

@swissr
Copy link
Contributor

swissr commented Dec 8, 2017

By enabling this we're inviting people who come from languages like Python and Ruby to implement interfaces via getfield overloading because it's just what they're used to, rather than encouraging them to familiarize themselves with what makes Julia special

I'm quite optimistic that 'people' will go the Julian way when pointed out (e.g. in Discours). Imho it has much to do with conventions/manual and what e.g. the doorkeepers for a Pkg3 'cathedral' registry recommend.

It's almost 5 years since #1974 and I think it is great that now finally important/core package authors have this syntax sugar when it makes sense (wouldn't have expected this to (likely) land in 1.0, what a nice surprise).

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

@chethega
Copy link
Contributor

chethega commented Dec 8, 2017

Glorious. Now I want something like

@inline getindex(p::Ptr{T}) where T = unsafe_load(p)
@inline getindex(p::Ptr{T}, i) where T = unsafe_load(p, i)

@inline setindex!(p::Ptr{T}, v::T) where T = unsafe_store!(p, T ,v)
@inline setindex!(p::Ptr{T}, v::T, i) where T = unsafe_store!(p, v, i)

@inline function _getproperty(p::Ptr{T}, v::Val{V}) where {T,V}
    offset = fieldoffset(T, fname)
    ftype = fieldtype(T, fname)
    return reinterpret(Ptr{ftype}, p+offset)
end

which makes .[] an equivalent of the C -> operator. Also needs a postfix reinterpret, and you can painlessly chase through pointy C-structs.

If we additionally get forward declarations (or a nice way of changing pointer types in a DataType after declaring it in julia, but before compiling code depending on it) then most structs can be expressed as datatypes in julia. If we additionally allow unions (either by providing syntax or by offering some ccall to add additional fieldname-fieldtype aliases at an offset) then we can probably express everything needed to talk to C without pain.

PS. This needs of course some equivalent of the weirdly missing

@inline @Base.pure function fieldoffset(T::DataType, fname::Symbol)
    for (i, sym) in enumerate(fieldnames(T))
        if sym==fname
            return fieldoffset(T, i)
        end
    end
    error("field $(fname) not present in $(T).")
end

Oh, and julia could export all internal C structs as julia structs, and give us a correctly typed pointer_from_objref, which would be significantly more transparent to users (who are then free to explore or crash their runtime).

@StefanKarpinski
Copy link
Member

Glorious. Now I want something like

This is exactly what people are worried about, I'm afraid. There's a reason that we made C-style pointer programming ugly and verbose: you should not be doing enough of it that you need nice syntax. Instead, you should wrap C APIs in a safe Julian API and then use that. Much of this can be done completely automatically with tools like Clang.jl.

@JeffBezanson
Copy link
Member

+1. I like the "property" terminology. Ultimately, I think we have no choice but to separate concrete field access APIs from an abstract property API, so clearer separation is better.

Defining properties with if-else feels un-julian to me too; on the other hand Val is not beautiful either. I don't have a great idea here yet.

@@ -914,6 +914,8 @@ export

# types
convert,
# getproperty,
# setproperty!,
Copy link
Member

Choose a reason for hiding this comment

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

export?

Copy link
Member Author

Choose a reason for hiding this comment

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

Keno asked me not to do so initially.

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
it's 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)
@vtjnash vtjnash force-pushed the jn/getfield-overloading branch from b418942 to db051ef Compare December 17, 2017 02:32
@vtjnash vtjnash removed the needs tests Unit tests are required for this change label Dec 17, 2017
@vtjnash
Copy link
Member Author

vtjnash commented Dec 17, 2017

Since everything is looking green and there doesn't seem to be any objections (triage said to go ahead with this), I'll plan to merge tomorrow.

@vtjnash vtjnash merged commit 4d3d49d into master Dec 18, 2017
@vtjnash vtjnash deleted the jn/getfield-overloading branch December 18, 2017 03:20
@davidanthoff
Copy link
Contributor

Just saw this now, and just want to say that this is fantastic! This will make soo many of my projects a heck of a lot simpler, so thank you!

| `A[i]` | [`getindex`](@ref) |
| `A[i] = x` | [`setindex!`](@ref) |
| `A.n` | [`getproperty`](@ref Base.getproperty) |
| `A.n = x` | [`setproperty!`](@ref Base.setproperty!) |
Copy link
Member

Choose a reason for hiding this comment

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

We really need more documentation on how to use this, explaining the arguments to getproperty, how to fall back to getfield, implications for performance and type stability, best practices....

Copy link
Member

Choose a reason for hiding this comment

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

Also, there are zero tests, so some tests would be good...

Copy link
Member

@ihnorton ihnorton Dec 18, 2017

Choose a reason for hiding this comment

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

@stevengj FWIW I played with this a bit last night for PyCall, and just put up what I have so far in case it helps (won't be able to look again until tonight or tomorrow). The getfield fallback is clearly suboptimal -- we should probably change all of the PyObject.o calls, while I only did a few -- and no setproperty! support, but the following works:

julia> np = pyimport("numpy")
PyObject <module 'numpy' from '/Users/inorton/.julia/v0.7/Conda/deps/usr/lib/python2.7/site-packages/numpy/__init__.pyc'>

julia> np.sin(1:100);

https://github.com/ihnorton/PyCall.jl/tree/getfield_parasido

Copy link
Member

@stevengj stevengj Dec 18, 2017

Choose a reason for hiding this comment

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

@ihnorton, thanks for looking into this! Yes, my plan is to release a PyCall 2.0 for Julia 0.7, and switch over completely to getproperty (finally dropping/deprecating @pyimport and pywrap). But I'm not planning to work on it until 0.7 is close to release.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, there are zero tests, so some tests would be good...

I wrote them. Apparently I forgot to forgot to push them.

vtjnash added a commit that referenced this pull request Dec 18, 2017
@sglyon sglyon mentioned this pull request Jan 30, 2018
3 tasks
Keno pushed a commit that referenced this pull request 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 this pull request may close these issues.

setfield! does not call convert allow overloading of a.b field access syntax