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 ES6 polyfills to use ToInteger and ToLength #3216

Closed
wants to merge 1 commit into from
Closed
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
24 changes: 6 additions & 18 deletions src/com/google/javascript/jscomp/js/es6/array/copywithin.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,9 @@
*/

'require util/polyfill';
'require util/tointeger';

$jscomp.polyfill('Array.prototype.copyWithin', function(orig) {
// requires strict mode to throw for invalid `this` or params
'use strict';

if (orig) return orig;

/**
Expand All @@ -33,10 +31,12 @@ $jscomp.polyfill('Array.prototype.copyWithin', function(orig) {
* @template VALUE
*/
var polyfill = function(target, start, opt_end) {
// TODO(tjgq): requires strict mode, lost in transpilation (b/24413211)
'use strict';
var len = this.length;
target = toInteger(target);
start = toInteger(start);
var end = opt_end === undefined ? len : toInteger(opt_end);
target = $jscomp.toInteger(target);
start = $jscomp.toInteger(start);
var end = opt_end === undefined ? len : $jscomp.toInteger(opt_end);
var to = target < 0 ? Math.max(len + target, 0) : Math.min(target, len);
var from = start < 0 ? Math.max(len + start, 0) : Math.min(start, len);
var final = end < 0 ? Math.max(len + end, 0) : Math.min(end, len);
Expand All @@ -63,17 +63,5 @@ $jscomp.polyfill('Array.prototype.copyWithin', function(orig) {
return this;
};

/**
* @param {number} arg
* @return {number}
*/
function toInteger(arg) {
var n = Number(arg);
if (n === Infinity || n === -Infinity) {
return n;
}
return n | 0;
}

return polyfill;
}, 'es6', 'es3');
21 changes: 14 additions & 7 deletions src/com/google/javascript/jscomp/js/es6/array/fill.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
*/

'require util/polyfill';
'require util/tointeger';
'require util/tolength';

$jscomp.polyfill('Array.prototype.fill', function(orig) {
if (orig) return orig;
Expand All @@ -31,14 +33,19 @@ $jscomp.polyfill('Array.prototype.fill', function(orig) {
* @suppress {reportUnknownTypes, strictPrimitiveOperators}
*/
var polyfill = function(value, opt_start, opt_end) {
var length = this.length || 0;
if (opt_start < 0) {
opt_start = Math.max(0, length + /** @type {number} */ (opt_start));
'use strict';
var length = $jscomp.toLength(this.length);
var start = $jscomp.toInteger(opt_start);
if (start < 0) {
start = Math.max(0, length + start);
}
if (opt_end == null || opt_end > length) opt_end = length;
opt_end = Number(opt_end);
if (opt_end < 0) opt_end = Math.max(0, length + opt_end);
for (var i = Number(opt_start || 0); i < opt_end; i++) {
var end = opt_end === undefined ? length : $jscomp.toInteger(opt_end);
Copy link
Member

@shicks shicks Mar 4, 2019

Choose a reason for hiding this comment

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

There's a lot of repetitive code here and in other files. What do you think about any of the following:

  • $jscomp.toInteger takes a second optional argument for what to return on undefined (may not be quite as smooth as just checking isNaN tho since we still want NaN to map to zero even if undefined maps to length - so not sure if it ends up being worth it).
  • add a $jscomp.toPosition(index, length) that handles negative indices and clamping all in one place (but would be nice to handle undefined here as well, and sometimes that's 0 and sometimes it's length - but at least we could just call $jscomp.toPosition(opt_start !== undefined ? opt_start : length, length))

if (end < 0) {
end = Math.max(0, length + end);
} else if (end > length) {
end = length;
}
for (var i = start; i < end; i++) {
this[i] = value;
}
return this;
Expand Down
10 changes: 8 additions & 2 deletions src/com/google/javascript/jscomp/js/es6/array/includes.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@

'require es6/object/is';
'require util/polyfill';
'require util/tointeger';
'require util/tolength';

$jscomp.polyfill('Array.prototype.includes', function(orig) {
if (orig) return orig;
Expand All @@ -33,12 +35,16 @@ $jscomp.polyfill('Array.prototype.includes', function(orig) {
* @suppress {reportUnknownTypes}
*/
var includes = function(searchElement, opt_fromIndex) {
'use strict';
var array = this;
if (array instanceof String) {
array = /** @type {!IArrayLike} */ (String(array));
}
var len = array.length;
var i = opt_fromIndex || 0;
var len = $jscomp.toLength(array.length);
if (len === 0) {
return false;
}
var i = $jscomp.toInteger(opt_fromIndex);
if (i < 0) {
i = Math.max(i + len, 0);
}
Expand Down
6 changes: 2 additions & 4 deletions src/com/google/javascript/jscomp/js/es6/string/codepointat.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

'require util/checkstringargs';
'require util/polyfill';
'require util/tointeger';

$jscomp.polyfill('String.prototype.codePointAt', function(orig) {
if (orig) return orig;
Expand All @@ -34,13 +35,10 @@ $jscomp.polyfill('String.prototype.codePointAt', function(orig) {
'use strict';
var string = $jscomp.checkStringArgs(this, null, 'codePointAt');
var size = string.length;
// Make 'position' a number (non-number coerced to NaN and then or to zero).
position = Number(position) || 0;
position = $jscomp.toInteger(position);
if (!(position >= 0 && position < size)) {
return void 0;
}
// Truncate 'position' to an integer.
position = position | 0;
var first = string.charCodeAt(position);
if (first < 0xD800 || first > 0xDBFF || position + 1 === size) {
return first;
Expand Down
5 changes: 3 additions & 2 deletions src/com/google/javascript/jscomp/js/es6/string/endswith.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

'require util/checkstringargs';
'require util/polyfill';
'require util/tointeger';

$jscomp.polyfill('String.prototype.endsWith', function(orig) {
if (orig) return orig;
Expand All @@ -34,8 +35,8 @@ $jscomp.polyfill('String.prototype.endsWith', function(orig) {
'use strict';
var string = $jscomp.checkStringArgs(this, searchString, 'endsWith');
searchString = searchString + '';
if (opt_position === void 0) opt_position = string.length;
var i = Math.max(0, Math.min(opt_position | 0, string.length));
var end = opt_position === void 0 ? string.length : $jscomp.toInteger(opt_position);
var i = Math.max(0, Math.min(end, string.length));
var j = searchString.length;
while (j > 0 && i > 0) {
if (string[--i] != searchString[--j]) return false;
Expand Down
8 changes: 5 additions & 3 deletions src/com/google/javascript/jscomp/js/es6/string/padend.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,9 @@
*/

'require util/checkstringargs';
'require util/stringpadding';
'require util/polyfill';
'require util/stringpadding';
'require util/tolength';

$jscomp.polyfill('String.prototype.padEnd', function(orig) {
if (orig) return orig;
Expand All @@ -32,8 +33,9 @@ $jscomp.polyfill('String.prototype.padEnd', function(orig) {
* @return {string}
*/
var padEnd = function(targetLength, opt_padString) {
var string = $jscomp.checkStringArgs(this, null, 'padStart');
var padLength = targetLength - string.length;
'use strict';
var string = $jscomp.checkStringArgs(this, null, 'padEnd');
var padLength = $jscomp.toLength(targetLength) - string.length;
return string + $jscomp.stringPadding(opt_padString, padLength);
};

Expand Down
4 changes: 3 additions & 1 deletion src/com/google/javascript/jscomp/js/es6/string/padstart.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
'require util/checkstringargs';
'require util/polyfill';
'require util/stringpadding';
'require util/tolength';

$jscomp.polyfill('String.prototype.padStart', function(orig) {
if (orig) return orig;
Expand All @@ -32,8 +33,9 @@ $jscomp.polyfill('String.prototype.padStart', function(orig) {
* @return {string}
*/
var padStart = function(targetLength, opt_padString) {
'use strict';
var string = $jscomp.checkStringArgs(this, null, 'padStart');
var padLength = targetLength - string.length;
var padLength = $jscomp.toLength(targetLength) - string.length;
return $jscomp.stringPadding(opt_padString, padLength) + string;
};

Expand Down
3 changes: 2 additions & 1 deletion src/com/google/javascript/jscomp/js/es6/string/repeat.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

'require util/checkstringargs';
'require util/polyfill';
'require util/tointeger';

$jscomp.polyfill('String.prototype.repeat', function(orig) {
if (orig) return orig;
Expand All @@ -32,10 +33,10 @@ $jscomp.polyfill('String.prototype.repeat', function(orig) {
var polyfill = function(copies) {
'use strict';
var string = $jscomp.checkStringArgs(this, null, 'repeat');
copies = $jscomp.toInteger(copies);
if (copies < 0 || copies > 0x4FFFFFFF) { // impose a 1GB limit
throw new RangeError('Invalid count value');
}
copies = copies | 0; // cast to a signed integer.
var result = '';
while (copies) {
if (copies & 1) result += string;
Expand Down
3 changes: 2 additions & 1 deletion src/com/google/javascript/jscomp/js/es6/string/startswith.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

'require util/checkstringargs';
'require util/polyfill';
'require util/tointeger';

$jscomp.polyfill('String.prototype.startsWith', function(orig) {
if (orig) return orig;
Expand All @@ -38,7 +39,7 @@ $jscomp.polyfill('String.prototype.startsWith', function(orig) {
var searchLen = searchString.length;
var i = Math.max(
0,
Math.min(/** @type {number} */ (opt_position) | 0, string.length));
Math.min($jscomp.toInteger(opt_position), string.length));
var j = 0;
while (j < searchLen && i < strLen) {
if (string[i++] != searchString[j++]) return false;
Expand Down
31 changes: 31 additions & 0 deletions src/com/google/javascript/jscomp/js/util/tointeger.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/*
* Copyright 2019 The Closure Compiler Authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

/**
* Converts argument to an integral numeric value.
*
* @see https://www.ecma-international.org/ecma-262/9.0/#sec-tointeger
*
* @param {*} arg
* @return {number}
*/
$jscomp.toInteger = function(arg) {
var n = Number(arg);
if (isNaN(n)) {
return 0;
}
return n > 0 ? Math.floor(n) : Math.ceil(n);
};
33 changes: 33 additions & 0 deletions src/com/google/javascript/jscomp/js/util/tolength.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/*
* Copyright 2019 The Closure Compiler Authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

'require util/tointeger';

/**
* Converts argument to an integer suitable for use as the length of an array-like object.
*
* @see https://www.ecma-international.org/ecma-262/9.0/#sec-tolength
*
* @param {*} arg
* @return {number}
*/
$jscomp.toLength = function(arg) {
var len = $jscomp.toInteger(arg);
if (len < 0) {
return 0;
}
return Math.min(len, Math.pow(2, 53) - 1);
};
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,19 @@ testSuite({
assertObjectEquals([1, 3, 4, 4, 5], [1, 2, 3, 4, 5].copyWithin(1, 2, -1));
},

testCopyWithin_infinity() {
assertObjectEquals([1, 2, 3, 4, 5], [1, 2, 3, 4, 5].copyWithin(Infinity, 1));
assertObjectEquals([1, 2, 3, 4, 5], [1, 2, 3, 4, 5].copyWithin(1, Infinity));
assertObjectEquals([2, 3, 4, 5, 5], [1, 2, 3, 4, 5].copyWithin(0, 1, Infinity));
},

testCopyWithin_over32bit() {
var n = 2147483648; // 2**31
assertObjectEquals([1, 2, 3, 4, 5], [1, 2, 3, 4, 5].copyWithin(n, 1));
assertObjectEquals([1, 2, 3, 4, 5], [1, 2, 3, 4, 5].copyWithin(1, n));
assertObjectEquals([2, 3, 4, 5, 5], [1, 2, 3, 4, 5].copyWithin(0, 1, n));
},

testCopyWithin_throwsIfNullish() {
// TODO(tjgq): requires strict mode, lost in transpilation (b/24413211)
// assertThrows(function() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,12 @@ testSuite({
assertObjectEquals([1, 2, 3], fill([1, 2, 3], 9, NaN, NaN));
assertObjectEquals([1, 2, 3], fill([1, 2, 3], 9, 1, NaN));
assertObjectEquals([9, 2, 3], fill([1, 2, 3], 9, NaN, 1));

assertObjectEquals([1, 1], fill([0, 0], 1, null));
assertObjectEquals([0, 0], fill([0, 0], 1, 0, null));

assertObjectEquals([0, 1], fill([0, 0], 1, 1.1));
assertObjectEquals([1, 0], fill([0, 0], 1, 0, 1.1));
},

testFill_arrayLike() {
Expand All @@ -71,6 +77,12 @@ testSuite({
assertObjectEquals({0: 'z', 1: 'z', 2: 'y', 3: 'safe', length: 3}, arr);
},

testFill_arrayLikeCoerced() {
const arr = {length: 1.5, 0: 0, 1: 0};
assertEquals(arr, Array.prototype.fill.call(arr, 'y'));
assertObjectEquals({0: 'y', 1: 0, length: 1.5}, arr);
},

testFill_notArrayLike() {
const arr = {2: 'x'}; // does nothing if no length
assertEquals(arr, Array.prototype.fill.call(noCheck(arr), 'y', 0, 4));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,13 @@ testSuite({
assertFalse(arr.includes(0, -3));
assertTrue(arr.includes(0, -4));
assertTrue(arr.includes(0, -5));
assertTrue(arr.includes(1, true));
var obj1 = {
valueOf: function() {
return 1;
}
};
assertTrue(arr.includes(1, obj1));

// null and undefined
// Make sure the compiler knows both null and undefined are allowed
Expand Down Expand Up @@ -77,5 +84,29 @@ testSuite({
assertTrue(Array.prototype.includes.call(arr, 6));
assertFalse(Array.prototype.includes.call(arr, 5, 1));
assertFalse(Array.prototype.includes.call(arr, 7));

// length
arr = {length: 0.1, 0: 5, 1: 6};
assertFalse(Array.prototype.includes.call(obj, 5));

// length boundary
var fromIndex = 9007199254740990; // 2 ** 53 - 2
arr = {
9007199254740990: 5,
9007199254740991: 6
};
arr.length = 9007199254740991;
assertTrue(Array.prototype.includes.call(arr, 5, fromIndex));
assertFalse(Array.prototype.includes.call(arr, 6, fromIndex));

arr.length = 9007199254740992;
assertTrue(Array.prototype.includes.call(arr, 5, fromIndex));
assertFalse(Array.prototype.includes.call(arr, 6, fromIndex));

// nullish this
// TODO(tjgq): requires strict mode, lost in transpilation (b/24413211)
// assertThrows(function() {
// Array.prototype.includes.call(null);
// });
},
});
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ testSuite({
assertUndefined('abc'.codePointAt(Infinity));
assertUndefined('abc'.codePointAt(-Infinity));
assertUndefined('abc'.codePointAt(-1));
assertUndefined('abc'.codePointAt(Math.power(2, 32)));

assertEquals(0x31, String.prototype.codePointAt.call(noCheck(14), 0));
assertEquals(
Expand Down
Loading