[js] Type mismatch when using Bool default values

[jgranick]

Using recent sources of the Haxe compiler, there appear to be mismatches when using Bool default values.

For example, there is an interface that defines a function with an optional Bool parameter. There is a class that implements the interface, which also has the optional Bool parameter.

Something like this:

interface ITest extends IAnotherInterface {

   function hello (world:String, cool:Bool = true):ISomeOtherInterface;

}

class TestImpl implements ITest {

    ...

    public function hello (world:String, cool:Bool = true):ISomeOtherInterface {

        ...

    }

}

When compiled, the compiler says that the class' default Bool value is really Null<Bool>, while the interface remains Bool. The types are not the same, and it throws a compile error.

If this were the real code, it would throw an error that String->Null<Bool>->ISomeOtherInterface should be String->Bool->ISomeOtherInterface and that it overrides the parent class (yes it uses the words "parent class") with the wrong type.

Interestingly (and lucky for me!) this does not occur if all parameters have a question mark. ?myValue:Bool = true is consistently Bool in all cases. It's using only myValue:Bool = true (which the code all originally uses) that results in the Null<Bool> issues.

Magically, this works:

interface ITest extends IAnotherInterface {

   function hello (world:String, ?cool:Bool = true):ISomeOtherInterface;

}

class TestImpl implements ITest {

    ...

    public function hello (world:String, ?cool:Bool = true):ISomeOtherInterface {

        ...

    }

}

I don't know if this would consistently happen all the time, or it's some combination of interfaces extending interfaces being implemented on classes extended by other classes. I don't know, but I wonder if the question mark exception here is intentional, or is how this issue was undetected (as I think all the Haxe test code uses question marks for optional values?)

The question marks should be optional, I am thankful for a workaround, but maybe you guys have an idea of the root cause? I believe it only affects 3.3 dev builds

[Simn]

I cannot reproduce the problem with this:

interface ISomeOtherInterface { }
interface IAnotherInterface { }

interface ITest extends IAnotherInterface {
   function hello (world:String, cool:Bool = true):ISomeOtherInterface;
}

class TestImpl implements ITest {
    public function hello (world:String, cool:Bool = true):ISomeOtherInterface {
        return null;
    }
}

Any more hints? Does it only happen on a specific target?

[Gama11]

Does it only happen on a specific target?

The issue title says "js".

[Simn]

Oh right! But I can't reproduce it on JS either.

[jgranick]

It's part of a very complex codebase. It's something like:

interface IModel {
    public function foo (a:Int, b:String):Dynamic;
    public function foo2 (a:Bool, b:Int):Void;
}

interface IListModel extends IModel {
    public function foo3 (a:Float, b:Bool = true):IOtherModel;
}

class ListModelImpl implements IListModel {
   ...
}

class OtherListModelImpl extends ListModelImpl implements YetAnotherModel {
   ...
}

I have a sinking feeling it may be hard to reproduce outside the original codebase, which I can't share (unfortunately). I had a feeling that the JS-specific nature and "question mark fixes the issue" characteristics might lend some clues to where or what in the code leads to this.

On a dynamic target, I assume Null would most of the time fly invisibly under the radar, but since it needs to subscribe to an interface, that's where trouble comes. Even more curiously, on the "ListModelImpl" class, it even says that "notifyListModel" overrides the parent class with a bad type. It has no parent type, other than an interface, and the interface has no such function (though it has a few that start with "notify"), so it's all very curious.

Layer upon layer upon layer

[Simn]

Well, ? does imply nullability so in that case you end up with a Null<Bool> type. But yes, I have no idea why a Null<T> T unification would ever fail, dynamic target or not.

Maybe a bisect is necessary.

[Simn]

Actually that's not true, Null<T> T is checked for redefinitions here: https://github.com/HaxeFoundation/haxe/blob/development/typeload.ml#L706

I have made a change to this line recently: 4557c34

However, that should only affect things if Dynamic is involved. Can you try if reverting this change removes the error on your end?

[jgranick]

The version of Haxe used was compiled from this source version:

f7fa452

This appears to be two days before the Dynamic change you made.

I hoped this commit would make a difference:

d86d452

...but I updated to the latest source from yesterday, same problem 😢

