1426459 - [meta] Linting problems found in /mailnews

Status: RESOLVED DUPLICATE of bug 1515877

(Thunderbird :: Build Config, defect)

Description

[:aceman]

I managed to run the Javascript linter on /mailnews (it didn't work on /mail yet) and there are a ton of cosmetic problems (but maybe also some hidden bugs).
The run is here: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=24efe89244893a69d5343831a9a7a9b56d9a15c4

Fixing some of those could be material for some easy patches to land when nothing better is available :)

Some of the problem probably can't be easily fixed, the linter should be made aware of objects being imported from .jsm modules, or there are cases where .js files reference objects from other files, that are linked in the same .xul file. Of course the linter can't see those.

Comment 1

[:aceman]

Attachment: list of problems (5MB txt)

I attach the full list of problems in case the run log is removed from the server in the future.

Comment 2

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I think taking the calendar config as a start is a good idea since it hides some of the linting issues where m-c does it differently. I'd then suggest to disable all the rules and then to enable them one by one. One patch for each rule. devtools even had one bug per rule.

If you can script some of the changes that would help, note though that eslint --fix sometimes does more damage than good.

Comment 3

[:aceman]

Attachment: list of problems 2 (2.5MB txt)

More sane list of problems after turning off undefined identifiers warning, of which most were bogus.

Comment 4

[:aceman]

I would maybe propose a bug per file or directory where all warnings would be solved. In that way we could start enabling linting directories one by one. If we made patches for each rule, we could only enable linting after the end of the process.

The calendar config seems useful, but it seems that the current linter defaults are quite sane and match our style. I can try to make a patch for some folder manually to see if we need to enable some rule to fit our style. If there are rules in the calendar config that enable checks that are even stricter than the defaults, those are up for discussion.

Comment 5

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Here is a good overview, using the default config minus no-undef:

no-dupe-keys 1
no-extra-boolean-cast 1
no-native-reassign 1
no-unexpected-multiline 1
no-useless-call 1
no-mixed-spaces-and-tabs 2
no-empty 3
no-unreachable 3
no-control-regex 4
no-eval 4
space-unary-ops 6
consistent-return 7
no-empty-pattern 7
complexity 9
no-useless-concat 9
no-unneeded-ternary 12
func-call-spacing 16
mozilla/use-default-preference-values 18
no-nested-ternary 18
no-useless-return 21
no-lonely-if 26
comma-style 32
no-array-constructor 35
mozilla/no-useless-parameters 36
space-before-blocks 38
no-extra-semi 69
dot-notation 76
no-new-object 78
no-redeclare 78
no-else-return 99
comma-spacing 130
block-spacing 131
keyword-spacing 132
no-tabs 151
no-unused-vars 189
generator-star-spacing 213
no-multi-spaces 214
spaced-comment 290
no-trailing-spaces 374
key-spacing 656
space-before-function-paren 904
space-infix-ops 1215
object-shorthand 1748
quotes 2248
brace-style 3149


Using mach eslint --fix gets you down to this:

declared 1
no-dupe-keys 1
no-native-reassign 1
no-unexpected-multiline 1
no-useless-call 1
space-infix-ops 1
no-mixed-spaces-and-tabs 2
no-unneeded-ternary 2
no-empty 3
no-unreachable 3
dot-notation 4
no-control-regex 4
no-eval 4
space-before-function-paren 4
no-multi-spaces 6
spaced-comment 6
consistent-return 7
no-empty-pattern 7
object-shorthand 8
complexity 9
no-useless-concat 9
comma-spacing 10
no-lonely-if 10
no-trailing-spaces 15
no-nested-ternary 17
mozilla/use-default-preference-values 18
quotes 31
no-array-constructor 35
no-new-object 78
no-redeclare 78
brace-style 89
no-tabs 147
no-unused-vars 186

Using the calendar config after eslint --fix, you get this:

