Bugzilla – Full Text Bug Listing
| Summary: | [quickfix] Offer a Quick Fix to replace a "new Array()" statement | ||
|---|---|---|---|
| Product: | [ECD] Orion | Reporter: | Steve Northover <steve_northover> |
| Component: | JS Tools | Assignee: | Michael Rennie <Michael_Rennie> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | enhancement | ||
| Priority: | P3 | CC: | mamacdon, Michael_Rennie, Silenio_Quarti |
| Version: | unspecified | ||
| Target Milestone: | 10.0 | ||
| Hardware: | PC | ||
| OS: | Windows 7 | ||
| Whiteboard: | |||
|
Description
Steve Northover
(In reply to Steve Northover from comment #0) > 1) Type in the following code: > > var digits = new Array (3); > > 2) You will get a warning about "new Array (3)" > > What should the code be for this case? There is confusion around "new > Array". > > new Array(3) - create an array with 3 undefined elements > new Array(3 2) - create with the elements [3, 2] > > Since indexing in JavaScript is loose, the "new Array(3)" is not really > needed, however, it captures the intent in the code that the array should be > of size 3 and allows for-loops to run on it over the length. What is the > "correct code" here? The "correct" code here is to always use array literal notation: var digits = []; You gain nothing from using the constructor, and can potentially introduce bugs using it. (In reply to Michael Rennie from comment #1) > You gain nothing from using the constructor, and can potentially introduce > bugs using it. That is true, but the fix doesn't have the same semantics as the code the user typed. The same code written properly would be either: > // Fix for `new Array(3)` > var digits = []; > for (var i = 0; i < 3; i++) { > digits[i] = undefined; > } Or in Steve's second case: > // Fix for `new Array(3, 2)` > var digits = [3, 2]; IMO if the fix actually re-wrote your bad code explicitly, it would make the behavior of new Array() clear to anyone who doesn't understand it, and then they could fix the resulting code if it didn't match their intent. var digits = n[]; is not the same as: var digits = new Array (3); The length of "digits" is 3 in the later case. There are situations when having an array of a given length with uninitialized values is valid. What is the proper way to achieve that? Mark's code in comment#2? Take a look at the documentation [1] and search for "arrayLength". [1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array The for loop would be very inefficient for large arrays. Another option is: var digits = []; digits.length = 3; But really that is not any better than the constructor call. (In reply to Silenio Quarti from comment #4) Or in your case, the "I know what I'm doing" option > /*eslint-disable no-new-array*/ (In reply to Mark Macdonald from comment #2) > That is true, but the fix doesn't have the same semantics as the code the > user typed. The same code written properly would be either: > Right, my sugestion for using array literal notation wasn't so much how steve should fix his code, rather the use ingeneral. Case one could be simplified as (for smaller arrays) var digits = [undefined, undefined, undefined]; > IMO if the fix actually re-wrote your bad code explicitly, it would make the > behavior of new Array() clear to anyone who doesn't understand it, and then > they could fix the resulting code if it didn't match their intent. This should be easy, a simple static analysis of the constructor params and creating the matching loops (if needed). We do something related to this for correcting sparse arrays. > There are situations when having an array of a given length with uninitialized values is valid. Personally I would disagree, I can't think of one useful case for creating an array and filling it with undefined (or null for sparse arrays). Can you give me a real world example where this is beneficial compared to simply creating an empty array? (In reply to Mark Macdonald from comment #5) > (In reply to Silenio Quarti from comment #4) > > Or in your case, the "I know what I'm doing" option > > > /*eslint-disable no-new-array*/ Its been on my backburner for a while to create a quick-fix for every problem that allows a user to ignore the problems from a given rule (in-file). But my fear was that devs would start using it to 'remove' the problems from their source without actually fixing them :) (In reply to Michael Rennie from comment #7) > Can you give me a real world example where this is beneficial compared to > simply creating an empty array? Try this -- http://jsperf.com/new-array-vs-literal-1000-elements Setting the initial capacity with `new Array(n)` or `array.length = n` does seem to make a difference when filling a large array. For code like textView.js or textModel.js, when we do operations like this > new Array(lineCount) The difference could be significant when lineCount is very large. (In reply to Mark Macdonald from comment #8) > (In reply to Michael Rennie from comment #7) > > > Can you give me a real world example where this is beneficial compared to > > simply creating an empty array? > > Try this -- http://jsperf.com/new-array-vs-literal-1000-elements > > Setting the initial capacity with `new Array(n)` or `array.length = n` does > seem to make a difference when filling a large array. For code like > textView.js or textModel.js, when we do operations like this > > > new Array(lineCount) > > The difference could be significant when lineCount is very large. Makes sense, thanks for pointing that out. I didn't know we used arrays like that in our text stuff. Bottom line? "Array (n) can make sense?" (In reply to Steve Northover from comment #10) > Bottom line? "Array (n) can make sense?" Yeah, for really large arrays the jsperf is saying it performs better. I think Mark's suggestion for the fix would make the most sense. Then devs can see what the code looks like and as he noted, maybe change what they were wanting to do. Can we say that "new Array(n)" is a good pattern while things like "new Array(n, m) etc." is bad? I guess that would be redefining what eslint says and that would be a bad thing. We don't want to disable all new-array's though for every place in the file. (In reply to Steve Northover from comment #12) > Can we say that "new Array(n)" is a good pattern while things like "new > Array(n, m) etc." is bad? I guess that would be redefining what eslint says > and that would be a bad thing. > > We don't want to disable all new-array's though for every place in the file. Looks like to once again match upstream ESLint, we should not even be marking the single param case: http://eslint.org/docs/rules/no-array-constructor.html Do we just need to update eslint to a better version? (In reply to Steve Northover from comment #14) > Do we just need to update eslint to a better version? In this case, no, we have to update the code in our version of the rule. (In reply to Michael Rennie from comment #15) > (In reply to Steve Northover from comment #14) > > Do we just need to update eslint to a better version? > > In this case, no, we have to update the code in our version of the rule. Updated our rule to match the upstream rule (plus tests): http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=efef883af58deb9e20af6857b69bee51535bc3a3 Added quick fix (plus tests) to turn new expression and call expression arrays into literal notation.
For example:
var ar = new Array('a'); -> var ar = ['a'];
var ar = new Array(1, 2, 3); -> var ar = [1, 2, 3];
var ar = new Array(1, 'd', {}); -> var ar = [1, 'd', {}];
var ar = Array('a'); -> var ar = ['a'];
var ar = Array(1, 2, 3); -> var ar = [1, 2, 3];
var ar = Array(1, 'd', {}); -> var ar = [1, 'd', {}];
(In reply to Michael Rennie from comment #17) > Added quick fix (plus tests) to turn new expression and call expression > arrays into literal notation. > The commit: http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=3530b74cd0c9487f63590c29b9af30e05da6a2d6 |