Misc bug fixes #397
Merged
Misc bug fixes #397
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rianhunter
changed the title
|
oops... sorry i missed seeing this. this looks important. any chances for some tests? |
|
Yeah these are all actually pretty serious / glaring bugs. I can add tests for each bug. |
js2coffee was not correctly indenting code such as:
while (true) {
{
{
var a = 0;
var b = 1;
}
}
}
In that case you'd get:
loop
a = 0
b = 1
The reason is that the code that emits 'BlockStatement' was
unconditionally prepending the current indent to each of its children.
In the preceding case, the code would add an indent to the top-level
BlockStatement then an indent for each child "a = 0" and "b = 0", resulting
in then output:
["loop", "\n", " ", " ", "a", "=", "0", "\n", " ", "b", "=", "0", "\n"]
This code makes it so that the code that emits 'BlockStatement' only outputs
an indent if the child isn't of type 'BlockStatement', effectively flattening
the output.
The `scope` member in the transformer base was being set to
the previously pushed scope on `popStack`, instead of the
scope that was active that before the previously pushed scope
was pushed. This resulted in code like:
var a = 0;
var foo = function () {
var b = function () {
return 0;
};
var a = 1;
};
Being compiled into:
a = 0
foo = ->
b = ->
`var a`
0
a = 1
return
The preceding example would happen because the duplicate
VariableDeclaration for `a` would trigger js2coffee's variable shadowing
handling code, automatically placing the `var a` at the the
top of the current scope's body. The current scope was erroneously
left as the last pushed scope, hence `var a` would be placed at the
top of the "b" function.
The `estraverse` module iterates over a function's AST body using fixed indices stored when the function is first entered. If the body of a function is modified during those methods, it can cause unexpected outcomes during traversal. One outcome is the complete skipping of a sub-statement. This change fixes that.
|
Okay added tests for each broken bug. The tests do a string-comparison against expected outcome. This may cause these tests to generate false positives in the future though I wasn't sure of a better way to add tests for code generation. |
|
a little late for the party, but i sweaaar i'll look at this soon >_< |
|
Ping! These are pretty serious bugs. |
|
Okay, merged, 2.2.0 is out and the website has been updated. Sorry this took a while, stepped away from js2coffee for a bit there. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Some bug fixes with code generation and ast generation.
For the "BlockStatement" commit, my observation is that this is yet another special case of checking for node type "BlockStatement." From what I can tell, this is because the code is structured so that the container nodes are responsible for outputting the indent. If the code was instead structured so that the code that emits statements was responsible for the indent, there would be no special cases and the code would be simpler. Something to think about for the future.