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

Something is wrong with @:op(A.B) #8572

Closed
Ilir-Liburn opened this issue Jul 21, 2019 · 13 comments
Closed

Something is wrong with @:op(A.B) #8572

Ilir-Liburn opened this issue Jul 21, 2019 · 13 comments

Comments

@Ilir-Liburn
Copy link

abstract Abstract({value:Dynamic, type:ValueType}) {
    @:from public static inline function from(v) return cast v;
    @:op(A.B) public static inline function resolve(self, key, val:Dynamic):Dynamic {
        return key == "value"? self.value = val : self.type = val; 
    }
}
        var a:Abstract = { value:0, type:TInt };
        a.value = 10;
        a.type = TInt;

Unlike any other operator, @:op(A.B) is forcing to declare additional parameter for setter or getter if function is declared as static. This parameter (self) is abstract itself and if used like above, it is causing recursion without warning:

		public static object resolve(object self, string key, object val) {
			if (( key == "value" )) {
				return global::_Main.Abstract_Impl_.resolve(self, "value", val);
			}
			else {
				return global::_Main.Abstract_Impl_.resolve(self, "type", val);
			}
		}

Or it's a bug, or it needs more work to use it safely. BTW, @:op(A.B) can also be declared as macro (doing nothing), so it should be fixed like it was fixed in case of the constructor.

@Ilir-Liburn
Copy link
Author

Ilir-Liburn commented Jul 21, 2019

It can get even worse

abstract Abstract({value:Dynamic, type:ValueType}) {
    var value(get, set):Dynamic;
    inline function get_value() return this.value;
    inline function set_value(v) return this.value = v;
    var type(get, set):ValueType;
    inline function get_type() return this.type;
    inline function set_type(v) return this.type = v;

    @:from public static inline function from(v) return cast v;

    @:op(A.B) public static inline function resolve(self, key, val:Dynamic):Dynamic {
        return key == "value"? self.value = val : self.type = val; 
    }
}

Idea to make individual private getters and setters over underlying type and call them in @:op(A.B) failed because @:op(A.B) doesn't have precedence over them. BTW, making them public inline turns underlying type into dynamic object. Inline getters and setters should keep underlying type inline, hoping @:op(A.B) could do the same

@Ilir-Liburn
Copy link
Author

Inline is actually not working for @:op(A.B) in this case

abstract Abstract(Dynamic) {
    public inline function new(v) { this = v; }
    @:from public static inline function from(v) { return new Abstract(v); }
    @:op(A.B) public inline function resolve(key, val:Dynamic) {
        return key == "value" ? this.value = val : this.type = val;
    }
        var a = new Abstract({ value:(null:Dynamic), type:TUnknown }); // this one doesn't inline
//        var a = { value:(null:Dynamic), type:TUnknown }; // this one does
        a.value = 10.0;
        a.type = TFloat;
}

@Ilir-Liburn
Copy link
Author

Ilir-Liburn commented Jul 23, 2019

But it's working for both new and @:from in this case

abstract Abstract<T>(T) {
    public inline function new(v:T) { this = v; }
    @:from public static inline function from(v) { return new Abstract(v); }
    @:op(A.B) public inline function resolve(key, val:Dynamic):Dynamic {
        return untyped key == "value" ? this.value = val : this.type = val;
    }
}

        var a = new Abstract({ value:(null:Dynamic), type:TUnknown }); // inline
//        var a:Abstract<Dynamic> = { value:(null:Dynamic), type:TUnknown }; inline
        a.value = 10.0;
        a.type = TFloat;

Cool. It would be even better if inline could work without T and untyped

@RealyUniqueName
Copy link
Member

I'm not sure if @:op(A.B) is supposed to work as a static method.
@ncannasse ?

@RealyUniqueName
Copy link
Member

Oh, actually in your original sample you get a recursion because your abstract doesn't have type and value fields, so they go to @:op(A.B) too. That seems to be working as designed. Not sure what to fix here.

@Ilir-Liburn
Copy link
Author

Ilir-Liburn commented Jul 23, 2019

Are you sure? I had to do this to get everything working

abstract Under<T>(T) {
    public inline function new(v:T) { this = v; }

    @:from public static inline function from(v) { return new Under(v); }

    @:op(A.B) public static inline function write(self, key, val:Dynamic):Dynamic {
        var ths:{} = cast self;
        return untyped key == "value" ? ths.value = val : ths.type = val;
    }
}

@:forward
abstract Abstract(Under<Dynamic>)
{
    public inline function new(v) this = new Under(v);
    @:from public static inline function from(v) return new Abstract(v);
/*
    @:op(A.B) public static inline function write(self, key, val:Dynamic):Dynamic {
        var ths:Under<Dynamic> = self;
        return untyped key == "value" ? ths.value = val : ths.type = val;
    }
*/
}

//        var a = new Abstract({ value:(null:Dynamic), type:TUnknown });
        var a:Abstract = { value:(null:Dynamic), type:TUnknown };
        a.value = 10.0;
        a.type = TFloat;

Note need to cast self to underlying type

@RealyUniqueName
Copy link
Member

Yes, I am sure. Your self is typed as Under, which doesn't have value and type fields. That's why compiler generates write calls for self.value = and self.type =.

@Ilir-Liburn
Copy link
Author

Ilir-Liburn commented Jul 25, 2019

OK, fine with me. Except self is useless if setter of value/type is defined. Because @:op(A.B) doesn't have priority over individual setter. It only works for reflection, or manual call

        var u1 = new Under({ value:(null:Dynamic), type:TUnknown });
        Under.write(u1, "value", 10.0);//u1.value = 10.0;
        Under.write(u1, "type", TInt);//u1.type = TInt;

Even if setter is defined as private, @:op(A.B) will not take priority. Put that into documentation, and case is closed.

@RealyUniqueName
Copy link
Member

Sorry, I don't understand you :)
@:op(A.B) doesn't need a priority. It's used if specified field is not available.

@Ilir-Liburn
Copy link
Author

And what if setter is private? Should compiler delegate to @:op(A.B)? If not, just mention that @:op(A.B) is always about resolve of unexisting fields into documentation.

@Simn
Copy link
Member

Simn commented Jul 30, 2019

I'm not sure anymore if introducing all these pseudo-@:op functions was a good idea, mainly because it's difficult to search for it. The documentation for @:resolve actually mentions this already:

Abstract fields marked with this metadata can be used to resolve unknown fields.

I suppose we need a proper section in the manual about @:op now.

@Aurel300
Copy link
Member

@Simn What's missing in https://haxe.org/manual/types-abstract-operator-overloading.html ? Could you open a HaxeManual issue with some bullet points that you can think of?

@Simn
Copy link
Member

Simn commented Jul 30, 2019

@Simn Simn closed this as completed Jul 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants