Closed Bug 1571567 Opened 2 months ago Closed 2 months ago

Enable no-fallthrough rule in the recommended config

Categories

(Firefox Build System :: Lint and Formatting, task)

task
Not set

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.

You need to log in before you can comment on or make changes to this bug.