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

fix(es/fixer): Wrap in expr in for-in head #9209

Merged
merged 10 commits into from
Jul 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions crates/swc/tests/fixture/issues-9xxx/9200/input/.swcrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
{
"jsc": {
"parser": {
"syntax": "typescript",
"tsx": true,
"decorators": true
},
"loose": false,
"minify": {
"compress": false,
"mangle": false
},
"target": "es2022"
},
"module": {
"type": "es6"
},
"minify": false,
"isModule": false
}
3 changes: 3 additions & 0 deletions crates/swc/tests/fixture/issues-9xxx/9200/input/1.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
let count = 0;
for (var a = 1 || (2 in {}) in { x: 1 }) count++;
console.log(count);
47 changes: 47 additions & 0 deletions crates/swc/tests/fixture/issues-9xxx/9200/input/2.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
for (var a = (b in c) in {});
for (var a = 1 || (b in c) in {});
for (var a = 1 + (2 || (b in c)) in {});
for (var a = (() => b in c) in {});
for (var a = 1 || (() => b in c) in {});
for (var a = (() => { b in c; }) in {});
for (var a = [b in c] in {});
for (var a = {b: b in c} in {});
for (var a = (x = b in c) => {} in {});
for (var a = class extends (b in c) {} in {});
for (var a = function (x = b in c) {} in {});

for (var a = (b in c);;);
for (var a = 1 || (b in c);;);
for (var a = 1 + (2 || (b in c));;);
for (var a = (() => b in c);;);
for (var a = 1 || (() => b in c);;);
for (var a = (() => { b in c; });;);
for (var a = [b in c];;);
for (var a = {b: b in c};;);
for (var a = (x = b in c) => {};;);
for (var a = class extends (b in c) {};;);
for (var a = function (x = b in c) {};;);

for (var a in (b in c));
for (var a in 1 || (b in c));
for (var a in 1 + (2 || (b in c)));
for (var a in (() => b in c));
for (var a in 1 || (() => b in c));
for (var a in (() => { b in c; }));
for (var a in [b in c]);
for (var a in {b: b in c});
for (var a in (x = b in c) => {});
for (var a in class extends (b in c) {});
for (var a in function (x = b in c) {});

for (;a = (b in c););
for (;a = 1 || (b in c););
for (;a = 1 + (2 || (b in c)););
for (;a = (() => b in c););
for (;a = 1 || (() => b in c););
for (;a = (() => { b in c; }););
for (;a = [b in c];);
for (;a = {b: b in c};);
for (;a = (x = b in c) => {};);
for (;a = class extends (b in c) {};);
for (;a = function (x = b in c) {};);
5 changes: 5 additions & 0 deletions crates/swc/tests/fixture/issues-9xxx/9200/output/1.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
let count = 0;
for(var a = 1 || (2 in {}) in {
x: 1
})count++;
console.log(count);
72 changes: 72 additions & 0 deletions crates/swc/tests/fixture/issues-9xxx/9200/output/2.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
for(var a = (b in c) in {});
for(var a = 1 || (b in c) in {});
for(var a = 1 + (2 || (b in c)) in {});
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have some false positives. It is no longer necessary to add parentheses around the in expression that is already in parentheses.

We need to check the in expressions from the outside in. But currently, the wrap calls are from the inside out, which cannot be completed in a single pass.

Do you have any thoughts? @kdy1

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we proceed with some false positives for now and refactor them outside? If you prefer, rewriting the fixer to work outside in is also fine.

Copy link
Member Author

@magic-akari magic-akari Jul 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I plan to leave a TODO for now, as this is a very rare edge case and has no negative impact.

