Rich-Harris / butternut

The fast, future-friendly minifier
https://butternut.now.sh
MIT License
1.17k stars 17 forks source link

Unused identifiers in object/array patterns are not removed #5

Open Rich-Harris opened 7 years ago

Rich-Harris commented 7 years ago

Edited: no longer a bug, but suboptimal output

// input
(function () {
  var {foo,bar} = obj;
  console.log(foo);
}())

// expected
!function(){var {foo:a}=obj;console.log(a)}()

// or maybe
!function(){console.log(obj.foo)}()

// actual
!function(){var {foo:a,bar:b}=obj;console.log(a)}()
trueadm commented 7 years ago

Wouldn't the better output be?

!function(){console.log(obj.foo)}()

That would be more along the lines of what Closure Compiler does but it would be awesome if butternut could too :)

Rich-Harris commented 7 years ago

Yes, but one thing at a time 😀 That would be a separate optimisation that determined a) if a variable was only used once, and b) whether it would be safe to inline it. Not specifically related to destructuring.

Incidentally this is no longer a bug (can try it out on https://butternut.now.sh/) — it now gives this result...

!function(){var{foo:a,bar:b}=obj;console.log(a)}()

...which is correct, but suboptimal. Working on eliminating unused identifiers inside object/array patterns.

Rich-Harris commented 7 years ago

(Incidentally, Closure generates code like you suggest; Uglify doesn't)

trueadm commented 7 years ago

@Rich-Harris could removing unused identifiers in objects cause performance deopts due to monomorphism?

Rich-Harris commented 7 years ago

Not talking about removing identifiers in objects, just in object patterns. Do you know if that has the same performance implications?

trueadm commented 7 years ago

@Rich-Harris I thought you meant you'd remove bar from the destructured object, I guess you just meant the alias to b would be removed?

Rich-Harris commented 7 years ago

Am saying that in this situation...

// current
!function(){var{foo:a,bar:b}=obj;console.log(a)}()

// better?
!function(){var{foo:a}=obj;console.log(a)}()

...we're not affecting the shape of obj in any way, we're just not creating a local identifier for obj.bar because it's unused. I'd assume that that doesn't have performance implications, but I'm not certain.

trueadm commented 7 years ago

@Rich-Harris That's what I originally meant, I'm not sure either – I thought it may have had an affect. I'll probably do some benchmarks on that actually sometime, I'm interested in knowing now. Either way, good work on getting this bug fixed :)