Closed Bug 1571567 Opened 3 years ago Closed 3 years ago

Enable no-fallthrough rule in the recommended config

Categories

(Developer Infrastructure :: Lint and Formatting, task)

task
Not set
normal

Tracking

(firefox70 fixed)

RESOLVED FIXED
mozilla70
Tracking Status
firefox70 --- fixed

People

(Reporter: jaws, Assigned: jaws)

References

(Blocks 1 open bug)

Details

Attachments

(10 files, 2 obsolete files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review

This rule would have prevented bug 1570945.

Assignee: nobody → jaws
Status: NEW → ASSIGNED
Attachment #9083198 - Attachment description: Bug 1571567 - Fix errors in /toolkit after enabling the no-fallthrough eslint rule. r?mconley → Bug 1571567 - Fix no-fallthrough errors in /toolkit. r?mconley
Attachment #9083197 - Attachment description: Bug 1571567 - Fix no-fallthrough errors in /browser. r?k88hudson!,gijs! → Bug 1571567 - Fix no-fallthrough errors in /browser. r?k88hudson!,dao!
Attachment #9083198 - Attachment description: Bug 1571567 - Fix no-fallthrough errors in /toolkit. r?mconley → Bug 1571567 - Fix no-fallthrough errors in /toolkit. r?MattN
Attachment #9083454 - Attachment description: Bug 1571567 - Fix no-fallthrough errors in /docshell. → Bug 1571567 - Fix no-fallthrough errors in /docshell. r?Standard8
Attachment #9083455 - Attachment description: Bug 1571567 - Fix no-fallthrough errors in /dom. → Bug 1571567 - Fix no-fallthrough errors in /dom. r?smaug
Attachment #9083456 - Attachment description: Bug 1571567 - Fix no-fallthrough errors in /js. → Bug 1571567 - Fix no-fallthrough errors in /js. r?jorendorff
Attachment #9083457 - Attachment description: Bug 1571567 - Fix no-fallthrough errors in /layout. → Bug 1571567 - Fix no-fallthrough errors in /layout. r?dholbert
Attachment #9083458 - Attachment description: Bug 1571567 - Fix no-fallthrough errors in /netwerk. → Bug 1571567 - Fix no-fallthrough errors in /netwerk. r?Honza
Attachment #9083459 - Attachment description: Bug 1571567 - Fix no-fallthrough errors in /services. → Bug 1571567 - Fix no-fallthrough errors in /services. r?markh
Attachment #9083460 - Attachment description: Bug 1571567 - Fix no-fallthrough errors in /testing. → Bug 1571567 - Fix no-fallthrough errors in /testing. r?jmaher
Attachment #9083461 - Attachment is obsolete: true
Attachment #9083458 - Attachment description: Bug 1571567 - Fix no-fallthrough errors in /netwerk. r?Honza → Bug 1571567 - Fix no-fallthrough errors in /netwerk.
Attachment #9083197 - Attachment description: Bug 1571567 - Fix no-fallthrough errors in /browser. r?k88hudson!,dao! → Bug 1571567 - Fix no-fallthrough errors in /browser. r?k88hudson!
Attachment #9083458 - Attachment description: Bug 1571567 - Fix no-fallthrough errors in /netwerk. → Bug 1571567 - Fix no-fallthrough errors in /netwerk. r?mayhemer

I have to unassign myself from this bug. Mark, are you able to fix up any remaining issues and land this?

Flags: needinfo?(standard8)
Assignee: jaws → nobody
Status: ASSIGNED → NEW
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/606bafb8211c
Enable the no-fallthrough eslint rule. r=Standard8
https://hg.mozilla.org/integration/autoland/rev/7d5fc57b2809
Fix no-fallthrough errors in /browser. r=MattN,k88hudson
https://hg.mozilla.org/integration/autoland/rev/42df735c8556
Fix no-fallthrough errors in /toolkit. r=MattN
https://hg.mozilla.org/integration/autoland/rev/7ec086bb5bd5
Fix no-fallthrough errors in /docshell. r=Standard8
https://hg.mozilla.org/integration/autoland/rev/2c67015f12c6
Fix no-fallthrough errors in /dom. r=smaug
https://hg.mozilla.org/integration/autoland/rev/e2ed4620f232
Fix no-fallthrough errors in /js. r=jorendorff
https://hg.mozilla.org/integration/autoland/rev/fbdf6b75a484
Fix no-fallthrough errors in /layout. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/777d79076e99
Fix no-fallthrough errors in /netwerk. r=dragana
https://hg.mozilla.org/integration/autoland/rev/7aa97ba7cce9
Fix no-fallthrough errors in /services. r=markh
https://hg.mozilla.org/integration/autoland/rev/ce83fa75ae32
Fix no-fallthrough errors in /testing. r=jmaher
Pushed by rgurzau@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8f699e0bfbad
Fix test failures from enabling no-fallthrough. r=daleharvey CLOSED TREE

Backed out changeset 8f699e0bfbad (Bug 1571567) for es lint failure on browser/head.js

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=linting%2Copt%2Csource-test-mozlint-eslint%2C%28es%29%2C738873c683f243d90c2843e79eb3f4c7f7871045&fromchange=c44d035213c1b4e8f2293c69dbdde06cdeaa15a8&tochange=50d4fa5e5e09d705380c92764df96ca156d86595&selectedJob=260598465

Backout link: https://hg.mozilla.org/integration/autoland/rev/50d4fa5e5e09d705380c92764df96ca156d86595

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=260598465&repo=autoland&lineNumber=275

[task 2019-08-08T14:39:45.849Z] Error processing command. Ignoring because optional. (optional:packages.txt:comm/build/virtualenv_packages.txt)
[task 2019-08-08T14:53:54.511Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/mozapps/extensions/test/browser/head.js:1413:7 | Delete ?? (prettier/prettier)
[taskcluster 2019-08-08 14:53:55.576Z] === Task Finished ===
[taskcluster 2019-08-08 14:53:56.923Z] Unsuccessful task run with exit code: 1 completed in 1103.291 seconds

I'll also backout the initial push

Backout by btara@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c1bdc57b9b3e
Backed out 10 changesets complementary backout after es lint failure on the patch CLOSED TREE
Attachment #9084020 - Attachment is obsolete: true
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/328db867c177
Enable the no-fallthrough eslint rule. r=Standard8
https://hg.mozilla.org/integration/autoland/rev/1e7c298b1d68
Fix no-fallthrough errors in /browser. r=MattN,k88hudson
https://hg.mozilla.org/integration/autoland/rev/c0ba1b368b58
Fix no-fallthrough errors in /toolkit. r=MattN
https://hg.mozilla.org/integration/autoland/rev/eb557d4ba927
Fix no-fallthrough errors in /docshell. r=Standard8
https://hg.mozilla.org/integration/autoland/rev/e54f2402aa49
Fix no-fallthrough errors in /dom. r=smaug
https://hg.mozilla.org/integration/autoland/rev/cade7a6a7948
Fix no-fallthrough errors in /js. r=jorendorff
https://hg.mozilla.org/integration/autoland/rev/9e23c2da81e3
Fix no-fallthrough errors in /layout. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/2db0ad03c4af
Fix no-fallthrough errors in /netwerk. r=dragana
https://hg.mozilla.org/integration/autoland/rev/34e6203ba194
Fix no-fallthrough errors in /services. r=markh
https://hg.mozilla.org/integration/autoland/rev/381a12355930
Fix no-fallthrough errors in /testing. r=jmaher

I've fixed the issues and relanded, so hopefully good now. Re-assigning back to Jared as he did most of the work here.

Assignee: nobody → jaws
Status: NEW → ASSIGNED
Flags: needinfo?(standard8)

We're looking at doing the same for TB. Why is this OK to remove?
https://hg.mozilla.org/mozilla-central/rev/c0ba1b368b5880253f4c6a5b3deced3d7044cf19#l13.12

Flags: needinfo?(standard8)
Regressions: 1575197

Looks like Jared made changes andmarked that comment as done, so I didn't notice it when landing.

I think what happened here is that initially the break; was moved from inside the if to a higher level. When Matt commented, it then got changed from a break; to a fall-through without re-inserting the original break;.

This looks like just an optimisation to avoid unnecessary updates, but probably safer to put it back in for now - I've filed bug 1575197 to handle that.

Flags: needinfo?(standard8)

(In reply to Mark Banner (:standard8) from comment #23)

Looks like Jared made changes andmarked that comment as done, so I didn't notice it when landing.

I think what happened here is that initially the break; was moved from inside the if to a higher level. When Matt commented, it then got changed from a break; to a fall-through without re-inserting the original break;.

This looks like just an optimisation to avoid unnecessary updates, but probably safer to put it back in for now - I've filed bug 1575197 to handle that.

Yeah, that's what happened. Sorry about that and thank you for fixing it.

Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.