refactor(es/minifier): Don't create invalid nodes #6191
Conversation
kdy1
changed the title
| @@ -1,5 +1,6 @@ | |||
| //// [ES5For-of16.ts] | |||
| for(var _i = 0, _iter = []; _i < _iter.length; _i++){ | |||
| _iter[_i]; | |||
| for(var _i1 = 0, _iter1 = []; _i1 < _iter1.length; _i1++)_iter1[_i1]; | |||
| for(var _i1 = 0, _iter1 = []; _i1 < _iter1.length; _i1++); | |||
There was a problem hiding this comment.
These tests changes are bugfix
There was a problem hiding this comment.
Previously we generated foo = Expr::Seq(seq) so logic expecting identifier in foo didn't work correctly.
There was a problem hiding this comment.
Now we generate foo = seq.0 so it's analyzed/processed correctly
There was a problem hiding this comment.
I believe this output is actually incorrect for the input. The following line should be inside the inner for loop, not after.
![kodiakhq[bot]](https://avatars.githubusercontent.com/in/29196?s=60&v=4){kind=small}
| @@ -1,5 +1,6 @@ | |||
| //// [ES5For-of16.ts] | |||
| for(var _i = 0, _iter = []; _i < _iter.length; _i++){ | |||
| _iter[_i]; | |||
| for(var _i1 = 0, _iter1 = []; _i1 < _iter1.length; _i1++)_iter1[_i1]; | |||
| for(var _i1 = 0, _iter1 = []; _i1 < _iter1.length; _i1++); | |||
There was a problem hiding this comment.
I believe this output is actually incorrect for the input. The following line should be inside the inner for loop, not after.
| @@ -986,7 +982,9 @@ where | |||
| }; | |||
| self.merge_sequences_in_seq_expr(&mut e); | |||
|
|
|||
| return Some(Expr::Seq(e)); | |||
| let mut e = Expr::Seq(e); | |||
There was a problem hiding this comment.
Do we know exprs has more than 1 element?
There was a problem hiding this comment.
We normalize instead of verifying, at the line below.
I think we can improve this in the future
| @@ -1008,7 +1006,9 @@ where | |||
| }; | |||
| self.merge_sequences_in_seq_expr(&mut e); | |||
|
|
|||
| Some(Expr::Seq(e)) | |||
| let mut e = Expr::Seq(e); | |||
| match s { | ||
| Stmt::Decl(Decl::Var(v)) if v.decls.is_empty() => { | ||
| s.take(); | ||
| if self.prepend_stmts.is_empty() && self.append_stmts.is_empty() { |
There was a problem hiding this comment.
This is the fix for wrong appending
|
|
||
| if seq.exprs.len() == 1 { | ||
| *e = *seq.exprs.take().into_iter().next().unwrap(); | ||
| self.normalize_expr(e); |
There was a problem hiding this comment.
Nit: isn't this already normalized from the above for e in seq.exprs? Can extracting it out of the sequence expression change the normalization?
There was a problem hiding this comment.
Actually it had an effect beause for e does not handle len() == 1
Description:
Related issue: