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

WIP: change getfield lowering and call overloaded version #5848

Closed
wants to merge 6 commits into from

Conversation

ihnorton
Copy link
Member

Here is an attempt to implement getfield overloading, for #1974.

@JeffBezanson
Copy link
Member

This is an admirable show of patience in digging through my dirty laundry :)

The biggest issue is deciding what exactly a.b means. The essential behavior here is that if the type has the specified field, a direct access is done, otherwise the generic version is called. I think it might be better to allow overloading even of the type's actual field names. It's strange for (re)naming a field to interfere with method applicability. But I'm open to debate on this.

Also, you want as much behavior as possible defined through method definitions, since the compiler already knows how to analyze those. Anything else will require more special cases. For example

getfield(x, f::Field) = Core.getfield(x, f)

will automatically do direct access for any fields that lack method definitions.

Using emit_call in codegen will not work, since that is too late for the call to be properly optimized.

@StefanKarpinski
Copy link
Member

The essential behavior here is that if the type has the specified field, a direct access is done, otherwise the generic version is called. I think it might be better to allow overloading even of the type's actual field names. It's strange for (re)naming a field to interfere with method applicability. But I'm open to debate on this.

I agree with this and the approach of using method dispatch for field lookup. It's not just better for the compiler – it reduces yet another language feature to method dispatch, which is just elegant.

@ihnorton
Copy link
Member Author

This PR now implements generic version (see sysimg).

Still WIP pending a couple test failures that should be tractable.

import Base: getfield

type Foo
end
foo = Foo()

getfield(x::Foo, ::Type{Field{:bar}}) = "Got foo.bar"
julia> foo.bar
"Got foo.bar"

@ihnorton ihnorton reopened this Apr 21, 2014
@stevengj
Copy link
Member

What is the status of this? It would be great to see some progress on this in 0.4.

@tknopp
Copy link
Contributor

tknopp commented Jun 27, 2014

@stevengj: The discussion in #1974 is not really finished with a consensus

@ihnorton
Copy link
Member Author

Agreed that this feature (if perhaps not this PR) would be really nice for 0.4. This PR still has a fundamental flaw in the module reference resolution. I do intend to look at it again, but not sure how much further I will get.

@stevengj
Copy link
Member

@ihnorton, @tknopp, note that a.b overloading was just added to the 0.4 projects list. The consensus of the Great Hornéd Ones appears to be that the feature is deemed Worthy.

@StefanKarpinski
Copy link
Member

Well, that's what I think. We'll see what @JeffBezanson thinks – a while back I think we agreed on this, but opinions to change over time.

@stevengj
Copy link
Member

I spoke with Jeff a few days ago, and he was still generally favorable to dot overloading as well.

@tknopp
Copy link
Contributor

tknopp commented Aug 18, 2014

I don't want to be the bad guy here. But the discussion in #1974 raises several issue which need to be IMHO clarified.

Note that I am not sure either if I want this or not. The biggest issue I see is that currently there is a clear distinction between the public and the private interface. This will break and we will need a clear guideline when and how to use it.

@StefanKarpinski
Copy link
Member

I think we'll need to feel this one out a bit once we have the functionality. We already tend to regard fields as private and I think that making x..f the "real" field access syntax will make that more so – it will be considered very poor form to directly access someone else's fields in a non-overloadable way. In a sense a private field would be one that you can only access with the x..f notation, while a public field would be one for which x.f works. We could even arrange that prefixing a field or bunch of fields with private would prevent generation of the x.f accessor. Or maybe we just don't generate x.f accessors by default and you have to do that yourself to make a field public.

@JeffBezanson
Copy link
Member

Yes, generally in favor, but concerned that the x..f notation is not that great, and that we might see divergence in use of x.length vs. length(x) etc.

As a slightly more abstract problem, I'm not sure it's good for Module.x to be subject to overloading. In another sense it's good though, since .. can mean only field access, and Module.x can be an overload that calls something like module_lookup. It will be nice to be able to write Module..name instead of module_name(Module).

However this makes julia harder to compile, since you basically must do an extra round of inference to resolve the Module.name expressions. This also makes life difficult for future code-analysis tools. It also makes certain things uglier, since import M.x will probably keep this syntax, but it clearly won't be calling any getfield overloads. A related case is function Base.f(...); should that depend on what getfield definitions are visible?

@tknopp
Copy link
Contributor

tknopp commented Aug 18, 2014

This is a really hard decision because it will have a very major impact on the language. The language bindings are one thing but if we really need to have "public fields" is IMHO another one. What about making a hard split? If a type x has a field y it will always be accessible by x.y. If it has no field y the getfield method will be called. So make the language bindings guys (and Gtk and enum) happy but nobody wants to rely on this to be a public interface.

Wouldn't this be a much less invasive change?

@ihnorton
Copy link
Member Author

Perhaps we should keep the language-level discussions in #1974 for the time being.

@stevengj
Copy link
Member

Any chance of resurrecting this? It seems like all of the linguistic debates have devolved to mere bikeshedding over .., and for now we could punt and use Core.getfield or __getfield rather than a special operator syntax.

@ihnorton
Copy link
Member Author

Ok, I'll have another go at this.

@stevengj
Copy link
Member

Awesome, thanks for rebasing!

@stevengj
Copy link
Member

@JeffBezanson and @ihnorton, what are the major remaining issues with this?

@ihnorton
Copy link
Member Author

Not really ready for review. I have more local work that fixed most of the inference issues (I think). But I'm stuck in bootstrap purgatory now.

@stevengj
Copy link
Member

stevengj commented Jul 1, 2015

Any timeline for resurrecting this?

@yuyichao
Copy link
Contributor

yuyichao commented Jul 5, 2015

Just to state the reason I like getfield overload before I forgot about it since I didn't find it explicitly mentioned anywhere in the thread (although I might just have missed it). I've also talked about it with @carnaval a while ago.

I think getfield overload is a nice way to avoid name clashing of object properties with local variables by having per-object namespace.

For example, I would like to have a type/object for an optical transition, which has many properties (frequency freq, angular frequency omega, wavelength lambda, line width gamma etc). I don't want to store everything in the structure since they are not independent and are not always all needed. I also don't want to write a global function for each of them since they would collide with one of my local variables very easily unless I make the name really long. Having the method names bind to the object would be the best solution to this problem I can think of.

Any timeline for resurrecting this?

Probably after 0.4 is released?

@ihnorton
Copy link
Member Author

ihnorton commented Aug 4, 2015

Will open a new PR if I get further in the future.

@ihnorton ihnorton closed this Aug 4, 2015
@stevengj
Copy link
Member

Would be great to see a PR on this for 0.5.

@yuyichao
Copy link
Contributor

I just rebased this on the current master at yyc/ovldot although I won't necessarily have time to push this further anytime soon either......

@stevengj
Copy link
Member

Can you or @ihnorton open a new PR with that? It's easier to discuss a PR than a branch.

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.

6 participants