Fix: radix rule crash on disabled globals

[mdjermanovic]

Prerequisites checklist

• I have read the contributing guidelines.
• The team has reached consensus on the changes proposed in this pull request. If not, I understand that the evaluation process will begin with this pull request and won't be merged until the team has reached consensus.

What is the purpose of this pull request? (put an "X" next to item)

[X] Bug fix

Tell us about your environment

• ESLint Version: 7.0.0-alpha.0
• Node Version: v12.14.0
• npm Version: v6.13.4

What parser (default, Babel-ESLint, etc.) are you using?

default

Please show your full configuration:

Configuration

module.exports = {
    parserOptions: {
        ecmaVersion: 2015
    },
};

What did you do? Please include the actual source code causing the issue.

/* eslint radix: error */
/* globals parseInt: off */

Online Demo Link (v6.8.0 actual)

What did you expect to happen?

Not to crash.

What actually happened? Please include the actual, raw output from ESLint.

TypeError: Cannot read property 'defs' of null

What changes did you make? (Give an overview)

Fixed the radix rule to check if the global variables parseInt/Number exist.

If not, the rule will not crash (and also will not report errors).

Is there anything you'd like reviewers to focus on?

[mdjermanovic]

It seems that the rules are not consistent on what to do if the targeted global doesn't exist as an ESLint global variable. Some rules do check unresolved references with the targeted name, some rules don't.

I'm not sure what's better to do in this rule, parseInt and Number should de facto exist in runtime.

[yeonjuan]

LGTM Thanks. 👍

It looks definitely a bug.

Some rules do check unresolved references with the targeted name, some rules don't.

I checked all other rules which use astUtils.getVariableByName(). And it seems radix is the only one which does not check unresolved references.

checked rules - using astUtils.getVariableByName()

• symbol-description
• no-catch-shadow
• no-undef-init
• no-label-var
• no-eval
• no-console
• no-regex-spaces
• no-shadow
• no-unmodified-loop-condition
• prefer-const

[kaicataldo]

@mdjermanovic @yeonjuan Really appreciate you two exploring all these inconsistencies. Inconsistency between rules is definitely confusing, and it's great to standardize our approach as much as possible!

Some other thoughts on this topic as we explore making things more consistent (not necessarily relevant to this PR specifically):

1. In the past, we have deprecated rules in favor of new rules if we feel like the API needs to be changed in a breaking way. This has allowed us to combine multiple rules that feel similar into one more all-encompassing rule as well. This allows folks to have time to transition, since we don't remove deprecated rules. Wanted to let you know this is an option!
2. Making behavior and configuration more consistent lessens the burden on the end user significantly and I think this is a big win for the project!

[mdjermanovic]

@yeonjuan I found no-new-symbol, which has a similar code like radix but just gets the variable from the global scope (astUtils.getVariableByName isn't actually necessary here because there are no upper scopes). And this is also the case with all rules that use ReferenceTracker

It looks like search won't be helpful enough, we'll have to check from the list of all rules :)

[mdjermanovic]

Just to mention some thoughts, although completely unrelated to this PR and global variables.

In the past, we have deprecated rules in favor of new rules if we feel like the API needs to be changed in a breaking way. This has allowed us to combine multiple rules that feel similar into one more all-encompassing rule as well. This allows folks to have time to transition, since we don't remove deprecated rules. Wanted to let you know this is an option!

The 5 naming convention rules are a good candidate for this: camelcase, id-blacklist, id-length, id-match, no-underscore-dangle. There is a lot of inconsistency between them and also a lot of bugs in each of them.

I was thinking about a proposal for a new rule to combine them all (or at least to unify or consolidate the logic in the existing rules if it wouldn't be a breaking change for some of them), but it will take some time to prepare, mostly to find out how to retain all the existing features.

One rule, or at least shared logic in one place, should be also helpful for the maintenance. It seems that the hard part is to determine which of the identifiers nodes should be checked and which shouldn't. Once it is known, the easy part is to check whether the variable names have appropriate length, have underscores, are blacklisted etc.