no-magic-number issues

[ilyavolodin]

I was working on fix for #4370 and I got very confused by the logic of this rule. Fix itself is easy to add, but I don't understand how some of the unittests are passing and I also don't understand what detectObjects supposed to do. detectObjects is very poorly tested and doesn't have a single valid test case. It's also clearly incorrect (as it will ignore any number who's parent has a type of AssignmentExpression which is the whole point of this rule). Can somebody explain what detectObjects supposed to do and why are the following unit tests passing:
https://github.com/eslint/eslint/blob/master/tests/lib/rules/no-magic-numbers.js#L34
https://github.com/eslint/eslint/blob/master/tests/lib/rules/no-magic-numbers.js#L38
https://github.com/eslint/eslint/blob/master/tests/lib/rules/no-magic-numbers.js#L45
https://github.com/eslint/eslint/blob/master/tests/lib/rules/no-magic-numbers.js#L60
https://github.com/eslint/eslint/blob/master/tests/lib/rules/no-magic-numbers.js#L63

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

[IanVS]

I'm going to take a wild guess here about why those tests are passing, but I have no real insider knowledge here. I don't think the point of the rule is to avoid assigning numbers to variables. In fact, the point of the rule is to force numbers to be assigned to variables, rather than be used directly in expressions throughout the code. I don't know what detectObjects is for, but I haven't actually looked at the code either.

• I think const foo = 42; has to pass because it is assigning to a const, presumably in order to avoid using the 42 later on in the code (thus avoiding a magic number).
• The same goes for the next test, because enforceConst is false.
• "var foo = -42;" passes because the es6 environment is not enabled, so it's not possible to use a const.
• var min, max, mean; min = 1; max = 10; mean = 4; seems to be testing assignment after declaration.
• var foo = { bar:10 } looks to be checking assignment inside of an object literal.

[ilyavolodin]

Ok, you are absolutely right about those tests. I lost the forest behind the trees, and it makes sense that assigning value to a variable should pass. So those unittests are passing correctly. Based on the code, however, I'm not sure why is var min, max, mean; min = 1; max = 10; mean = 4; passing, since the exception is made for VariableDeclarator and not for AssignmentExpression.
As far as I understand from the code detectObjects supposed to not flag following expressions var a = {}; a.9 = 'test' and var a = { 9: 'test' }. First one, however, is illegal syntax, and will not even parse, second one should be so incredibly rare, that I'm not sure what would be the point of creating a separate option for it. In the current implementation if detectObjects is on, it will also stop flagging any magic numbers in AssignmentExpression, which is very strange, since it will stop flagging them even if they are outside of the object declaration.

[ilyavolodin]

@kombucha any insights?

[nzakas]

These seem like they shouldn't be valid:

var min, max, mean; min = 1; max = 10; mean = 4;

var a = {}; a[9] = 'test';

The intent of detectObjects was to toggle allowing this:

var foo = {
    bar: 5
};

I agree, this is confusing. We should figure out a way to clean this up.

[nzakas]

@ilyavolodin any followup on this?

[ilyavolodin]

I'm not really sure what to do about those issues. I didn't participate in the original discussion and don't know the reasoning behind some of the decisions for this rule. Since we are not getting any response from the author, I think we basically have to re-write this rule and clean-up documentation and tests.

[alberto]

What do we want to do here?

var a = {}; a[9] = 'test';

This is not a valid test case, @ilyavolodin I think you might have gotten the behaviour backwards. @nzakas example is the correct one of what this is supposed to flag.

So, I think the only issue is if we want this rule to flag or not:

var min, max, mean; min = 1; max = 10; mean = 4;

[nzakas]

I think we do.

[alberto]

Accepting this as a bug, then.

[not-an-aardvark]

Working on this.