I believe the issue is still type mismatches (such as with an interface), not issues with passing parameters, though it is strange that errors occurred for at least one field that was not part of an interface or a parent class.

Although the error occurs when using interfaces, its interesting that error occurs on the "override" code path, not the interface code path:

https://github.com/HaxeFoundation/haxe/blob/development/typeload.ml#L889-L891

[jgranick]

I just got the same error again, let me share the error output:

path/to/Container.hx:120: lines 120-140 : Field setPrincipalComponentAndWalk overloads parent class with different or incomplete type
path/to/ActionOverlayView.hx:29: lines 29-117 : Defined in this class
path/to/Container.hx:120: lines 120-140 : child : com.framework.IFocusableComponent -> ?stopAtLayer : Null<Bool> -> Void should be child : com.framework.IFocusableComponent -> ?stopAtLayer : Bool -> Void
path/to/Container.hx:120: lines 120-140 : Null<Bool> should be Bool
path/to/ActionOverlayView.hx:29: lines 29-117 : Defined in this class

The relevant function looks like this:

private function setPrincipalComponentAndWalk(child : IFocusableComponent, stopAtLayer : Bool = false) : Void { ... }

This occurs a few times, for basically any value:Bool = true parameter in the functions on these classes. I think it is related (somehow?) to these classes implementing interfaces, though not extending any base class. It is curious.

[Simn]

Are you sure you don't have value:Bool = null anywhere? This would infer the type as Null<Bool> and cause the observed error.

[jgranick]

I just hit an area of code that got upset about openfl.events.IEventDispatcher, same problem. To fix the problem (temporarily), I added question marks to the IEventDispatcher interface. This now causes openfl.events.EventDispatcher to fail to compile. Perhaps, this at its core, is the root issue.

This original code works outside the project:

interface IEventDispatcher {

    public function addEventListener (type:String, listener:Dynamic->Void, useCapture:Bool = false, priority:Int = 0, useWeakReference:Bool = false):Void;
    public function removeEventListener (type:String, listener:Dynamic->Void, useCapture:Bool = false):Void;
    ...

}

class EventDispatcher implements IEventDispatcher {

    public function addEventListener (type:String, listener:Dynamic->Void, useCapture:Bool = false, priority:Int = 0, useWeakReference:Bool = false):Void { ... }
    public function removeEventListener (type:String, listener:Dynamic->Void, useCapture:Bool = false):Void { ... }
    ...

}

This modified code fails to compile... when I changed the interface to try and fix the other project, it broke EventDispatcher. The following does not unify:

interface IEventDispatcher {

    public function addEventListener (type:String, listener:Dynamic->Void, ?useCapture:Bool = false, ?priority:Int = 0, ?useWeakReference:Bool = false):Void;
    public function removeEventListener (type:String, listener:Dynamic->Void, ?useCapture:Bool = false):Void;
    ...

}

class EventDispatcher implements IEventDispatcher {

    public function addEventListener (type:String, listener:Dynamic->Void, useCapture:Bool = false, priority:Int = 0, useWeakReference:Bool = false):Void { ... }
    public function removeEventListener (type:String, listener:Dynamic->Void, useCapture:Bool = false):Void { ... }
    ...

}

I always think of the question mark as being superfluous syntax when assigning a default value. It actually, in the current compiler and the JS target, results in Bool versus Null<Bool> which results in compile errors.

Do you think that we should treat all value:Bool = true parameters as a Null<Bool>, similar to using a question mark, or do you think that (on the reverse) ?value:Bool should be treated as value:Bool = false and not type as Null<Bool>?

If both are treated consistently, it may solve our problem. C++ (among other targets) definitely work with the code, so it seems related either to being a dynamic target, or maybe the JS code compilation process specifically.

Thank you!

[Simn]

As I said ? implies nullability. I have argued before that ?name:Bool should be a compiler error because it becomes ?name:Null<Bool> which effectively ignores the explicit type-hint. The same is true for name:Bool = null.

Let's try to get an overview:

argument	nullable	optional
?name	true	true
name = null	true	true
name = value	false	true
name:Null<T>	true	false
name:Null<T> = value	true	true

[jgranick]

