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

Resolve structural issue with exception handling in IRFactory #967

Closed
tuchida opened this issue Jun 29, 2021 · 21 comments · Fixed by #1519
Closed

Resolve structural issue with exception handling in IRFactory #967

tuchida opened this issue Jun 29, 2021 · 21 comments · Fixed by #1519
Labels
bug Issues considered a bug

Comments

@tuchida
Copy link
Contributor

tuchida commented Jun 29, 2021

In the example below, one can see that the IRFactory throws a ParserException when in this case a SyntaxException ought to be thrown.

Further down comment #967 (comment) points to a fix that results in the proper Error being thrown, but proper location info is missing.

These issues ought to be fixed, possibly requiring the refactoring of the IRFactory as suggested below

#853 (comment)
If I evaluate 1=1, I will expect only JavaScript error, but Java exception will be thrown.

$ java -jar buildGradle/libs/rhino-1.7.14-SNAPSHOT.jar
Rhino 1.7.14-SNAPSHOT 2021 06 29
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:329)
	at org.mozilla.javascript.Parser.reportError(Parser.java:315)
	at org.mozilla.javascript.Parser.reportError(Parser.java:310)
	at org.mozilla.javascript.IRFactory.createAssignment(IRFactory.java:2309)
	at org.mozilla.javascript.IRFactory.transformAssignment(IRFactory.java:448)
	at org.mozilla.javascript.IRFactory.transform(IRFactory.java:222)
	at org.mozilla.javascript.IRFactory.transformExprStmt(IRFactory.java:559)
	at org.mozilla.javascript.IRFactory.transform(IRFactory.java:219)
	at org.mozilla.javascript.IRFactory.transformScript(IRFactory.java:1133)
	at org.mozilla.javascript.IRFactory.transform(IRFactory.java:201)
	at org.mozilla.javascript.IRFactory.transformTree(IRFactory.java:121)
	at org.mozilla.javascript.Context.parse(Context.java:2437)
	at org.mozilla.javascript.Context.compileImpl(Context.java:2354)
	at org.mozilla.javascript.Context.compileString(Context.java:1368)
	at org.mozilla.javascript.Context.compileString(Context.java:1356)
	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:534)
	at org.mozilla.javascript.ContextFactory.call(ContextFactory.java:472)
	at org.mozilla.javascript.tools.shell.Main.exec(Main.java:163)
	at org.mozilla.javascript.tools.shell.Main.main(Main.java:138)

Parser$ParserException is private class. There is something strange about this being thrown out of Parser.

There are probably two problems with this. (I'm sorry if I'm wrong.)

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

2. Reanalyzing the entire AST.

Rhino parser seems to work as follows.

  1. Parser make AST from source code.
  2. Within IRFactory, Decompiler reverts the all AST to source code.
  3. IRFactory and Parser will reanalyze the source code.

The syntax of ECMAScript is ambiguous and some things need to be reanalyzed.
https://262.ecma-international.org/12.0/#sec-syntactic-grammar

In certain cases, in order to avoid ambiguities, the syntactic grammar uses generalized productions that permit token sequences that do not form a valid ECMAScript Script or Module. For example, this technique is used for object literals and object destructuring patterns. In such cases a more restrictive supplemental grammar is provided that further restricts the acceptable token sequences. Typically, an early error rule will then define an error condition if "P is not covering an N", where P is a Parse Node (an instance of the generalized production) and N is a nonterminal from the supplemental grammar. Here, the sequence of tokens originally matched by P is parsed again using N as the goal symbol. (If N takes grammatical parameters, then they are set to the same values used when P was originally parsed.) An error occurs if the sequence of tokens cannot be parsed as a single instance of N, with no tokens left over. Subsequently, algorithms access the result of the parse using a phrase of the form "the N that is covered by P". This will always be a Parse Node (an instance of N, unique for a given P), since any parsing failure would have been detected by an early error rule.

However, it should only be necessary to reanalyze the grammar that write "cover".

P is not covering an N

@tuchida
Copy link
Contributor Author

tuchida commented Jun 29, 2021

Rhino's Function#toString is output by the Decompiler, so comments are lost or formatted.

(function() { /* comment */ if(1){}}).toString()
// output
function () {
    if (1) {
    }
}

For example, this is what it looks like in SpiderMonkey now. (It used to be the same as Rhino)

(function() { /* comment */ if(1){}}).toString()
// output
"function() { /* comment */ if(1){}}"

