Skip to content

fixes and improvements to [].join()#1453

Closed
alexlamsl wants to merge 1 commit intomishoo:masterfrom
alexlamsl:array-join
Closed

fixes and improvements to [].join()#1453
alexlamsl wants to merge 1 commit intomishoo:masterfrom
alexlamsl:array-join

Conversation

@alexlamsl
Copy link
Copy Markdown
Collaborator

Bug fixes

  • ["a", , "b"].join() ➡️ "a,,b" (was "a,undefined,b")
  • ["a", null, "b"].join() ➡️ "a,,b" (was "a,null,b")
  • ["a", undefined, "b"].join() ➡️ "a,,b" (was "a,undefined,b")

New optimisations

  • ["a", "b"].join(null) ➡️ "anullb"
  • ["a", "b"].join(undefined) ➡️ "a,b"

@alexlamsl alexlamsl changed the title Array join fixes and improvements to [].join() Jan 31, 2017
@alexlamsl
Copy link
Copy Markdown
Collaborator Author

There seems to be a previous effort in #301, looking if I have missed anything and merging their test cases...

@alexlamsl
Copy link
Copy Markdown
Collaborator Author

alexlamsl commented Jan 31, 2017

One extra bug fix:

  • [ a ].join() ➡️ "" + a (was a, which may not be a string)

Also improved string-type checking to output more optimal code, e.g. [a + "b", c].join("") now becomes a + "b" + c instead of "" + a + "b" + c

};
input: {
var a = [ null ].join();
var b = [ , ].join();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you please add another case with an array hole? [ ,1,,3 ].join()

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Added.

@kzc
Copy link
Copy Markdown
Contributor

kzc commented Jan 31, 2017

LGTM

fixes
- [a].join() => "" + a
- ["a", , "b"].join() => "a,,b"
- ["a", null, "b"].join() => "a,,b"
- ["a", undefined, "b"].join() => "a,,b"

improvements
- ["a", "b"].join(null) => "anullb"
- ["a", "b"].join(undefined) => "a,b"
- [a + "b", c].join("") => a + "b" + c
@alexlamsl
Copy link
Copy Markdown
Collaborator Author

@kzc thanks! Squashed and ready to go.

@kzc
Copy link
Copy Markdown
Contributor

kzc commented Feb 1, 2017

This was a longstanding bug in unsafe join - thanks for fixing it.

How did you come across this issue, I'm curious?

@alexlamsl
Copy link
Copy Markdown
Collaborator Author

I was combing through AST_Node.evaluate() usages in the code base when I stepped onto separator == null and thought "that was weird".

Then the rest is history.

alexlamsl added a commit to alexlamsl/UglifyJS that referenced this pull request Feb 18, 2017
fixes
- [a].join() => "" + a
- ["a", , "b"].join() => "a,,b"
- ["a", null, "b"].join() => "a,,b"
- ["a", undefined, "b"].join() => "a,,b"

improvements
- ["a", "b"].join(null) => "anullb"
- ["a", "b"].join(undefined) => "a,b"
- [a + "b", c].join("") => a + "b" + c

closes mishoo#1453
@alexlamsl alexlamsl closed this in 100307a Feb 23, 2017
@alexlamsl alexlamsl deleted the array-join branch February 24, 2017 00:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants