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

Omitting function body when overriding @:coreApi classes. #3285

Open
nadako opened this issue Aug 30, 2014 · 17 comments
Open

Omitting function body when overriding @:coreApi classes. #3285

nadako opened this issue Aug 30, 2014 · 17 comments
Milestone

Comments

@nadako
Copy link
Member

nadako commented Aug 30, 2014

While working on splitting haxe.io stuff, I once again came to think that it would be useful to be able to omit function body for non-extern @:coreApi classes which would mean that function body is copied from the common class.

For example:

Comon haxe.io.Input

class Input {
    public function readByte() : Int {
        throw "Not implemented";
    }

    public function readInt8() : Int {
        var n = readByte();
        if( n >= 128 )
            return n - 256;
        return n;
    }
}

Target-specific haxe.io.Input

@:coreApi class Input {
    public function readByte() : Int {
        return someImplementation();
    }

    public function readInt8() : Int; // use the same algorithm
}
@ncannasse
Copy link
Member

Seems fine, but maybe tag with a metadata for more explicit behavior ?

@Simn
Copy link
Member

Simn commented Sep 1, 2014

I would like to see a more general solution to this issue which is not tied to @:coreApi. I came across the same thing when playing with type classes where I would have an interface like this:

interface Eq<T> {
    public function equals(a:T, b:T):Bool;
    public function equalsNot(a:T, b:T):Bool;
}

In Haskell you could now create a concrete type class such as ArrayEq by defining either equals or equalsNot, with equals having a standard implementation of !equalsNot and vice versa.

I realize "there"s a macro for that", but if we go for some sort of field copying anyway we might as well do it properly and sell it as compile-time inheritance.

@nadako
Copy link
Member Author

nadako commented Sep 1, 2014

I"m not sure I follow...

@Simn
Copy link
Member

Simn commented Sep 1, 2014

You want to copy fields from base implementations depending on @:coreApi metadata. I want an independent solution for that so it can be used in user code.

@nadako
Copy link
Member Author

nadako commented Sep 1, 2014

Yeah, but I don"t understand how should that be used. You posted an example with interface, does that mean adding some default implementation to the interface itself?

@Simn
Copy link
Member

Simn commented Sep 1, 2014

That"s just because I needed multiple inheritance which we can"t really do with classes, even at macro-level. But now that you mention it a single inheritance copying solution would still be too limiting for my use case, so it wouldn"t really help much.

Anyway, I don"t like our current @:coreApi mechanic because it"s limited to the standard API and requires equal type paths. It would be much better if we had a nice compile-time inheritance mechanism, so a target-specific class could be defined to contain all the fields of some base class minus the ones it defines itself.

But this probably leads to traits and language extensions on a too large scale, so I"m not gonna interfere anymore with this particular issue.

@nadako
Copy link
Member Author

nadako commented Sep 1, 2014

I think @:coreApi will do fine for now. The two most significant problems i see with it right now is this issue (reusing common methods) and that it overrides whole modules instead of types (i.e. if we want to split haxe.CallStack, we should copy its inner enum StackItem everywhere, which sucks hard and is stopping me from splitting haxe.CallStack

@nadako
Copy link
Member Author

nadako commented Sep 7, 2014

Pls review #3327

nadako added a commit to nadako/haxe that referenced this issue Sep 9, 2014
nadako added a commit to nadako/haxe that referenced this issue Sep 18, 2014
@Simn Simn added this to the 3.3 milestone Dec 14, 2014
Simn added a commit that referenced this issue Nov 24, 2015
This is related to #3285, but it"s also useful in its own right for macro users.
@Simn
Copy link
Member

Simn commented Nov 24, 2015

I have implemented something on this branch: https://github.com/HaxeFoundation/haxe/tree/ast_source

It has two components:

  1. @:astSource: Adding this to a method causes the compiler to add the method expression as argument. It becomes @:astSource({ the expression }).
  2. @:useAstSource dot.path.To.field: This inserts the @:astSource expression of To.field in its place. Additionally, if @:coreApi is added (@:useAstSource @:coreApi dot.path.To.field) then the core API class field is used instead.

With this we can express @nadako"s example like so:

@:coreApi
class Input {
    public function readByte():Int {
        return 0;
    }

    public function readInt8():Int @:useAstSource @:coreApi haxe.io.Input.readInt8;
}

Obviously this could be abbreviated further to make it more elegant.

@nadako
Copy link
Member Author

nadako commented Nov 24, 2015

I wonder to what extent this is actually useful, since we still have to manually provide everything that is required for resolving identifiers in copied untyped AST (imports/usings/fields/etc), I"m a bit concerned that this is too error-prone to be a proper language feature.

@Simn
Copy link
Member

Simn commented Nov 24, 2015

Users certainly have to be aware, but then the same is true for macros that return expressions that are retrieved from external sources.

@ncannasse
Copy link
Member

I don"t like it that much either, that requires to be able to know in advance which AST will have to be reused, which breaks the separation of implementation between the base api and the platform-specific one.

@Simn
Copy link
Member

Simn commented Nov 25, 2015

You suggested exactly that and I"ve implemented it accordingly.

@ncannasse
Copy link
Member

My suggestion was to have this done automatically, maybe it"s the case?

@Simn
Copy link
Member

Simn commented Nov 25, 2015

Not yet, but to do "something" automatically you have to implement "something" first. Also I"m not sure how to approach this, i.e. how to detect which classes are core classes. This is especially true for your ideas in #4066.

Simn added a commit that referenced this issue Nov 29, 2015
This is related to #3285, but it"s also useful in its own right for macro users.
@ncannasse
Copy link
Member

look at Typeload.load_core_class: we"re loading core classes in a separate core context, which have -D coreApi defined

@Simn
Copy link
Member

Simn commented Dec 1, 2015

Yes I figured that out after mentioning it and added it accordingly. I"m a bit concerned that it"s for the most part a pointless waste of memory, but then again it"s hardly going to make any difference in the big picture.

@Simn Simn modified the milestones: 3.3.0-rc1, 3.4 Feb 23, 2016
@Simn Simn modified the milestones: 3.4, 4.0 Jan 9, 2017
@Simn Simn modified the milestones: Release 4.0, Design Apr 17, 2018
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

3 participants