Hmm, I don't think there is an = null in the code, that I could tell. Something about the layers of complexity is typing it to Null<Bool> at some point, which causes a reaction of mismatching types.

It is still curious to me that other platforms (notably C++) are not having compile errors, I wonder if the type is inferred to Null<Bool> at some point due to convenience, or expecting that omitting the default value on JavaScript would end up being null at some point.

The way it reacts about functions on a class with no parent type, unrelated to interfaces used, responding that it overrides with the wrong type, almost suggests to me that one pass (macro pass?) is typing something as Null<Bool>, then the next pass properly types as Bool and gets upset, or vice versa, on the same method.

I assumed that ? on Bool values implied false, and on Int or Float implied 0 as the default value, but it makes sense to have it consistent. It isn't originally used in this code, I've been applying it as a workaround to this strange problem

[jgranick]

The real issue of grief comes from Null<Bool> should be Bool errors. When looking at the code, it clearly states name:Bool = true.

"It already is Bool, not Null, what am I supposed to do?!" (╯°□°）╯︵ ┻━┻)

If you would like to tighten things up so the error occurs elsewhere (not allowing value:Bool = null ... though I'm not sure that's the true cause in the code I'm testing) I don't mind, or if Null<T> and T can unify on dynamic targets, perhaps it should not throw the type errors during compilation and be more lenient.

If ? remains nullable by default, I would suggest that it does not for explicit types, like this:

argument	nullable	optional
?name	true	true
name = null	true	true
name = value	false	true
?name = value	true	true
?name:T = value	false	true
name:Null<T>	true	false
?name:Null<T>	true	true
name:Null<T> = value	true	true
?name:Null<T> = value	true	true

I've always thought of ? as being similar to public and private in classes and enums, they are implied by default, but you can specific about it if you prefer to be verbose or clear. The key difference would be that ?name:T = value would not be nullable, as you suggested you argued for.

HOWEVER -- that won't fix the problem I'm having, it would remove my workaround to get the code to compile on the JavaScript target, and instead of adding ? I would have to also change all the Bool types to Null<Bool> explicitly, which would be good if that's what I wanted, but I'd prefer these to just be regular Bool as they were in the original project 😄

[Simn]

I have improved error reporting, it now reports the position of the base/interface field as well like so:

Main.hx:5: lines 5-8 : Field v has different type than in I
Main.hx:2: characters 8-28 : Interface field is defined here
Main.hx:5: lines 5-8 : String -> Void should be Dynamic -> Void
Main.hx:5: lines 5-8 : String should be Dynamic

This tells you exactly which two field the compiler "compares". Please paste the signatures of both fields so we can be sure they are indeed equal.

[ncannasse]

I agree we should forbid value:Bool = null when no ? is set. This could be checked post compilation the way we already have "null can't be Bool on static platform"

[jgranick]

I also suggest ?value:Bool = true should act the same as value:Bool = true, even though ? usually suggests nullability, I think converting Bool to Null<Bool> due to ? is confusing.

I'm trying to test with the latest source, thanks @Simn

[Simn]

The only nice thing about ? is that it's consistent, so let's please not change that. ?value:Bool is a contradiction in my opinion.

[jgranick]

The new changes you made revealed one error like this:

interface TestInterface {
    function test (a:Dynamic):Void;
}

class Test implements TestInterface {
    function test (a:CustomType):Void;
}

This used to compile, but looks fishy. I'll keep running the tests to see what else comes up

[Simn]

Yes we no longer allow this, it violates variance. See #4378.

Reminds me to update the changelog.

[jgranick]

I am still working through it, but I wanted to mention that I got this output in an error I was working on:

type : String -> listener : (Dynamic -> Void) -> ?useCapture : Null<Bool> -> Void should be type : String -> listener : (Dynamic -> Void) -> ?useCapture : Bool -> Void

It says ?useCapture : Null<Bool> should be ?useCapture : Bool. You see why I think making ?value:Bool = true change the type to Null<Bool> behind the scenes is confusing as a user? 😄

Anyway, I'll report more findings soon

[Simn]

This is probably related to type printing only. Internally our function type arguments is represented as (name, optional, type). For both ?name and name = value we get optional = true, but the default value itself is not part of the type. Thus when printing them both become ?name to identify that they are indeed optional.

That's a discrepancy to our syntax though, so I suppose we should address it. I guess we can check if the argument type is nullable and only print the ? if it is.

[Gama11]

Related: #2561

[Simn]

Actually this is in favor of Joshua's idea because ?name:Bool and ?name:Null<Bool> would be printed correctly.

[jgranick]

Alright, here's what I get:

path/to/Component.hx:502: lines 502-505 : Field addEventListener overloads parent class with different or incomplete type
path/to/ScrollableContainer.hx:22: lines 22-293 : Defined in this class
path/to/Component.hx:502: lines 502-505 : Base field is defined here
path/to/ScrollableContainer.hx:22: lines 22-293 : Defined in this class
path/to/Component.hx:502: lines 502-505 : type : String -> listener : (Dynamic -> Void) -> ?useCapture : Null<Bool> -> ?priority : Null<Int> -> ?useWeakReference : Null<Bool> -> Void should be type : String -> listener : (Dynamic -> Void) -> ?useCapture : Bool -> ?priority : Int -> ?useWeakReference : Bool -> Void
path/to/Component.hx:502: lines 502-505 : Null<Bool> should be Bool
path/to/ScrollableContainer.hx:22: lines 22-293 : Defined in this class

Here's an overview of what the code looks like, relevant to the error:

interface IEventDispatcher {

    public function addEventListener (type:String, listener:Dynamic->Void, useCapture:Bool = false, priority:Int = 0, useWeakReference:Bool = false):Void;
    ...

}

interface IInvalidating {
        ...
}

interface IValidating {
        ...
}

interface IComponent extends IEventDispatcher extends IInvalidating extends IValidating {
        ...
}

class Component implements IComponent {

        public function addEventListener(type : String, listener : Dynamic -> Void, useCapture : Bool = false, priority : Int = 0, useWeakReference : Bool = false) : Void { ... }
        ...

}

interface IScrollableContainer extends IContainer {
        ...
}

class ScrollableContainer extends Container implements IScrollableContainer {

        ...

}

(the ... suggest where there's more code that is unrelated)

[Simn]

You keep mentioning interfaces, but this error clearly comes from an override. It looks very strange because the error positions suggest that the field is checking overrides against itself. At this point I wonder if there are any build macros involved? The only conceivable situation where something like this could happen is if a build macro makes a "copy" of a field and uses that fields' position.

[jgranick]

You are right, it is funny how it behaves as if it has overridden itself. I had looked for signs of munit doing something, or if mcover was enabled, but it appears to be caused by a library called mockatoo. Removing this haxelib appears to resolve these errors.

https://github.com/misprintt/mockatoo/blob/master/src/mockatoo/macro/MockMaker.hx

I have been looking at the code to see if there is something that would convert Bool to Null<Bool> on the JS target, I wonder if it something to do with how types are represented in the macro context for JavaScript that somehow changes the fields? I mostly see type.toComplexType (and related) when it comes to member methods

EDIT: Actually, I'm not sure yet if that solves the issue, or just creates different compile errors that mask it. Trying to figure it out 😓

[Simn]

I found the culprit, see linked issue. I'm still not sure why this behavior changed though. The is_null check has been in 3.2.1 as well: https://github.com/HaxeFoundation/haxe/blob/3.2.1/typeload.ml#L706.

Either way, this seems to be working as (currently) intended on our end. I'll keep it open to deal with name:Bool = null.

[Simn]

Mockatoo has the exact same problem we have: When translating back to syntax they can't print ? for any optional argument because it could come from name:Bool = true. Inferring the ? state requires checking if the type is nullable as well, which is quite awkward. The more I think about it the more I am in favor of ? not implying nullability if an explicit type-hint exists.

[ncannasse]

I don't think we can make such change in 3.x, we can forbid some invalid cases if we decide to do so, but not silently change the semantics of ?

Just a recap : the difference between ? and = was introduced because some platforms had their own native way to have not nullable optional parameters, and it was necessary to express this behavior to correctly declare such externs and be able to override native classes.

[Simn]

I think I agree that we shouldn't change this at the moment. I'm struggling to see why we should disallow name:Bool = null but not ?name:Bool though. Shouldn't that be exactly the same, i.e. invalid?

[jgranick]

I never knew that ? implied nullability, in all my years as a developer.

I always believed it was shorthand, or an extra way of being verbose, like:

class MyClass {
    ...
    function privateFunction () {}
    private function privateFunction2 ():Void {}
}

Both private and Void are implied in privateFunction and explicit in privateFunction2.

I always thought ?value acted the same as ?value:Bool = false (for a detected Bool value) but is shorthand/being extra verbose. Guess not 😉

When did ? become strict nullability, from day one, or was there a specific release (Haxe 3.0, 3.1?) that introduced this behavior? Should it be allowed to unify on dynamic targets? I always thought that Null<T> was our only way of adding nullability to non-nullable types. I guess that's what's going on under the hood, but transforming T to Null<T> seems a little too magical to me 😄

[Simn]

As far as I'm aware ? has always implied nullability.

Regarding unification on Dynamic targets, maybe we could indeed skip the check, but I'm not sure. It's probably not good for self-documentation if an overriding field has a different signature, so I'm not quite sure what we would gain by allowing this.

[ncannasse]

@Simn I agree both are equivalent, but I think there's a lot of ?a:Bool occurrences out there, much more than a:Bool = null. Also, I would indeed like to keep ?a:Bool in 4.x

[Simn]

I really think we should do something about the fact that we can't print our own function type properly for Haxe 4.

[jgranick]

Mockatoo would iterate over TFun args in a Haxe macro. Since there are no default values on the args, it would loop and assign its own default values, based on type:

https://github.com/misprintt/mockatoo/blob/master/src/mockatoo/macro/tool/ComplexTypes.hx#L34-L68

Static platforms received 0 or false for Int, Float or Bool default values, but dynamic platforms were using null as the default value.

This forced type Bool to be treated as type Null<Bool> within the compiler, resulting in the very confusing override error.

misprintt/mockatoo#51

[jgranick]

Does arg.opt = true imply Null<T> when using macros to create function arguments?

Would it be { t: macro :Bool, value: macro true, opt: false, name: "value" } for value:Bool = true and { t: macro :Bool, value: macro true, opt: true, name: "value" } for ?value:Bool = true?

[Simn]

arg.opt = true at syntax-level means ?, which implies nullability again. Your two examples look correct.

[jgranick]

I will leave it to you all to decide if any changes are made to the compile behavior.

IMHO, var value:Bool = null; should not be allowed.

This is invalid syntax:

var value = null;
if (value) {}

If you cannot use null as if it were a Bool in conditional code, then it seems logical that you should not be able to assign a null value to a standard Bool type, right?

On a different subject, as Nicolas mentions, I do believe there a lot of ?a:Bool instances in code. However, outside of extern classes, I assume most function code works like this:

function test (?a:Bool) {

    return a ? true : false;

}

There is no null check in the above sample, the code expects it to be a Bool value.

I do see exceptions to this, but 100% they seem to be related to Neko only, as Neko uses null for all uninitialized values, regardless of whether they are optional or not.

#if neko
if (a == null) a = 0;
#end

Changing the behavior of ? to not change Bool into Null<Bool> seems more sensible to me, but I'll leave it all in your hands 😄

[mariommoreno]

I have the same (or similar) problem with default Bools for Haxe/JS.
Here an easy example to reproduce it:

class Data {
    public var boolValue:Bool;
    public function new() {}
}
class DataWithDefaul {
    public var boolValue:Bool = false;
    public function new() {}
}

class ParserTest
{
    public function new() 
    {
        var data:Data = new Data();
        trace("data.boolValue is bool",Std.is(data.boolValue, Bool));

        var dataDef:DataWithDefaul = new DataWithDefaul();
        trace("dataDef.boolValue is bool",Std.is(dataDef.boolValue, Bool));
    }

}

The Output is:

data.boolValue is bool,false
dataDef.boolValue is bool,true

Bool is supposed to be false as default, but it is null and it isn't even Bool at all (only for compile proposes).

[Simn]

That's how it works on dynamic targets.

[Simn]

Reading through this issue again I think we can close it because the original issue is resolved. Changing the way ? works is material for #4684.