JS ES6 Support and Template Literals Support #1493
Conversation
| } | ||
|
|
||
| @Override | ||
| public CAstNode visitTemplateLiteral(TemplateLiteral node, WalkContext arg) { |
There was a problem hiding this comment.
I can't really understand the logic here; I think we need some comments to explain. Is the code in this method handling just the string concatenation?
There was a problem hiding this comment.
This is the code handling the string concatenation. I can add a comment, and also simplify the logic a bit.
| public CAstNode visitTaggedTemplateLiteral(TaggedTemplateLiteral node, WalkContext arg) { | ||
| // TODO Auto-generated method stub | ||
| return null; | ||
| } |
There was a problem hiding this comment.
I'm concerned this will cause a crash if we get an input with a tagged template literal. If we don't handle this case yet, maybe we should not add this method and just skip visiting the nodes?
| } else if (node instanceof SwitchStatement) { | ||
| return visitSwitchStatement((SwitchStatement) node, arg); | ||
| } else if (node instanceof TaggedTemplateLiteral) { | ||
| return visitTaggedTemplateLiteral((TaggedTemplateLiteral) node, arg); |
There was a problem hiding this comment.
See comment above about not visiting these nodes if we don't yet handle them; or is the issue that we'll get an unexpected node type?
In general, what happens right now when we try to pass an ES6 input to WALA, given that we haven't set the language version? Do we just get a parse error in Rhino?
There was a problem hiding this comment.
Right now, it errors based on a Rhino parse error when given an ES6 input without the language version set.
TaggedTemplateLiteral is a separate node type from Rhino. I haven't tested if I could put a "dummy node" instead to prevent a crash, but I was mirroring some of the other unimplemented node types.
There was a problem hiding this comment.
Ok. I imagine there are various other ES6 nodes we don't handle yet. I think we should update line 205 to throw some kind of catchable exception (maybe UnsupportedNodeTypeException extends RuntimeException) and document that this type of exception will be thrown if we encounter node types we don't handle. It would be nice to return some kind of valid AST and not crash out in such cases, but I don't really see how we could do that.
| CAstCallGraphUtil.dumpCG(B.getCFAContextInterpreter(), B.getPointerAnalysis(), CG); | ||
| } | ||
|
|
||
| private static final Object[][] assertionsES6Args = |
There was a problem hiding this comment.
For the test cases where the template substitution has a function call, can we assert that the appropriate call edge(s) are present?
|
There has been no new discussion about this PR since April 27, 2025. @apple5775 and @msridhar, do you still expect to reach consensus that this PR is good and ready to merge? Or has this PR been abandoned, in which case we should just close it? |
|
@liblit Sorry, I haven't taken the time to go through these yet. I will do my best this week to make the requested changes. |
|
@apple5775 wrote:
Sure, that's fine. There's no rush. I just want to make sure that we're not forgetting anything that still has the potential to improve WALA. As long as I know you still intend to move this PR forward, I don't care how long it takes to do that. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1493 +/- ##
=========================================
Coverage 50.20% 50.21%
- Complexity 12651 12652 +1
=========================================
Files 1365 1365
Lines 85211 85234 +23
Branches 14732 14737 +5
=========================================
+ Hits 42784 42802 +18
- Misses 37632 37635 +3
- Partials 4795 4797 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| } | ||
|
|
||
| @Override | ||
| public CAstNode visitTemplateLiteral(TemplateLiteral node, WalkContext arg) { |
|
|
||
| @Override | ||
| public CAstNode visitTaggedTemplateLiteral(TaggedTemplateLiteral node, WalkContext arg) { | ||
| // TODO Auto-generated method stub |
There was a problem hiding this comment.
Replace with a comment indicating this feature is not yet implemented
| public CAstNode visitTaggedTemplateLiteral(TaggedTemplateLiteral node, WalkContext arg) { | ||
| // TODO Auto-generated method stub | ||
| return null; | ||
| } |
| } else if (node instanceof SwitchStatement) { | ||
| return visitSwitchStatement((SwitchStatement) node, arg); | ||
| } else if (node instanceof TaggedTemplateLiteral) { | ||
| return visitTaggedTemplateLiteral((TaggedTemplateLiteral) node, arg); |
There was a problem hiding this comment.
Ok. I imagine there are various other ES6 nodes we don't handle yet. I think we should update line 205 to throw some kind of catchable exception (maybe UnsupportedNodeTypeException extends RuntimeException) and document that this type of exception will be thrown if we encounter node types we don't handle. It would be nice to return some kind of valid AST and not crash out in such cases, but I don't really see how we could do that.
For javascript, add ES6 Support for rhino 1.7.15, and support Template Literals as a concatenation/addition of strings. Also includes a few tests for Template Literals in "es6.js" file. Tagged Template Literals are not yet supported.