Add an eslint rule to report an error when a get*Pref call is the only instruction in a try block

RESOLVED FIXED in Firefox 55

Status

enhancement
RESOLVED FIXED
2 years ago
5 months ago

People

(Reporter: florian, Assigned: florian)

Tracking

(Depends on 1 bug)

3 Branch
mozilla55
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(6 attachments)

Assignee

Description

2 years ago
Now that bug 1338306 landed, we can cleanup the tree.
Assignee

Comment 1

2 years ago
Attachment #8843964 - Flags: review?(jaws)
Assignee

Comment 2

2 years ago
Posted file xpcshell script
This will handle the most common cases, and try to do as little damage as possible.

It handles:

 let foo = 42;
 try {
   foo = branch.getIntPref("foo");
 } catch(e) {}

and

 try {
   foo = branch.getIntPref("foo");
 } catch(e) {
   foo = 42;
 }

If foo is not an identifier, or if 42 is not a literal or an identifier, it won't change the code.
Assignee

Comment 3

2 years ago
Attachment #8843967 - Flags: review?(jaws)
Assignee

Comment 4

2 years ago
Comment on attachment 8843967 [details] [diff] [review]
script-generated patch

When reviewing, I would suggest paying a bit more attention to getCharPref calls, because the default value can only be a string, so in some cases the rewrite will cause the result to be "" instead of null or undefined before.

When there's a null check on the result right after the getCharPref call, it won't many any difference. In a few cases it's less obvious that the change has no side effect, but I haven't seen anything too scary, and the green try run gives me some confidence.
Assignee

Comment 5

2 years ago
This includes things like:
- reverting the few SpecialPowers.get*Pref calls (the SpecialPowers pref API doesn't support default values)
- simplifying code like: let rv = branch.get*Pref(..., default); return rv;
- restoring meaningful comments that were in catch blocks and got nuked by the script.

I'm attaching this separately for easier review, so that the previous attachment is 100% script-generated, but my intent is to fold this into the previous attachment before landing to minimize the impact on blame.
Attachment #8843974 - Flags: review?(jaws)
Assignee

Comment 6

2 years ago
This patch is made by hand, it handles the cases that the script identified but didn't dare touching (due to a non-empty catch block, or due to the default value being more complicated than a litteral or an identifier).
Attachment #8843975 - Flags: review?(jaws)
Assignee

Comment 7

2 years ago
Handle the cases that remain to make the eslint rule happy.
Attachment #8843976 - Flags: review?(jaws)
Comment on attachment 8843964 [details] [diff] [review]
Add an eslint rule

Review of attachment 8843964 [details] [diff] [review]:
-----------------------------------------------------------------

::: tools/lint/eslint/eslint-plugin-mozilla/lib/index.js
@@ +53,5 @@
>      "no-useless-parameters": 0,
>      "no-useless-removeEventListener": 0,
>      "reject-importGlobalProperties": 0,
>      "reject-some-requires": 0,
> +    "use-default-preference-values": 0,

Is there a bug on file already to convert these to string names instead of integers? I think I mentioned this in a previous review for a different bug.
Attachment #8843964 - Flags: review?(jaws) → review+
Comment on attachment 8843967 [details] [diff] [review]
script-generated patch

Review of attachment 8843967 [details] [diff] [review]:
-----------------------------------------------------------------

rs=me (skimmed through the changes and looked closely at about 60% of them)
Attachment #8843967 - Flags: review?(jaws) → review+
Assignee

Comment 11

2 years ago
Thanks for the reviews!

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #9)

> ::: tools/lint/eslint/eslint-plugin-mozilla/lib/index.js
> @@ +53,5 @@
> >      "no-useless-parameters": 0,
> >      "no-useless-removeEventListener": 0,
> >      "reject-importGlobalProperties": 0,
> >      "reject-some-requires": 0,
> > +    "use-default-preference-values": 0,
> 
> Is there a bug on file already to convert these to string names instead of
> integers? I think I mentioned this in a previous review for a different bug.

I filed bug 1339042 as a result of bug 1338585 comment 4.
Assignee

Comment 12

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a36e117f41369e0045582d15401b765ad064a6b
Bug 1344711 - Add an eslint rule to report an error when a get*Pref call is the only instruction in a try block, r=jaws.

https://hg.mozilla.org/integration/mozilla-inbound/rev/bff53385b4941566f66332ddb51d339341069f32
Bug 1344711 - script-generated patch to remove try blocks around get*Pref calls, r=jaws.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c92165fef205c73e6ed11dcdd3f70c435ad3853
Bug 1344711 - hand cleanup of cases that were ignored by the script to remove try blocks around get*Pref calls, r=jaws.

https://hg.mozilla.org/integration/mozilla-inbound/rev/340e7f8a7229786fc8808d4a78983338997f81f3
Bug 1344711 - hand cleanup of remaining useless try blocks around get*Pref calls identified by eslint, r=jaws.

Updated

a year ago
Product: Testing → Firefox Build System
Version: Version 3 → 3 Branch
You need to log in before you can comment on or make changes to this bug.