for(var a = ()=>(b in c) in {});
for(var a = 1 || (()=>(b in c)) in {});
for(var a = ()=>{
b in c;
} in {});
for(var a = [
b in c
] in {});
for(var a = {
b: b in c
} in {});
for(var a = (x = b in c)=>{} in {});
for(var a = class extends (b in c) {
} in {});
for(var a = function(x = b in c) {} in {});
for(var a = (b in c);;);
for(var a = 1 || (b in c);;);
for(var a = 1 + (2 || (b in c));;);
for(var a = ()=>(b in c);;);
for(var a = 1 || (()=>(b in c));;);
for(var a = ()=>{
b in c;
};;);
for(var a = [
b in c
];;);
for(var a = {
b: b in c
};;);
for(var a = (x = b in c)=>{};;);
for(var a = class extends (b in c) {
};;);
for(var a = function(x = b in c) {};;);
for(var a in b in c);
for(var a in 1 || b in c);
for(var a in 1 + (2 || b in c));
for(var a in ()=>b in c);
for(var a in 1 || (()=>b in c));
for(var a in ()=>{
b in c;
});
for(var a in [
b in c
]);
for(var a in {
b: b in c
});
for(var a in (x = b in c)=>{});
for(var a in class extends (b in c) {
});
for(var a in function(x = b in c) {});
for(; a = b in c;);
for(; a = 1 || b in c;);
for(; a = 1 + (2 || b in c););
for(; a = ()=>b in c;);
for(; a = 1 || (()=>b in c););
for(; a = ()=>{
b in c;
};);
for(; a = [
b in c
];);
for(; a = {
b: b in c
};);
for(; a = (x = b in c)=>{};);
for(; a = class extends (b in c) {
};);
for(; a = function(x = b in c) {};);
2 changes: 1 addition & 1 deletion crates/swc_ecma_minifier/tests/benches-full/echarts.js
Original file line number Diff line number Diff line change
Expand Up @@ -6154,7 +6154,7 @@
needDrawBg && this._renderBackground(style, style, boxX, boxY, outerWidth_1, outerHeight);
}
textY += lineHeight / 2, textPadding && (textX = getTextXForPadding(baseX, textAlign, textPadding), 'top' === verticalAlign ? textY += textPadding[0] : 'bottom' === verticalAlign && (textY -= textPadding[2]));
for(var defaultLineWidth = 0, useDefaultFill = !1, textFill = null == (fill = ('fill' in style) ? style.fill : (useDefaultFill = !0, defaultStyle.fill)) || 'none' === fill ? null : fill.image || fill.colorStops ? '#000' : fill, textStroke = getStroke(('stroke' in style) ? style.stroke : bgColorDrawn || defaultStyle.autoStroke && !useDefaultFill ? null : (defaultLineWidth = 2, defaultStyle.stroke)), hasShadow = style.textShadowBlur > 0, fixedBoundingRect = null != style.width && ('truncate' === style.overflow || 'break' === style.overflow || 'breakAll' === style.overflow), calculatedLineHeight = contentBlock.calculatedLineHeight, i = 0; i < textLines.length; i++){
for(var defaultLineWidth = 0, useDefaultFill = !1, textFill = null == (fill = ('fill' in style) ? style.fill : (useDefaultFill = !0, defaultStyle.fill)) || 'none' === fill ? null : fill.image || fill.colorStops ? '#000' : fill, textStroke = getStroke('stroke' in style ? style.stroke : bgColorDrawn || defaultStyle.autoStroke && !useDefaultFill ? null : (defaultLineWidth = 2, defaultStyle.stroke)), hasShadow = style.textShadowBlur > 0, fixedBoundingRect = null != style.width && ('truncate' === style.overflow || 'break' === style.overflow || 'breakAll' === style.overflow), calculatedLineHeight = contentBlock.calculatedLineHeight, i = 0; i < textLines.length; i++){
var el = this._getOrCreateChild(TSpan), subElStyle = el.createStyle();
el.useStyle(subElStyle), subElStyle.text = textLines[i], subElStyle.x = textX, subElStyle.y = textY, textAlign && (subElStyle.textAlign = textAlign), subElStyle.textBaseline = 'middle', subElStyle.opacity = style.opacity, subElStyle.strokeFirst = !0, hasShadow && (subElStyle.shadowBlur = style.textShadowBlur || 0, subElStyle.shadowColor = style.textShadowColor || 'transparent', subElStyle.shadowOffsetX = style.textShadowOffsetX || 0, subElStyle.shadowOffsetY = style.textShadowOffsetY || 0), textStroke && (subElStyle.stroke = textStroke, subElStyle.lineWidth = style.lineWidth || defaultLineWidth, subElStyle.lineDash = style.lineDash, subElStyle.lineDashOffset = style.lineDashOffset || 0), textFill && (subElStyle.fill = textFill), subElStyle.font = textFont, textY += lineHeight, fixedBoundingRect && el.setBoundingRect(new BoundingRect(adjustTextX(subElStyle.x, style.width, subElStyle.textAlign), adjustTextY(subElStyle.y, calculatedLineHeight, subElStyle.textBaseline), style.width, calculatedLineHeight));
}
Expand Down
4 changes: 2 additions & 2 deletions crates/swc_ecma_minifier/tests/benches-full/jquery.js
Original file line number Diff line number Diff line change
Expand Up @@ -3125,9 +3125,9 @@
for(!function(props, specialEasing) {
var index, name, easing, value, hooks;
// camelCase, specialEasing and expand cssHook pass
for(index in props)if (easing = specialEasing[name = camelCase(index)], Array.isArray(value = props[index]) && (easing = value[1], value = props[index] = value[0]), index !== name && (props[name] = value, delete props[index]), (hooks = jQuery.cssHooks[name]) && ("expand" in hooks)) // Not quite $.extend, this won't overwrite existing keys.
for(index in props)if (easing = specialEasing[name = camelCase(index)], Array.isArray(value = props[index]) && (easing = value[1], value = props[index] = value[0]), index !== name && (props[name] = value, delete props[index]), (hooks = jQuery.cssHooks[name]) && "expand" in hooks) // Not quite $.extend, this won't overwrite existing keys.
// Reusing 'index' because we have the correct "name"
for(index in value = hooks.expand(value), delete props[name], value)(index in props) || (props[index] = value[index], specialEasing[index] = easing);
for(index in value = hooks.expand(value), delete props[name], value)index in props || (props[index] = value[index], specialEasing[index] = easing);
else specialEasing[name] = easing;
}(props, animation.opts.specialEasing); index < length; index++)if (result = Animation.prefilters[index].call(animation, elem, props, animation.opts)) return isFunction(result.stop) && (jQuery._queueHooks(animation.elem, animation.opts.queue).stop = result.stop.bind(result)), result;
return jQuery.map(props, createTween, animation), isFunction(animation.opts.start) && animation.opts.start.call(elem, animation), // Attach callbacks from options
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8201,14 +8201,14 @@
for(var RegExpWrapper = function(pattern, flags) {
var rawFlags, dotAll, sticky, handled, result, state, thisIsRegExp = this instanceof RegExpWrapper, patternIsRegExp = isRegExp(pattern), flagsAreUndefined = void 0 === flags, groups = [], rawPattern = pattern;
if (!thisIsRegExp && patternIsRegExp && flagsAreUndefined && pattern.constructor === RegExpWrapper) return pattern;
if ((patternIsRegExp || pattern instanceof RegExpWrapper) && (pattern = pattern.source, flagsAreUndefined && (flags = ("flags" in rawPattern) ? rawPattern.flags : getFlags.call(rawPattern))), pattern = void 0 === pattern ? "" : toString1(pattern), flags = void 0 === flags ? "" : toString1(flags), rawPattern = pattern, UNSUPPORTED_DOT_ALL && ("dotAll" in re1) && (dotAll = !!flags && flags.indexOf("s") > -1) && (flags = flags.replace(/s/g, "")), rawFlags = flags, UNSUPPORTED_Y && ("sticky" in re1) && (sticky = !!flags && flags.indexOf("y") > -1) && (flags = flags.replace(/y/g, "")), UNSUPPORTED_NCG && (pattern = (handled = handleNCG(pattern))[0], groups = handled[1]), result = inheritIfRequired(NativeRegExp(pattern, flags), thisIsRegExp ? this : RegExpPrototype, RegExpWrapper), (dotAll || sticky || groups.length) && (state = enforceInternalState(result), dotAll && (state.dotAll = !0, state.raw = RegExpWrapper(handleDotAll(pattern), rawFlags)), sticky && (state.sticky = !0), groups.length && (state.groups = groups)), pattern !== rawPattern) try {
if ((patternIsRegExp || pattern instanceof RegExpWrapper) && (pattern = pattern.source, flagsAreUndefined && (flags = "flags" in rawPattern ? rawPattern.flags : getFlags.call(rawPattern))), pattern = void 0 === pattern ? "" : toString1(pattern), flags = void 0 === flags ? "" : toString1(flags), rawPattern = pattern, UNSUPPORTED_DOT_ALL && "dotAll" in re1 && (dotAll = !!flags && flags.indexOf("s") > -1) && (flags = flags.replace(/s/g, "")), rawFlags = flags, UNSUPPORTED_Y && "sticky" in re1 && (sticky = !!flags && flags.indexOf("y") > -1) && (flags = flags.replace(/y/g, "")), UNSUPPORTED_NCG && (pattern = (handled = handleNCG(pattern))[0], groups = handled[1]), result = inheritIfRequired(NativeRegExp(pattern, flags), thisIsRegExp ? this : RegExpPrototype, RegExpWrapper), (dotAll || sticky || groups.length) && (state = enforceInternalState(result), dotAll && (state.dotAll = !0, state.raw = RegExpWrapper(handleDotAll(pattern), rawFlags)), sticky && (state.sticky = !0), groups.length && (state.groups = groups)), pattern !== rawPattern) try {
// fails in old engines, but we have no alternatives for unsupported regex syntax
createNonEnumerableProperty(result, "source", "" === rawPattern ? "(?:)" : rawPattern);
} catch (error) {
/* empty */ }
return result;
}, proxy = function(key) {
(key in RegExpWrapper) || defineProperty(RegExpWrapper, key, {
key in RegExpWrapper || defineProperty(RegExpWrapper, key, {
configurable: !0,
get: function() {
return NativeRegExp[key];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3855,9 +3855,9 @@
for(function(props, specialEasing) {
var value, name1, index, easing, hooks;
// camelCase, specialEasing and expand cssHook pass
for(index in props)if (easing = specialEasing[name1 = jQuery.camelCase(index)], value = props[index], jQuery.isArray(value) && (easing = value[1], value = props[index] = value[0]), index !== name1 && (props[name1] = value, delete props[index]), (hooks = jQuery.cssHooks[name1]) && ("expand" in hooks)) // not quite $.extend, this wont overwrite keys already present.
for(index in props)if (easing = specialEasing[name1 = jQuery.camelCase(index)], value = props[index], jQuery.isArray(value) && (easing = value[1], value = props[index] = value[0]), index !== name1 && (props[name1] = value, delete props[index]), (hooks = jQuery.cssHooks[name1]) && "expand" in hooks) // not quite $.extend, this wont overwrite keys already present.
// also - reusing 'index' from above because we have the correct "name"
for(index in value = hooks.expand(value), delete props[name1], value)(index in props) || (props[index] = value[index], specialEasing[index] = easing);
for(index in value = hooks.expand(value), delete props[name1], value)index in props || (props[index] = value[index], specialEasing[index] = easing);
else specialEasing[name1] = easing;
}(props, animation.opts.specialEasing); index < length; index++)if (result = animationPrefilters[index].call(animation, elem, props, animation.opts)) return result;
// attach callbacks from options
Expand Down
96 changes: 71 additions & 25 deletions crates/swc_ecma_transforms_base/src/fixer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,17 +71,6 @@ enum Context {
FreeExpr,
}

macro_rules! array {
($name:ident, $T:tt) => {
fn $name(&mut self, e: &mut $T) {
let old = self.ctx;
self.ctx = Context::ForcedExpr;
e.elems.visit_mut_with(self);
self.ctx = old;
}
};
}

impl Fixer<'_> {
fn wrap_callee(&mut self, e: &mut Expr) {
match e {
Expand All @@ -102,7 +91,13 @@ impl Fixer<'_> {
impl VisitMut for Fixer<'_> {
noop_visit_mut_type!();

array!(visit_mut_array_lit, ArrayLit);
fn visit_mut_array_lit(&mut self, e: &mut ArrayLit) {
let ctx = mem::replace(&mut self.ctx, Context::ForcedExpr);
let in_for_stmt_head = mem::replace(&mut self.in_for_stmt_head, false);
e.elems.visit_mut_with(self);
self.in_for_stmt_head = in_for_stmt_head;
self.ctx = ctx;
}

fn visit_mut_arrow_expr(&mut self, node: &mut ArrowExpr) {
let old = self.ctx;
Expand Down Expand Up @@ -173,7 +168,9 @@ impl VisitMut for Fixer<'_> {
}

fn visit_mut_assign_pat(&mut self, node: &mut AssignPat) {
let in_for_stmt_head = mem::replace(&mut self.in_for_stmt_head, false);
node.visit_mut_children_with(self);
self.in_for_stmt_head = in_for_stmt_head;

if let Expr::Seq(..) = &*node.right {
self.wrap(&mut node.right);
Expand All @@ -185,7 +182,9 @@ impl VisitMut for Fixer<'_> {

let old = self.ctx;
self.ctx = Context::ForcedExpr;
let in_for_stmt_head = mem::replace(&mut self.in_for_stmt_head, false);
node.value.visit_mut_with(self);
self.in_for_stmt_head = in_for_stmt_head;
self.ctx = old;
}

Expand Down Expand Up @@ -345,6 +344,12 @@ impl VisitMut for Fixer<'_> {
}
}

fn visit_mut_block_stmt(&mut self, n: &mut BlockStmt) {
let in_for_stmt_head = mem::replace(&mut self.in_for_stmt_head, false);
n.visit_mut_children_with(self);
self.in_for_stmt_head = in_for_stmt_head;
}

fn visit_mut_block_stmt_or_expr(&mut self, body: &mut BlockStmtOrExpr) {
body.visit_mut_children_with(self);

Expand Down Expand Up @@ -376,9 +381,14 @@ impl VisitMut for Fixer<'_> {
}

fn visit_mut_class(&mut self, node: &mut Class) {
let old = self.ctx;
self.ctx = Context::Default;
node.visit_mut_children_with(self);
let ctx = mem::replace(&mut self.ctx, Context::Default);

node.super_class.visit_mut_with(self);

let in_for_stmt_head = mem::replace(&mut self.in_for_stmt_head, false);
node.body.visit_mut_with(self);
self.in_for_stmt_head = in_for_stmt_head;

match &mut node.super_class {
Some(e)
if e.is_seq()
Expand All @@ -393,7 +403,7 @@ impl VisitMut for Fixer<'_> {
}
_ => {}
};
self.ctx = old;
self.ctx = ctx;

node.body.retain(|m| !matches!(m, ClassMember::Empty(..)));
}
Expand Down Expand Up @@ -472,6 +482,12 @@ impl VisitMut for Fixer<'_> {
self.handle_expr_stmt(&mut s.expr);
}

fn visit_mut_for_head(&mut self, n: &mut ForHead) {
let in_for_stmt_head = mem::replace(&mut self.in_for_stmt_head, true);
n.visit_mut_children_with(self);
self.in_for_stmt_head = in_for_stmt_head;
}

fn visit_mut_for_of_stmt(&mut self, s: &mut ForOfStmt) {
s.visit_mut_children_with(self);

Expand Down Expand Up @@ -507,15 +523,13 @@ impl VisitMut for Fixer<'_> {
}

fn visit_mut_for_stmt(&mut self, n: &mut ForStmt) {
let old = self.in_for_stmt_head;
self.in_for_stmt_head = true;
let in_for_stmt_head = mem::replace(&mut self.in_for_stmt_head, true);
n.init.visit_mut_with(self);
self.in_for_stmt_head = in_for_stmt_head;

n.test.visit_mut_with(self);
n.update.visit_mut_with(self);

self.in_for_stmt_head = false;
n.body.visit_mut_with(self);
self.in_for_stmt_head = old;
}

fn visit_mut_if_stmt(&mut self, node: &mut IfStmt) {
Expand Down Expand Up @@ -594,10 +608,9 @@ impl VisitMut for Fixer<'_> {
}

fn visit_mut_new_expr(&mut self, node: &mut NewExpr) {
let old = self.ctx;
self.ctx = Context::ForcedExpr;
let ctx = mem::replace(&mut self.ctx, Context::ForcedExpr);

node.args.visit_mut_with(self);
self.ctx = old;

self.ctx = Context::Callee { is_new: true };
node.callee.visit_mut_with(self);
Expand All @@ -612,7 +625,7 @@ impl VisitMut for Fixer<'_> {
| Expr::Lit(..) => self.wrap(&mut node.callee),
_ => {}
}
self.ctx = old;
self.ctx = ctx;
}

fn visit_mut_opt_call(&mut self, node: &mut OptCall) {
Expand Down Expand Up @@ -767,13 +780,46 @@ impl VisitMut for Fixer<'_> {
expr.arg.visit_mut_with(self);
self.ctx = old;
}

fn visit_mut_object_lit(&mut self, n: &mut ObjectLit) {
let in_for_stmt_head = mem::replace(&mut self.in_for_stmt_head, false);
n.visit_mut_children_with(self);
self.in_for_stmt_head = in_for_stmt_head;
}

fn visit_mut_params(&mut self, n: &mut Vec<Param>) {
let in_for_stmt_head = mem::replace(&mut self.in_for_stmt_head, false);
n.visit_mut_children_with(self);
self.in_for_stmt_head = in_for_stmt_head;
}

// only used in ArrowExpr
fn visit_mut_pats(&mut self, n: &mut Vec<Pat>) {
let in_for_stmt_head = mem::replace(&mut self.in_for_stmt_head, false);
n.visit_mut_children_with(self);
self.in_for_stmt_head = in_for_stmt_head;
}

fn visit_mut_expr_or_spreads(&mut self, n: &mut Vec<ExprOrSpread>) {
let in_for_stmt_head = mem::replace(&mut self.in_for_stmt_head, false);
n.visit_mut_children_with(self);
self.in_for_stmt_head = in_for_stmt_head;
}
}

impl Fixer<'_> {
fn wrap_with_paren_if_required(&mut self, e: &mut Expr) {
let mut has_padding_value = false;
match e {
Expr::Bin(BinExpr { op: op!("in"), .. }) if self.in_for_stmt_head => {
// TODO:
// if the in expression is in a parentheses, we should not wrap it with a
// parentheses again. But the parentheses is added later,
// so we don't have enough information to detect it at this moment.
// Example:
// for(var a = 1 + (2 || b in c) in {});
// |~~~~~~~~~~~|
// this parentheses is removed by unwrap_expr and added again later
self.wrap(e);
}

Expand Down
Loading