Closed Bug 1230369 Opened 9 years ago Closed 6 years ago

Add an ESLint rule/configuration to disallow definition of Cc, Ci etc or Components.classes, Components.interfaces etc

Categories

(Developer Infrastructure :: Lint and Formatting, defect, P2)

Product:
Developer Infrastructure
Component:
Lint and Formatting
Type:
defect

Tracking

(firefox60 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: MattN, Assigned: standard8)

References

Details

Attachments

(2 files)

Bug 1230369 - Add ESLint rules to disallow defining Cc/Ci etc and to prefer the use of Cc/Ci rather than the Components.* equivalents.
59 bytes, text/x-review-board-request
florian
: review+
Details
Bug 1230369 - Manually fix some remaining instances of Ci/Cc/Cu definitions and switch Components.* to using the shorthand in some places.
59 bytes, text/x-review-board-request
florian
: review+
Details 
In reviews we tell developers to use Cc/Ci/Cu/Cr/etc. instead of the more verbose Components.* so we should enforce this with linting.
provided we fix bug 406371 (that is, fix bug 767640), or the linting plugin is able to figure if it's not in the browser global object.
Component: General → ESLint
Depends on: 767640
P5 until we can get some of the blocking issues resolved.
Priority: -- → P5
Severity: enhancement → normal
Blocks: 1433175
Bug 767640 & co are in progress, so bumping priority for this.
Priority: P5 → P2
I'm looking into options for this. We might be able to do this with some config options for existing ESLint rules, but I'm not sure just yet.
Assignee: nobody → standard8
Depends on: 1432992
Summary: Add an eslint rule if Components.* is used instead of the shortened alias Cc/Ci/Cu/Cr/etc. → Add an ESLint rule/configuration to disallow definition of Cc, Ci etc or Components.classes, Components.interfaces etc
I've dug around a bit. I'd like to change our config to use no-redeclare's builtinGlobals option:

    "no-redeclare": ["error", {"builtinGlobals": true}],

https://eslint.org/docs/rules/no-redeclare#builtinglobals

However, it looks like we currently have lots of false positives with this, some to do with how we currently handle globals. I'm also a bit wary that this is the right thing to do. 

In any case, I think the best thing to do here is to create a rule that blocks the use of the ones we're really interested in, and then think about no-redeclare later.
The rules I've just uploaded should be working fine. I still need to finish documenting & wait for bug 1432992 to land in central. Then I'll sort out whitelists etc for parts that haven't been removed yet.
Blocks: 1436303
Comment on attachment 8948933 [details]
Bug 1230369 - Manually fix some remaining instances of Ci/Cc/Cu definitions and switch Components.* to using the shorthand in some places.

https://reviewboard.mozilla.org/r/218356/#review224152

These changes look good, but I don't see why browser/base/content/test/general/browser_bug567306.js wasn't handled by the script. Was it maybe causing some unexplained test failures? dbg-actors.js was mentioned in bug 1432992 comment 10.
Attachment #8948933 - Flags: review?(florian) → review+
Comment on attachment 8948824 [details]
Bug 1230369 - Add ESLint rules to disallow defining Cc/Ci etc and to prefer the use of Cc/Ci rather than the Components.* equivalents.

https://reviewboard.mozilla.org/r/218198/#review224156

Looks good. r=me with the following trivial comments addressed. Thanks for doing this! :-)

::: tools/lint/docs/linters/eslint-plugin-mozilla.rst:197
(Diff revision 3)
> +This disallows statements such as:
> +
> +.. code-block:: js
> +
> +   var Cc = Components.classes;
> +   var Ci = Components.interfaces;

Should we include the var {Ci: interfaces, Cc: classes, Cu: utils} = Components; form in the example too?

::: tools/lint/eslint/eslint-plugin-mozilla/lib/configs/mochitest-test.js:51
(Diff revision 3)
>    "rules": {
>      "mozilla/import-content-task-globals": "error",
>      "mozilla/import-headjs-globals": "error",
>      "mozilla/mark-test-function-used": "error",
> +    // Turn off no-define-cc-etc for mochitests as these don't have Cc etc defined in the
> +    // global scope.

Is this configuration file applying to mochitest plain but not mochitest-bc?

::: tools/lint/eslint/eslint-plugin-mozilla/lib/index.js:82
(Diff revision 3)
>      "import-headjs-globals": "off",
>      "mark-test-function-used": "off",
>      "no-aArgs": "off",
>      "no-arbitrary-setTimeout": "off",
>      "no-cpows-in-tests": "off",
> +    "no-define-cc-etc": "off",

We've been doing this for all eslint rule additions, but I'm not clear on the use of this list. What would break if we just removed it? What breaks when something is in the "rules" list but not the "rulesConfig" list? (eg. mark-exported-symbols-as-used). If it's not require, I would like to remove it to simplify adding more eslint rules.

::: tools/lint/eslint/eslint-plugin-mozilla/lib/rules/no-define-cc-etc.js:15
(Diff revision 3)
> +
> +// -----------------------------------------------------------------------------
> +// Rule Definition
> +// -----------------------------------------------------------------------------
> +
> +const componentsBlacklist = [ "Cc", "Ci", "Cr", "Cu"];

nit: it's strange to have a space after [ but not before ]. I would remove the space to make the spacing consistent.

::: tools/lint/eslint/eslint-plugin-mozilla/tests/no-define-cc-etc.js:28
(Diff revision 3)
> +  }]};
> +}
> +
> +ruleTester.run("no-define-cc-etc", rule, {
> +  valid: [
> +    "var Cm = Components.modules;",

I would prefer to test this with properties that do exist:
Components.manager
Components.Constructor

::: tools/lint/eslint/eslint-plugin-mozilla/tests/no-define-cc-etc.js:29
(Diff revision 3)
> +}
> +
> +ruleTester.run("no-define-cc-etc", rule, {
> +  valid: [
> +    "var Cm = Components.modules;",
> +    "var {Cm} = Components.modules;",

This doesn't seem valid to me. This would be:
var {CC: Constructor, Cm: manager} = Components;

I would include a valid case with 'const' instead of 'var'.

::: tools/lint/eslint/eslint-plugin-mozilla/tests/no-define-cc-etc.js:33
(Diff revision 3)
> +    "var Cm = Components.modules;",
> +    "var {Cm} = Components.modules;",
> +    "foo.Cc.test();"
> +  ],
> +  invalid: [
> +    invalidCode("var Cc;", "Cc"),

I would like the invalid examples to be stuff that used to be valid and that we are removing:

var Cc = Components.classes;
const Cu = Components.utils;
var {Ci: interfaces, Cc: classes} = Components;
Attachment #8948824 - Flags: review?(florian) → review+
(In reply to Florian Quèze [:florian] from comment #12)
> These changes look good, but I don't see why
> browser/base/content/test/general/browser_bug567306.js wasn't handled by the
> script. Was it maybe causing some unexplained test failures?

I'd assumed it was to do with the specific format used or something.
Comment on attachment 8948824 [details]
Bug 1230369 - Add ESLint rules to disallow defining Cc/Ci etc and to prefer the use of Cc/Ci rather than the Components.* equivalents.

https://reviewboard.mozilla.org/r/218198/#review224156

> Is this configuration file applying to mochitest plain but not mochitest-bc?

Yes. Mochitest-bc is "browser-test.js". Thinking about it, it isn't 100% ideal as some test folders contain plain + bc tests mixed together, but I guess it'll do for now.

> We've been doing this for all eslint rule additions, but I'm not clear on the use of this list. What would break if we just removed it? What breaks when something is in the "rules" list but not the "rulesConfig" list? (eg. mark-exported-symbols-as-used). If it's not require, I would like to remove it to simplify adding more eslint rules.

This section is now unused according to https://eslint.org/docs/user-guide/migrating-to-2.0.0#plugins-no-longer-have-default-configurations, so I'm dropping it.
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/76c1582d760e
Manually fix some remaining instances of Ci/Cc/Cu definitions and switch Components.* to using the shorthand in some places. r=florian
https://hg.mozilla.org/integration/autoland/rev/f58c1a70c026
Add ESLint rules to disallow defining Cc/Ci etc and to prefer the use of Cc/Ci rather than the Components.* equivalents. r=florian
(In reply to Florian Quèze [:florian] from comment #12)
> These changes look good, but I don't see why
> browser/base/content/test/general/browser_bug567306.js wasn't handled by the
> script. Was it maybe causing some unexplained test failures?

That is odd. Maybe the file has DOS line endings and my script didn't handle that? Oh well. It wasn't an intentional omission.
https://hg.mozilla.org/mozilla-central/rev/76c1582d760e
https://hg.mozilla.org/mozilla-central/rev/f58c1a70c026
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Product: Testing → Firefox Build System
Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: