Skip to content

Use Object.create() instead of our own polyfill #10184

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+38 −15
Open

Use Object.create() instead of our own polyfill #10184

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
403d2cd
Use Object.create() instead of our own polyfill
niloc132 Oct 30, 2025
3616e45
Add a test, use {} as the fallback, and fix a comment
niloc132 Nov 12, 2025
f20bfd2
Update more docs, refs to createFor that should be maybeSetClassLiteral
niloc132 Nov 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 13 additions & 15 deletions dev/core/super/com/google/gwt/dev/jjs/intrinsic/com/google/gwt/lang/Runtime.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,11 @@
* Otherwise it creates the prototype for the class by calling {@code createSubclassPrototype()}.
* It also assigns the castable type map and sets the constructors prototype field to the
* current prototype.
* Finally adds the class literal if it was created before the call to {@code defineClass}.
* <p>
* Finally, adds the class literal if it was created before the call to {@code defineClass}.
* Class literals might be created before the call to {@code defineClass} if they are in separate
* code-split fragments. In that case Class.createFor* methods will have created a placeholder and
* stored in {@code prototypesByTypeId} the class literal.
* code-split fragments. In that case, Class.createFor* methods will call have called
* Class.maybeSetClassLiteral to create a placeholder and stored it in {@code prototypesByTypeId}.
* <p>
* As a prerequisite if superTypeIdOrPrototype is not null, it is assumed that defineClass for the
* supertype has already been called.
Expand Down Expand Up @@ -77,23 +78,20 @@
}
}-*/;

private static native JavaScriptObject portableObjCreate(JavaScriptObject obj) /*-{
function F() {};
F.prototype = obj || {};
return new F();
}-*/;

/**
* Create a subclass prototype.
*/
private static native JavaScriptObject createSubclassPrototype(
JavaScriptObject superTypeIdOrPrototype) /*-{
var superPrototype = superTypeIdOrPrototype && superTypeIdOrPrototype.prototype;
if (!superPrototype) {
// If it is not a prototype, then it should be a type id.
superPrototype = @Runtime::prototypesByTypeId[superTypeIdOrPrototype];
var superPrototype = {};
if (superTypeIdOrPrototype != null) {
superPrototype = superTypeIdOrPrototype && superTypeIdOrPrototype.prototype;
if (!superPrototype) {
// If it is not a prototype, then it should be a type id.
superPrototype = @Runtime::prototypesByTypeId[superTypeIdOrPrototype];
}
}
return @Runtime::portableObjCreate(*)(superPrototype);
return Object.create(superPrototype);
Copy link
Collaborator

@zbynek zbynek Nov 11, 2025

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@niloc132 niloc132 Nov 12, 2025

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@niloc132 niloc132 Nov 12, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@zbynek zbynek Nov 13, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@niloc132 niloc132 Nov 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@tbroyer tbroyer Nov 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@niloc132 niloc132 Nov 13, 2025

Choose a reason for hiding this comment

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

?? 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.

Copy link
Member Author

@niloc132 niloc132 Nov 13, 2025

Choose a reason for hiding this comment

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

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.

}-*/;

public static native void copyObjectProperties(JavaScriptObject from, JavaScriptObject to) /*-{
Expand All @@ -109,7 +107,7 @@
*/
private static native JavaScriptObject maybeGetClassLiteralFromPlaceHolder(
JavaScriptObject entry) /*-{
// TODO(rluble): Relies on Class.createFor*() storing the class literal wrapped as an array
// TODO(rluble): Relies on Class.maybeSetClassLiteral() storing the class literal wrapped as an array

Check warning on line 110 in dev/core/super/com/google/gwt/dev/jjs/intrinsic/com/google/gwt/lang/Runtime.java

View workflow job for this annotation

GitHub Actions / build (21) 

[checkstyle] reported by reviewdog 🐶
Line is longer than 100 characters (found 105).

Raw Output:
/home/runner/work/gwt/gwt/gwt/dev/core/super/com/google/gwt/dev/jjs/intrinsic/com/google/gwt/lang/Runtime.java:110:0: warning: Line is longer than 100 characters (found 105). (com.puppycrawl.tools.checkstyle.checks.sizes.LineLengthCheck)
// to distinguish it from an actual prototype.
return (entry instanceof Array) ? entry[0] : null;
}-*/;
Expand Down
25 changes: 25 additions & 0 deletions user/test/com/google/gwt/core/interop/JsTypeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,6 +374,31 @@ public void testEnumSubclassEnumeration() {
assertEquals(1, callPublicMethodFromEnumerationSubclass(MyEnumWithSubclassGen.C));
}

@JsType(isNative = true, namespace = JsPackage.GLOBAL)
public static class DoesntExist {}

@JsType
public static class SubtypeOfDoesntExist extends DoesntExist {
public static boolean test() {
return true;
}
}

public void testMissingClassHierarchy() {
// The class itself can be created and referenced, static members will work
assertTrue(SubtypeOfDoesntExist.test());

// Creating a new instance will fail, something like "Cannot call method "call" of undefined"
try {
new SubtypeOfDoesntExist();
fail("Should not be able to create instance");
} catch (Exception expected) {
// ignore, expected
}

assertFalse(new Object() instanceof SubtypeOfDoesntExist);
}

private static native int callIntFunction(Object object, String functionName) /*-{
return object[functionName]();
}-*/;
Expand Down
Loading