declared 1
no-catch-shadow 1
no-dupe-keys 1
no-floating-decimal 1
no-mixed-spaces-and-tabs 1
no-native-reassign 1
no-sequences 1
no-unexpected-multiline 1
no-useless-call 1
object-curly-newline 1
semi 1
yoda 1
no-div-regex 2
no-fallthrough 2
no-undef-init 2
no-unneeded-ternary 2
object-curly-spacing 2
no-confusing-arrow 3
no-empty 3
no-inner-declarations 3
no-unreachable 3
space-in-parens 3
array-bracket-spacing 4
dot-notation 4
no-control-regex 4
no-eval 4
no-unmodified-loop-condition 4
operator-linebreak 4
space-before-function-paren 4
dot-location 5
spaced-comment 6
consistent-return 7
consistent-this 7
no-empty-pattern 7
prefer-arrow-callback 8
no-useless-concat 9
no-lonely-if 10
prefer-spread 10
comma-spacing 11
no-multi-spaces 11
no-mixed-operators 14
no-return-assign 15
no-case-declarations 17
no-nested-ternary 17
mozilla/use-default-preference-values 18
no-trailing-spaces 19
no-unused-expressions 19
no-multi-str 21
block-spacing 22
no-useless-escape 29
quotes 31
radix 34
no-array-constructor 35
no-shadow 66
no-new-object 78
no-redeclare 78
no-negated-condition 109
no-tabs 117
curly 123
brace-style 226
object-shorthand 266
max-statements-per-line 292
id-length 624
block-scoped-var 740
func-names 1034
indent-legacy 1073
no-unused-vars 1107
mozilla/var-only-at-top-level 2423

It is up to Magnus to decide what the style would look like, as you can imagine I am biased towards the calendar style. I spent a bit of time going through all the rules and adjusting it to my liking.

Fixing the few issues after eslint --fix using the default config would probably get you a long way, then you can look into enabling more rules. You probably want to disable no-unused-vars for the time being as well.

Comment 6

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

By the way, this is what I used to count:

cat issues.txt | awk '/(eslint)/ { A[$(NF-1)]++ } END { for(i in A) print i,A[i] }' | sort -k 2 -n

Comment 7

[:aceman]

>It is up to Magnus to decide what the style would look like, as you can imagine I am biased towards the calendar style. I >spent a bit of time going through all the rules and adjusting it to my liking.

OK, if he can do it. But I can't decide which options we want just by reading them, whether the default ones or the calendar settings. I would go manually though the flagged problems and only if it flags something that we consider OK (or may discuss it), then we can turn off that particular option.
From glancing at the current set of warnings based on default settings it seems to me most of them are valid and we would want to fix them.

It may be calendar explicitly lists some settings with values that already are default.

>Fixing the few issues after eslint --fix using the default config would probably get you a long way, then you can look into >enabling more rules.

You discouraged from eslint --fix previously. But if we can inspect the resulting patch we could see what it does and stop it.

>You probably want to disable no-unused-vars for the time being as well.

Why?

Comment 8

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to :aceman from comment #7)
> >It is up to Magnus to decide what the style would look like, as you can imagine I am biased towards the calendar style. I >spent a bit of time going through all the rules and adjusting it to my liking.
> 
> OK, if he can do it. But I can't decide which options we want just by
> reading them, whether the default ones or the calendar settings. I would go
> manually though the flagged problems and only if it flags something that we
> consider OK (or may discuss it), then we can turn off that particular option.
> From glancing at the current set of warnings based on default settings it
> seems to me most of them are valid and we would want to fix them.
Of course, much easier to read if you see them in action. I'm just saying, if someone invests a lot of effort to change the style, we don't want to have to say "we're not taking this patch because we don't think the style should look like this".

That said, the default config looks fairly reasonable just from a skim of the rules, and especially those with < 30 instances are probably not a lot of work to fix.


> 
> It may be calendar explicitly lists some settings with values that already
> are default.
> 
> >Fixing the few issues after eslint --fix using the default config would probably get you a long way, then you can look into >enabling more rules.
> 
> You discouraged from eslint --fix previously. But if we can inspect the
> resulting patch we could see what it does and stop it.
I did, yes. It is definitely helpful, I'm just saying we can't rely on it point blank, instead you'll need to go through the patch it creates and verify that the changes are ok.

> 
> >You probably want to disable no-unused-vars for the time being as well.
> 
> Why?
Just because there are a huge number of instances and it makes more sense to start with the other rules. You'll have to check for each variable if it is used or not, and use /* exported */ quite a lot. When everything else is done, this is a good rule to enable.

Comment 9

[:aceman]

In case there is some linting left to do on /mailnews, you could finish this bug :)

Comment 10

[Geoff Lankow (:darktrojan)]

Some? Last time I checked there were 27000 errors. :-) I'll get there eventually.