Use Object.create() instead of our own polyfill

[niloc132]

No description provided.

[niloc132]

Whoops, I had another change in this branch, but rolled it back because I thought it was just be being picky about code style, but its meaningful here.

[zbynek]

Do we need Object.create(superPrototype || null) to be compatible with the polyfill?

[niloc132]

It turns out the compiler tests at least assumes that we're not going to Object.create(null) for cases like this, the failing test this PR still has is for calling hasOwnProperty on a "java" object

gwt/user/test/com/google/gwt/core/interop/JsMethodTest.java

Lines 36 to 49 in 7b221c1

	class MyObject {
	@JsProperty
	public int mine;
	@JsMethod
	public native boolean hasOwnProperty(String name);
	}
	public void testNativeJsMethod() {
	MyObject obj = new MyObject();
	obj.mine = 0;
	assertTrue(obj.hasOwnProperty("mine"));
	assertFalse(obj.hasOwnProperty("toString"));
	}

superTypeIdOrPrototype is either a typeid (which can vary in type depending on how the compiler is set up) or an object prototype... or it is null. This impl guards against the null at the start instead of waiting and after failing the && check, then failing the if check, then failing the map lookup, just bails with the earliest if - but as you pointed out, the old polyfill was passing {} instead of the null, and while my updated did guard against undefined (since Object.create(undefined) is an error), there are differences that break the above code.

gwt/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java

Lines 2083 to 2093 in 7b221c1

	private void generateCallToDefineClass(JClassType type, List<JsNameRef> constructorArgs) {
	JClassType superClass = type.getSuperClass();
	JExpression superTypeId = (superClass == null) ? JNullLiteral.INSTANCE :
	getRuntimeTypeReference(superClass);
	String jsPrototype = getSuperPrototype(type);
	List<JsExpression> defineClassArguments = Lists.newArrayList();
	defineClassArguments.add(transform(getRuntimeTypeReference(type)));
	defineClassArguments.add(jsPrototype == null ? transform(superTypeId) :
	createGlobalQualifier(jsPrototype, type.getSourceInfo()));

The three cases again:

• typeid (int or string, both of which have undefined prototypes), go to the type map and look up the java superclass object to use to chain prototypes
• object, use that as the "superclass" by taking its prototype and passing it to Object.create()
• null - there is no prototype to use.
  • This is what is what happens for GWT's own java.lang.Object, which according to the test above explicitly uses js Object as its supertype. The test wants that, but do we generally want that to be true? I'll assume so for now, so that this patch is just removing the polyfill rather than changing how the output behaves.
  • This is also what happens if your jsinterop code specifies a type that doesn't exist - the generateCallToDefineClass method will emit the defineClass(...) call with a reference to what you claimed, but then it will be null/undefined. Existing code likewise would have normalized that to {}.

I've added another test locally to confirm that this still works (a class with a missing supertype - can't instantiate it, but you can at least reference it). The more I look at this setup code, the more I don't understand why it is as complex as it is... I wonder if there were internal Google users calling some of these "public" methods, so they couldn't be inlined?

[niloc132]

Changed my mind, this code is too ossified, clean up will take more time than I can put to it now. There are confusing/inaccurate comments, extra code that can't be optimized out of JSNI, incomplete renames/removals.

I'm adding another test which validates some edge case (but working) behavior, and restoring the {} as discussed above, and I'd appreciate any review to tell me to revert more or even just drop the whole thing.

[zbynek]

Looking at 3616e45#diff-9c7cf490fdcb064930a5430886b061bd25a79106b9bbf390b7b0fa4144f01d16R85 I'm not sure whether that changes anything 🤔

Object.create({}) and Object.create(null) both return {}

My concern was that if the lookup fails, i.e. @Runtime::prototypesByTypeId[superTypeIdOrPrototype] is undefined, the method will now fail and before it was returning {}. However, from your comment

updated did guard against undefined (since Object.create(undefined) is an error),

I understand that the difference is intentional.

[niloc132]

Object.create({}) and Object.create(null) both return {}

I thought so too, but it breaks tests:

>  Object.create(null).hasOwnProperty 
< null
>  Object.create({}).hasOwnProperty 
< function hasOwnProperty()

Its possible someone somewhere is using that feature in their app.

[tbroyer]

Object.create(null) creates an object with a null prototype (equivalent to __proto__: null), whereas Object.create({}) creates an object with {} as its prototype (so almost equivalent to just using {} or new Object(); as they'll all have Object.prototype in their prototype chain)

portableObjCreate used F.prototype = obj || {}, so the new code should use Object.create(obj ?? {}) for backwards compatibility

[niloc132]

?? isn't an option in JSNI (though || could be used as it was before) - the use case where it would matter would be a non-native jsinterop type that declared a native jsinterop type to be its superclass, but that native type had an undefined prototype. Technically possible, but its going to fail at runtime when you use the type anyway.

As above, this code is more complex than it needs to be - this method is only called in one place, and the caller already knows if it was a super typeid or a super prototype, but lets this method figure it out for itself anyway. This edge case is a bit like the unit test I added - I think it would be reasonable to fail sooner than new if the type didn't exist (since after GWT types are loaded, changes in supertypes outside the app won't be respected). The case here is technically a regression, but its a regression of code that didn't work anyway.

Or am I missing a case where this backwards compatibility would break a real case that isn't already broken? I do know there are a couple of broken types in elemental2 (TypedArray, ArrayBufferView, but you won't want to declare subtypes of those anyway), so I'll give this a spin with a real app that uses some of those and make sure nothing actually breaks. In those two examples though, it isn't a question if the type exists in JS (those two don't), but if they exist and have an undefined (not null, that's safe) prototype.

[niloc132]

At least in the limited real-world code I have that interacts with badly defined jsinterop types, I'm not running into errors.

That said, reviewer's choice - if any of you would like the defensive ||{} back, I'll restore it.