Skip to content

Tackle the case of === #1111

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

Closed
wants to merge 10 commits into from
Closed

Tackle the case of === #1111

wants to merge 10 commits into from

Conversation

dcodeIO
Copy link
Member

@dcodeIO dcodeIO commented Feb 14, 2020

This is an experiment of what has to be done to make === not-so-special anymore, essentially degrading it to an alias of == with no special semantics. While we'll lose the ability to do safe identity comparisons, feedback has shown that the special semantics we had compared to TS are hard to justify.

  • Replaces all occurrences of === and !== in the standard library with pointer ops
  • Unifies String overloads to always accept string | null
  • Removes unnecessary null checks in String methods if the method does not accept null anyway
  • Fixes highlighting problems in the std/array test

Would be a breaking change for users depending on the old semantics.

Comment on lines 1567 to 1569
i32.const 160
i32.const 160
call $~lib/string/String.__eq
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. Seems this force full string equal

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the downside is that we cannot precompute === string comparisons anymore. Previously, === compiled to just an i32.eq due to the differing semantics, which is trivial to precompute, while it now calls the overload.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Hope increase limits for inlining helps with this

Copy link
Member Author

@dcodeIO dcodeIO Feb 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes me wonder if we can handle strings differently, by doing something like i32.eq(lhs, rhs) | String.__eq(lhs, rhs) where Binaryen can see that this is trueish and potentially eliminate the overload call. But looks like temp locals will become a problem there.

Copy link
Member

@MaxGraey MaxGraey Feb 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I prefer use overloading for string operators. It simplify compiler and the same time show more explicitly what's doing on. Also it allow inherit String class and overload any operator which also pretty flexible.

I guess tuning inline params need anyway and this will helps with this. Also it could be done earlier in MIR part in future as part of StdLibSimplify pass

Comment on lines -475 to -490

if (ASC_SHRINK_LEVEL < 1) {
if (isString<T>()) return joinStringArray(dataStart, length, separator);
}
// For rest objects and arrays use general join routine
Copy link
Member

@MaxGraey MaxGraey Feb 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this conditional guard potentially decreased code size so recommend leave as is

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, not sure about this one as well. Removed it because joinReferenceArray wasn't compatible with strings anymore, but there's probably something better we can do.

@petersalomonsen
Copy link
Contributor

hmm.. if going this way, wouldn't this mean that we'd also have to do type conversion for == so that 10=='10' would be true?

Sure we are getting more like typescript/javascript this way, but do we really want/need to go there?

I'm not sure. It's good that AssemblyScript is familiar for JS/TS developers, on the other hand it's good that there are some differences that preserves the "low level mindset".

@MaxGraey
Copy link
Member

@petersalomonsen No, it's about remove special meaning for === and !== in AssemblyScript which used as raw pointer comparison but for current PR this behaviour will be same as == and !==

@petersalomonsen
Copy link
Contributor

yes @MaxGraey - my point was just that if going in that direction, wouldn't the next expectation (demand from users) be that we do type conversion for '10'==10 ?

=== as a pointer comparison isn't that "controversial" in a WebAssembly context.

@MaxGraey
Copy link
Member

MaxGraey commented Feb 15, 2020

Typescript also restrict "10" == 10 due to TS2367 and even if not AS try to avoid implicit conversion as much as possible anyway. Even for this (10 as i32) >= (10 as u32) will be compile error with Operator '>=' cannot be applied to types 'i32' and 'u32'.

@petersalomonsen
Copy link
Contributor

ahh good point :-) very well then 👍 thanks for clarifying :-)

@dcodeIO
Copy link
Member Author

dcodeIO commented Mar 14, 2020

Appears this breaks null checking because, when compiling to Wasm, the compiler can't predict how every single overload turns out, affecting code like

if (arg0 !== null) message = message.replace("{0}", arg0);

where arg0 !== null compiles to a function call in Wasm, calling String.__eq. This is correct so far in that an overload can do whatever it wants, including returning true if any argument is null, so there is no way for the compiler to know except by hardcoding certain overloads.

One solution would be to add special cases for if any argument is a literal null, and make that circumvent operator overloads, but of course that'd also mean that such a comparison cannot be overloaded. Seems slightly less inconsistent than before however. Not sure.

@MaxGraey
Copy link
Member

MaxGraey commented Mar 14, 2020

I guess all this could be solved using some top reference type like object.
<object>(ref) == null much better than changetype<usize>(ref) == 0

@dcodeIO
Copy link
Member Author

dcodeIO commented Mar 14, 2020

<object>(ref) == null

Seems to conflict with our goal to make stuff properly virtual eventually. In such a case the code would still virtually call the top-most overload. Related: #1152

