Prevent wrapping statements into JS.ExpressionStatement #211
Conversation
In a visitExpressionStatement parser method, if the expression is a Statement, it shouldn't be wrap into ExpressionStatement. Also in a JavaScriptVisitor class remove unwrapping ExpressionStatement into Statement, if it is a Statement.
| import * as J from '../java' | ||
| import * as JS from "./tree"; | ||
|
|
||
| export const statement_implementations_list: Array<{ new (...args: any[]): J.Statement }> = [ |
There was a problem hiding this comment.
Can we implement this as a type guard?
There was a problem hiding this comment.
Reimplemented the check as isStatement and isExpression methods
Unfortunately, there is no simple way in TS to require that the array contains only classes that implement the Statement/Expression interface.
| J.Label, | ||
| J.Lambda, | ||
| J.MethodDeclaration, | ||
| J.MethodInvocation, |
There was a problem hiding this comment.
Do you also need something like an is_expression or how do we make sure that the types which are both expression and statement don't get wrapped? Also note the inverse wrapper (StatementExpression) which might show the same behavior.
There was a problem hiding this comment.
Have removed unwrapping for the StatementExpression in a visitStatementExpression.
There is only one place in a parser where the StatementExpression is directly used.
There was a problem hiding this comment.
LGTM. I think it would be best if we could have this generated instead of maintaining it by hand, as now every time we add a new type to either J or JS, we need to make sure to check if we need to update these arrays.
There was a problem hiding this comment.
I am missing the following four types from J: DoWhileLoop, ForEachLoop, ForLoop, and WhileLoop. AFAIK we use all of these except for ForEachLoop in our parser.
There was a problem hiding this comment.
I am also missing the JS types ForOfLoop and ForInLoop.
There was a problem hiding this comment.
Yes, thanks!
I've used a small util to generate the list of classes, but missed the Loop inheritors.
There was a problem hiding this comment.
LGTM. I think it would be best if we could have this generated instead of maintaining it by hand, as now every time we add a new type to either
JorJS, we need to make sure to check if we need to update these arrays.
Yes, it will be a more consistent way to keep lists up-to-date.
- remove unwrapping for visitStatementExpression
| JS.Unary, | ||
| JS.Union, | ||
| JS.Void, | ||
| JS.Yield |
There was a problem hiding this comment.
Should we here also have ExpressionStatement and StatementExpression?
2 participants
In a
visitExpressionStatementTS parser method, if the expression is aStatement, it shouldn't be wrapped intoExpressionStatement.Also, in a
JavaScriptVisitorclass, remove unwrappingExpressionStatementintoStatementif it is aStatement.