Maybe the refactoring will eliminate the need for the Decompiler.

@p-bakker p-bakker added the bug Issues considered a bug label Jun 29, 2021
@p-bakker
Copy link
Collaborator

Looks related: #406

@p-bakker
Copy link
Collaborator

p-bakker commented Jul 6, 2021

@tuchida I see 3 things in this case:

  1. desire to refactor IRFactory
  2. A ParseException instead of SyntaxException on 1 = 1
  3. Comments going missing

With regards to 3: this should maybe be a separate issue? We also have #122, which is old but contains a lot of tests and changes to make those tests pass. Might be interesting to resurrect that PR

Couldn't 2 be fixed, without a complete overhaul of the IRFactory?

And are you going to work on 1? If so, shall I assign the issue to you?

@tuchida
Copy link
Contributor Author

tuchida commented Jul 6, 2021

I can't promise that I'll fix it. I'm sorry.
There are several ways to handle this, but the easiest is to just catch Parser$ParserException. But it's not a fundamental fix.
For example, #984 appears to be fixed, but the correct location of the error is not displayed because the TokenStream cannot be referenced.

$ java -jar buildGradle/libs/rhino-1.7.14-SNAPSHOT.jar -version 200
Rhino 1.7.14-SNAPSHOT 2021 07 06
js> 




for (let [x, x] in {}) {}js> js> js> js> js> 
js: line 1: TypeError: redeclaration of variable x.   // It is not the first line.

@p-bakker
Copy link
Collaborator

p-bakker commented Jul 6, 2021

no worries, gonna update the description a bit to better represent what this issue is about then

@p-bakker p-bakker changed the title Refactor IRFactory Resolve structural issue with exception handling in IRFactory Jul 6, 2021
@tuchida
Copy link
Contributor Author

tuchida commented Jul 6, 2021

A ParseException instead of SyntaxException on 1 = 1

It's a small detail, but it seems that SyntaxException is raised and then Parser$ParserException is thrown.

@tuchida
Copy link
Contributor Author

tuchida commented Jul 6, 2021

IRFactory inheritance of Parser

I am not sure, but perhaps this is not right. First, I want to change this to delegation.
What do you think? Is such a big change acceptable to you?

@p-bakker
Copy link
Collaborator

p-bakker commented Jul 6, 2021

IDK, not very (or basically not) familiar with this part of Rhino, maybe others have some insight on this, sry

@tuchida
Copy link
Contributor Author

tuchida commented Jan 18, 2022

I am in the process of fixing it, but I would like to modify it in this way.
master...tuchida:refactor-IRFactory

  1. IRFactory no longer inherits from Parser. This eliminates Code reuse via inheritance.
  2. Decompiler has been removed. This make that Function.prototype.toString behaves as per the ECMA262 specification.
  3. It is now possible to throw a SyntaxError in IRFactory. It will no longer terminate at 1=1.

I believe that this fix is necessary if we are going to add the latest ECMA262 features to Rhino in the future. When I actually tried to add Computed Property Names and default parameters, I had trouble with the IRFactory and Decompiler code. However, I also understand that it is a big risk for Rhino, with its old history, to accept such a big modification. I can also split the Pull Request.

I would like to discuss with you what kind of pull requests you can accept.

@p-bakker
Copy link
Collaborator

Hi @tuchida, great to hear you're working on this!

As I mentioned before, I don't have much insight into this area of Rhino, but in general any change that fixes problems and gets us closer to EcmaScript compatibility is a good thing imho.

Whether such a (big) change can be accepted depends (again imho) on the potential impact, in area's of performance and backwards compatibility. I tried to look through the changes, but the first thing I noticed was that interspersed with the actual changes were a lot of formatting changes.

I think it would make sense to split out the formatting changes from the other changes and create a separate PR for those, so the actual changes become easier to review.

And if you're up for it: why not add all the remaining formatting of src files while at it, as per #1024

@p-bakker
Copy link
Collaborator

And oh: if this helps/is required for the implementation of Computed Property Names and default parameters I'm all for it 👍

@p-bakker
Copy link
Collaborator

@tuchida #1159 got merged so any chance you could rebase your branch and then create a (draft) PR so it can be more easily discussed here?

@tuchida
Copy link
Contributor Author

tuchida commented Jan 28, 2022

ref. master...tuchida:refactor-IRFactory
I rebased it.

  1. Decompiler has been removed. This make that Function.prototype.toString behaves as per the ECMA262 specification.

Presumably this can be enabled test262 by corresponding to #958.

@p-bakker
Copy link
Collaborator

Can you make a (draft) PR of your branch and maybe a comment on that PR what state is in (wip/prototype/ready for merge/...)?

@p-bakker
Copy link
Collaborator

I believe that this fix is necessary if we are going to add the latest ECMA262 features to Rhino in the future. When I actually tried to add Computed Property Names and default parameters, I had trouble with the IRFactory and Decompiler code

@tuchida you wrote earlier that this issue was hampering you when trying to implement Computed Property Names. Now that the IRFactory part is merged, any change that your Computed Properties work is any closer to finishing maybe?

@p-bakker
Copy link
Collaborator

@tuchida other questions for you: as the 'IRFactory inherit from Parser' part of this case is now done:

  • should this case not be closed and another case to get rid of the Decompiler be created?
  • In Shorthand property names #853 you mentioned 2 issues (1 = 1 and for ({};;);) which I think triggered you make this case and make IRFactory from not inheriting from Parser, correct? Since IRFactory no longer inherits from Parser now, is there anything to address those problems?

@tuchida
Copy link
Contributor Author

tuchida commented Oct 23, 2023

1 = 1

This is fixed in d378847 regarding this issue.
I would like to proceed with #1188 but have stopped because I cannot write a test. Once #958 is implemented, the test for Function.prototype.toString in test262 can be enabled.

I removed the #1188 draft for now.

@p-bakker
Copy link
Collaborator

I just tried 1 = 1 with a 1.7.15 snapshot I built and get this:

$ java -jar rhino-1.7.15-SNAPSHOT.jar -version 200
Rhino 1.7.15-SNAPSHOT
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:339)
        at org.mozilla.javascript.Parser.reportError(Parser.java:325)
        at org.mozilla.javascript.Parser.reportError(Parser.java:320)
        at org.mozilla.javascript.IRFactory.createAssignment(IRFactory.java:2304)
        at org.mozilla.javascript.IRFactory.transformAssignment(IRFactory.java:446)
        at org.mozilla.javascript.IRFactory.transform(IRFactory.java:219)
        at org.mozilla.javascript.IRFactory.transformExprStmt(IRFactory.java:557)
        at org.mozilla.javascript.IRFactory.transform(IRFactory.java:216)
        at org.mozilla.javascript.IRFactory.transformScript(IRFactory.java:1129)
        at org.mozilla.javascript.IRFactory.transform(IRFactory.java:198)
        at org.mozilla.javascript.IRFactory.transformTree(IRFactory.java:118)
        at org.mozilla.javascript.Context.parse(Context.java:2517)
        at org.mozilla.javascript.Context.compileImpl(Context.java:2434)
        at org.mozilla.javascript.Context.compileString(Context.java:1402)
        at org.mozilla.javascript.Context.compileString(Context.java:1390)
        at org.mozilla.javascript.tools.shell.Main.processSource(Main.java:480)
        at org.mozilla.javascript.tools.shell.Main.processFiles(Main.java:180)
        at org.mozilla.javascript.tools.shell.Main$IProxy.run(Main.java:98)
        at org.mozilla.javascript.Context.call(Context.java:544)
        at org.mozilla.javascript.ContextFactory.call(ContextFactory.java:475)
        at org.mozilla.javascript.tools.shell.Main.exec(Main.java:164)
        at org.mozilla.javascript.tools.shell.Main.main(Main.java:142)

Is that the correct exception to be thrown?

@tuchida
Copy link
Contributor Author

tuchida commented Oct 23, 2023

If #1188 is merged, it will look like this

Rhino 1.7.15-SNAPSHOT
js> 1=1
js: line 2: Invalid assignment left-hand side.
js: 
js: ^
js: line 2: Compilation produced 1 syntax errors.

@p-bakker
Copy link
Collaborator

Right, I see now...

@tonygermano
Copy link
Contributor

Glad to see there is already progress on this. I just rediscovered the same bug. First I accidentally tried (<></> += 'asdf'), and when I saw the "Invalid assignment left-hand side" I tried 'a' = 'b' which also threw the Parser$ParserException.

@p-bakker p-bakker added this to the Release 1.7.16 milestone Jul 14, 2024
@p-bakker p-bakker unpinned this issue Jul 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues considered a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants