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

Shorthand property names #853

Merged
merged 2 commits into from
Apr 13, 2021
Merged

Conversation

tuchida
Copy link
Contributor

@tuchida tuchida commented Apr 3, 2021

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Object_initializer#new_notations_in_ecmascript_2015

We can now do this.

var a = 123;
({a});
var fn = ({ a }) => print(a);

This has always been possible.

var { a } = { a: 123 };
a; // 123

function fn({ a }) { print(a); }
fn({ a: 123 }); // 123

! dstr-obj-id-put-unresolvable-no-strict.js
! dstr-obj-id-put-unresolvable-strict.js
! dstr-obj-id-simple-no-strict.js
! dstr-obj-id-simple-strict.js
Copy link
Collaborator

Choose a reason for hiding this comment

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

looks great, seems like many more tests passing. Any idea about this one - why was this passing in the past?

Copy link
Contributor Author

@tuchida tuchida Apr 3, 2021

Choose a reason for hiding this comment

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

https://github.com/tc39/test262/blob/041da54c02ae7d17edb8c134ab7691c4f643bafe/test/language/statements/for-of/dstr-obj-id-simple-strict.js

for ({ eval } of [{}]) ;

Previously, it was an error because shorthand was not possible. Correctly, it should be an error because it cannot be assigned to eval in strict mode.

@@ -2466,7 +2466,7 @@ void decompileObjectLiteral(ObjectLiteral node) {
for (int i = 0; i < size; i++) {
ObjectProperty prop = props.get(i);
boolean destructuringShorthand =
Boolean.TRUE.equals(prop.getProp(Node.DESTRUCTURING_SHORTHAND));
Boolean.TRUE.equals(prop.getProp(Node.SHORTHAND_PROPERTY_NAME));
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe we need a matching variable name here also

Copy link
Contributor Author

Choose a reason for hiding this comment

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

5e9aadf Fixed it.

@gbrail
Copy link
Collaborator

gbrail commented Apr 3, 2021

Looks very promising. Can you look at the "eval" case that @rbri found? I'll test myself and merge next week. Thank you!

@tuchida
Copy link
Contributor Author

tuchida commented Apr 4, 2021

I would like to check the eval in this one.

return createDestructuringAssignment(-1, left, right);

However, It seems that if call reportError from IRFactory, the ParserException is not caught.

$ java -jar buildGradle/libs/rhino-1.7.14-SNAPSHOT.jar -version 200
Rhino 1.7.14-SNAPSHOT 2021 04 04
js> 1 = 1
js: line 1: Invalid assignment left-hand side.
js: 
js: ^
Exception in thread "main" org.mozilla.javascript.Parser$ParserException
	at org.mozilla.javascript.Parser.reportError(Parser.java:330)
	at org.mozilla.javascript.Parser.reportError(Parser.java:312)
	at org.mozilla.javascript.Parser.reportError(Parser.java:307)
	at org.mozilla.javascript.IRFactory.createAssignment(IRFactory.java:2275)
	at org.mozilla.javascript.IRFactory.transformAssignment(IRFactory.java:442)
	at org.mozilla.javascript.IRFactory.transform(IRFactory.java:215)
	at org.mozilla.javascript.IRFactory.transformExprStmt(IRFactory.java:547)
	at org.mozilla.javascript.IRFactory.transform(IRFactory.java:212)
	at org.mozilla.javascript.IRFactory.transformScript(IRFactory.java:1077)
	at org.mozilla.javascript.IRFactory.transform(IRFactory.java:194)
	at org.mozilla.javascript.IRFactory.transformTree(IRFactory.java:119)
	at org.mozilla.javascript.Context.parse(Context.java:2606)
	at org.mozilla.javascript.Context.compileImpl(Context.java:2540)
	at org.mozilla.javascript.Context.compileString(Context.java:1544)
	at org.mozilla.javascript.Context.compileString(Context.java:1533)
	at org.mozilla.javascript.tools.shell.Main.processSource(Main.java:495)
	at org.mozilla.javascript.tools.shell.Main.processFiles(Main.java:181)
	at org.mozilla.javascript.tools.shell.Main$IProxy.run(Main.java:101)
	at org.mozilla.javascript.Context.call(Context.java:586)
	at org.mozilla.javascript.ContextFactory.call(ContextFactory.java:525)
	at org.mozilla.javascript.tools.shell.Main.exec(Main.java:163)
	at org.mozilla.javascript.tools.shell.Main.main(Main.java:138)

This seems to be the behavior in 1.7R5 as well.
Is this the behavior you expected?

@tuchida
Copy link
Contributor Author

tuchida commented Apr 4, 2021

I found a few other problems and am working on fixing them.
(None of this seems to have anything to do with this Pull Request, though.)

(() => { for ({} of []);}).toString(); 

// {} will disappear
() => {
    for (of []) {
    }
}
js> for ({};;);
Exception in thread "main" java.lang.NullPointerException
	at org.mozilla.javascript.optimizer.BodyCodegen.visitObjectLiteral(BodyCodegen.java:2076)
	at org.mozilla.javascript.optimizer.BodyCodegen.generateExpression(BodyCodegen.java:1121)
	at org.mozilla.javascript.optimizer.BodyCodegen.generateStatement(BodyCodegen.java:828)
	at org.mozilla.javascript.optimizer.BodyCodegen.generateStatement(BodyCodegen.java:646)
	at org.mozilla.javascript.optimizer.BodyCodegen.generateStatement(BodyCodegen.java:646)
	at org.mozilla.javascript.optimizer.BodyCodegen.generateBodyCode(BodyCodegen.java:60)

@gbrail
Copy link
Collaborator

gbrail commented Apr 8, 2021

OK -- I will hold off on merging until you confirm.

Also, we just made a big change to "test262.properties" so please merge with master and re-push when you're ready.

@tuchida
Copy link
Contributor Author

tuchida commented Apr 9, 2021

Thanks!

#853 (comment)
I will create another issues with these.

However, It seems that if call reportError from IRFactory, the ParserException is not caught.

This appears to be a rather deep-rooted problem.

Parser$ParserException is private class. There is something strange about this being thrown out of Parser.
Why IRFactory inheritance of Parser? This makes it difficult to know where to catch the Parser$ParserException.
I felt that IRFactory inheritance of Parser was an anti-pattern of Code reuse via inheritance.

If you are interested in fixing it, I will create an issue.

@rbri
Copy link
Collaborator

rbri commented Apr 11, 2021

@tuchida sorry i made another pr including a change of the test262.properties. But this will be the last one as we reached now the head of the test suite.
I guess @gbrail will merge the 262 update soon :-)

@gbrail
Copy link
Collaborator

gbrail commented Apr 12, 2021

Can you confirm that this works with the recent changes to test262.properties and then we'll merge? Thanks!

@tuchida
Copy link
Contributor Author

tuchida commented Apr 13, 2021

I rebased on top of master.

@gbrail
Copy link
Collaborator

gbrail commented Apr 13, 2021

Great -- thanks!

@gbrail gbrail merged commit 06801e9 into mozilla:master Apr 13, 2021
@tuchida tuchida mentioned this pull request Apr 25, 2021
@tuchida tuchida deleted the shorthand-property-name branch May 27, 2021 08:14
@p-bakker p-bakker added this to the Release 1.7.14 milestone Jun 29, 2021
@p-bakker p-bakker added the feature Issues considered a new feature label Oct 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Issues considered a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants