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

Some operator tests related to Null<T> #8426

Closed
wants to merge 4 commits into from

Conversation

Simn
Copy link
Member

@Simn Simn commented Jun 14, 2019

I'm not 100% positive that all these conform to the spec, but I'd like to get an overview of where we're at.

Related: #8367

@Aurel300
Copy link
Member

Aurel300 commented Jun 14, 2019

f(...) neko eval hl java jvm cs cpp cppia flash9
0 >= nullInt . . F F . F . . .
0 < nullInt . . . . . . . . .
0 <= nullInt . . F F . F . . .
nullInt >= 0 . . F F . F . . .
nullInt <= 0 . . F F . F . . .
1 > nullInt . . F F . F . . .
1 >= nullInt . . F F . F . . .
1 < nullInt . . . . . . . . .
1 <= nullInt . . . . . . . . .
nullInt < 1 . . F F . F . . .
nullInt <= 1 . . F F . F . . .
-1 < nullInt . . F F . F . . .
-1 <= nullInt . . F F . F . . .
nullInt > -1 . . F F . F . . .
nullInt >= -1 . . F F . F . . .
0. >= nullFloat . . F F . F . . .
0. < nullFloat . . . . . . . . .
0. <= nullFloat . . F F . F . . .
nullFloat >= 0. . . F F . F . . .
nullFloat <= 0. . . F F . F . . F
1. > nullFloat . . F F . F . . F
1. >= nullFloat . . F F . F . . F
1. < nullFloat . . . . . . . . .
1. <= nullFloat . . . . . . . . .
nullFloat < 1. . . F F . F . . F
nullFloat <= 1. . . F F . F . . F
-1. < nullFloat . . F F . F . . F
-1. <= nullFloat . . F F . F . . F
nullFloat > -1. . . F F . F . . F
nullFloat >= -1. . . F F . F . . F

(F for failure)

Flash 9 on Linux shows at least the failures above without displaying the total count; on OS X it shows the total number of failures (24) without displaying the specific lines … It is probably the same as HL.

@Simn
Copy link
Member Author

Simn commented Jun 18, 2019

@hughsando Could you check cppia? It should align with cpp.

@Aurel300 Could you resolve the conflict?

@Simn
Copy link
Member Author

Simn commented Jun 19, 2019

cppia confirmed fixed, I've updated the table.

I think we should fix Java/C#/HL, but I'm not sure how easy this is to fix. I'm tentatively setting this to 4.0. I'll look into Java myself, but I'll need help with C# and HL (@kLabz and @ncannasse).

@Simn
Copy link
Member Author

Simn commented Jun 19, 2019

Actually pinging @nadako as well because the flash behavior might bite them.

@Simn
Copy link
Member Author

Simn commented Jun 19, 2019

@waneck Any pointers to where this is handled? The problem here is that when doing Null<Int> >= Int, genjava generates haxe.lang.Runtime.toInt(lhs) >= rhs which leads to a 0 >= 0 comparison. We shouldn't apply this cast for comparison operators.

@kLabz
Copy link
Contributor

kLabz commented Jun 19, 2019

@Simn it seems to be handled there: https://github.com/HaxeFoundation/haxe/blob/development/src/codegen/gencommon/hardNullableSynf.ml#L173

I get some results with a simple test, but break many things because it's just a naive one. I get:

testNadakoOps: FAILURE ....................F.....F...F.F.................F.....F...F.F...

instead of:

testNadakoOps: FAILURE .............F.F.F.FFF....FF..FFFF.........F.F.F.FFF....FF..FFFF..

@Simn
Copy link
Member Author

Simn commented Jun 19, 2019

For these cases we have to generate nullable == null ? false : unwrap(nullable) op otherValue

If both operands are nullable it has to be lhs == null ? rhs == null : rhs == null ? false : unwrap(lhs) op unwrap(rhs).

There might also be evaluation order concerns if the rhs is the nullable one...

@Simn Simn modified the milestones: Release 4.0, Release 4.1 Jul 1, 2019
@Simn
Copy link
Member Author

Simn commented Jul 1, 2019

I had a look like this but gencommon is too much for me to handle in this heat. Will maybe look at it again later...

@Simn Simn mentioned this pull request Jul 1, 2019
@RealyUniqueName
Copy link
Member

Maybe we should just unspecify >=, <=, < and > with null values.
Because our dynamic targets do this:

class Main {
	static var i:Int = null; // or `Null<Int>` no difference for dynamic targets

	public static function main():Void {
		i = null;
		trace(i == 0);
		trace(i < 0);
		trace(i > 0);
		trace(i >= 0);
		trace(i <= 0);
	}
}

python throws TypeError: '<' not supported between instances of 'NoneType' and 'int'
lua throws attempt to compare nil with number
php,js:

src/Main.hx:5: false
src/Main.hx:6: false
src/Main.hx:7: false
src/Main.hx:8: true
src/Main.hx:9: true

But if I comment out i = null; line in "main" function, then js emits this:

src/Main.hx:5: false
src/Main.hx:6: false
src/Main.hx:7: false
src/Main.hx:8: false
src/Main.hx:9: false

Also null safety emits a compile time errors for <,<=,>,=> ops with nullable values.

@ncannasse
Copy link
Member

I think we should keep the spec defined by the provided unit tests for comparisons - for static targets at least - since they will match most dynamic targets specification (js in particular).

@Simn
Copy link
Member Author

Simn commented Feb 17, 2020

I think we should keep the spec defined by the provided unit tests for comparisons - for static targets at least - since they will match most dynamic targets specification (js in particular).

I have some trouble parsing what you mean here exactly. So, what do you mean here exactly?

@Simn Simn modified the milestones: Backlog, Later Mar 24, 2023
@Simn Simn mentioned this pull request Nov 27, 2023
# Conflicts:
#	tests/runci/targets/Flash.hx
@Simn
Copy link
Member Author

Simn commented Feb 8, 2024

In 2024, HL still fails many of the comparison tests against null. The problem seems to come from the fact that any null in x > null is converted to 0, which I don't think is correct. I think this kind of conversion should only be done when assigning to a basic type storage.

@Simn
Copy link
Member Author

Simn commented Mar 20, 2024

Fixed by #11612

@Simn Simn closed this Mar 20, 2024
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.

7 participants