-
-
Notifications
You must be signed in to change notification settings - Fork 648
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
BREAKING CHANGE: Improve null comparison #2760
base: main
Are you sure you want to change the base?
Conversation
eca239f
to
7457a31
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The big question is in my last comment.
export function compare_null_with_operator_overload(): void { | ||
null == "aa"; | ||
null != "aa"; | ||
"aa" == null; | ||
"aa" != null; | ||
|
||
null === "aa"; | ||
null !== "aa"; | ||
"aa" === null; | ||
"aa" !== null; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something to be very careful with. Your current code is perfectly fine for String, but I have to wonder what happens with classes with operator overloads that have signatures with non-null parameters. In that case, you would need to add some null-checking code for ==
that's equivalent to a && b ? eqOverload(a, b) : a == b
, where a
and b
are the pointers of the LHS and RHS respectively, and eqOverload
represents the overload function. Similar null-checking code applies to !=
. You could also opt to error if ==
/!=
is used on nullable types that have an overload without nullable parameters.
Whoops, sorry, I didn't realize you removed the request to review. |
I find it should handle float and integer promotion more carefully. |
BREAK_CHANGE Fixes AssemblyScript#2662
7457a31
to
321f9c7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't quite right either.
i32.const 0 | ||
i32.const 32 | ||
i32.eq | ||
drop | ||
i32.const 0 | ||
i32.const 32 | ||
i32.ne | ||
drop |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't give me a good feeling, because this means null == overloaded
isn't being compiled correctly, while overloaded == null
is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I think it should not block this fix.
The same thing will happen when comparing new A() == new B()
We should introduce more strictly check for operator overload.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose so. I don't like having different semantics for null ==
vs == null
as well as for A ==
and == A
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #2761
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like having different semantics for
null ==
vs== null
as well as forA ==
and== A
.
Agree. This patch should land after we implement better operator overload resolution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
implement better operator overload resolution
We would still need to special-case null
somehow. There needs to be some way to differentiate between a value of type Object | null
and a null
literal, which has the same type in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we need to differentiate between a value of type Object | null and a null literal? If we want to do it, it is necessary to introduce a new primitive type.
Fixes #2662.