@dcodeIO
Copy link
Member Author

dcodeIO commented Mar 14, 2020

Multiple solutions:

  1. Internalize string overloads, so it works for strings but remains a problem for any other class using a == overload
  2. Internalize everything, completely removing the ability to declare custom overloads for the sake of providing something reliable
  3. Keep === as is, which doesn't appear so bad anymore on this background as well

Preferring either 2 or 3.

@MaxGraey
Copy link
Member

MaxGraey commented Mar 14, 2020

In C# for example operators cannot be virtual. In Java you could overload only '+' and it can't be also virtual. So I suggest also restrict polymorphism for overloaded operators because it produce many problems and performance issues

@dcodeIO
Copy link
Member Author

dcodeIO commented Mar 14, 2020

  1. would mean that we get rid of the @operator decorator completely and make those overloads we need runtime functions that the compiler maps to certain types like String, Array, the typed arrays, StaticArray, maybe Pointer and whatnot. This has always been on my mind simply because the concept doesn't play well with TypeScript anyway, and there hasn't been a particularly compelling use case for operator overloads outside of stdlib anyway so far.

@MaxGraey
Copy link
Member

In C# for example operators cannot be virtual. In Java you could overload only '+' and it can't be also virtual. So I suggest also restrict polymorphism for overloaded operators because it produce many problems and performance issues

In this case <object>(ref) == null will solve most of edge cases I hope

@MaxGraey
Copy link
Member

Internalize everything, completely removing the ability to declare custom overloads for the sake of providing something reliable

Is not a good solution due to we already have some codebases which based on this feature. Also it quite nice feature and JavaScript try to find way to it

@MaxGraey
Copy link
Member

Basically all languages have some sort of operator overloading except of JavaScript =)

@dcodeIO
Copy link
Member Author

dcodeIO commented Mar 15, 2020

Last commit addresses these issues by inlining part of the ==, != and ! overloads so the compiler can still precompute relevant cases, at the expense of increased code size. Is quite dramatic in untouched output, but not so significant anymore once optimized. About 1.000 additional lines (now 220.500) on the compiler itself.

Also means that anyone implementing these overloads must do the same if null checking is relevant to them. Upside is that it's all still possible, downside is that it requires users to understand the ins and outs of those specific overloads.

@dcodeIO
Copy link
Member Author

dcodeIO commented Mar 15, 2020

An alternative implementation might be to internalize the null special cases so overloads don't have to deal with them, but also cannot receive nulls anymore. Opinions?

@MaxGraey
Copy link
Member

Hmm. May be better inline __eq instead __eqi?

@dcodeIO
Copy link
Member Author

dcodeIO commented Mar 15, 2020

__eqi is the inlined part relevant to precompute, while __eq covers those cases we don't precompute anyway. I believe the confusion in the wats comes from Binaryen deciding to inline both and then picking the other label name, or something like that.

@dcodeIO
Copy link
Member Author

dcodeIO commented Mar 16, 2020

This now should bring us closer to efficient code in that the == and != overloads implicitly handle null as detailed above, making null checking non-overloadable. There's more that can be done here, by applying some precompute as well to detect something that is already known to be null, not just nullable, in the best case leading to full static precompute of expressions like if ("a" == null) -> i32.const(0)

One side effect is that we can't allow top-level

var someGlobal: string;

without an initializer anymore because flows can't track when the global is becoming initialized. Hence, reference-typed globals now require either an initializer or a nullable type. Example:

var someGlobal: string;

function a(): void {
  someGlobal = "123";
}
function b(): void {
  someGlobal.length; // not known to b's flow
}
a(); // commenting this out will break the runtime
b();

@MaxGraey
Copy link
Member

MaxGraey commented Mar 16, 2020

var someGlobal: string;

function a(): void {
  someGlobal = "123";
}
function b(): void {
  someGlobal.length; // not known to b's flow
}
a(); // commenting this out will break the runtime
b();
 // commenting this out will break the runtime

Basically it's normal and expected behaviour but will be great handle (emit error) about uninitialised global in compile time ofc

@dcodeIO
Copy link
Member Author

dcodeIO commented Apr 20, 2020

Quick heads up: WebAssembly/binaryen#2702 has just been merged

@jtenner
Copy link
Contributor

jtenner commented Apr 20, 2020

So are we switching === to become a glorified == now? 😅

I can't wait to see if as-pect breaks now.

@MaxGraey
Copy link
Member

btw some people uses === especially for generics like:

class SomeContainer<K, V> {
   find(key: K): V {
      if (this.root.key === key) {
        ...
      }
   }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants