1326071 - [meta][eslint] Reduce the maximum cyclomatic complexity level

Status: NEW

(Toolkit :: General, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

Right now 'complexity' shows one warning in WebRequest.jsm's runChannelListener function (complexity=22).

This function will need to be refactored before the rule can be enabled.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: eslint error log

There are actually 104 functions that are above the default complexity of 20.

Comment 2

[Mark Banner (:standard8)]

Now that bug 1347884 has landed, we have some maximums landed for various areas. We should work to get those down to reasonable levels - e.g. the default is 20, but devtools is 35. Aiming for 35 in the first instance might be a good start.

Comment 3

[Dão Gottwald [:dao]]

Can you elaborate on how the default of 20 is reasonable for our code base?

In bug 1352069 we hit the limit of 70 (!) in nsBrowserGlue.js::BG__migrateUI and that method isn't particularly complex, just a bit long in a way that didn't seem to bother anyone.

http://eslint.org/docs/rules/complexity says: "If you can’t determine an appropriate complexity limit for your code, then it’s best to disable this rule."

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

I think _migrateUI is an abnormal case of complexity. Most of the other functions that are highlighted by this rule are complex, hard to reason about, and not maintained well for fear of breaking a hidden code path. I'll also add that we should not be OK with having functions that are 341 lines long (measured based off of abf145ebd05f). Functions like this that have so many branches become impossible to test using automation.

The limit for this particular function is defined in /browser/components/.eslintrc.js. As part of bug 1352069 I'm asking for the limit to be lowered when that patch lands.

Comment 5

[Dão Gottwald [:dao]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #4)
> I think _migrateUI is an abnormal case of complexity. Most of the other
> functions that are highlighted by this rule are complex, hard to reason
> about, and not maintained well for fear of breaking a hidden code path. I'll
> also add that we should not be OK with having functions that are 341 lines
> long (measured based off of abf145ebd05f).

I have repeatedly worked with this method over the years, so have others. I haven't heard complaints or seen problems with others handling it. Makes me wonder whether the way complexity is measured here is even sane. Again, this method doesn't even seem complex to me. It's just long, but well structured. It may be "abnormal", but it's not clear to me why this should be banned.

> Functions like this that have so
> many branches become impossible to test using automation.

It's hard to test using automation (and in fact untested) because you'd need to set the profile to an old state and, more problematically, run startup, which isn't well supported by our infrastructure. This is unrelated to this method's "complexity."

> The limit for this particular function is defined in
> /browser/components/.eslintrc.js. As part of bug 1352069 I'm asking for the
> limit to be lowered when that patch lands.

We'll add more migration steps and dumping a lower limit on people without a clear path forward isn't productive.

How do you envision this method should be rewritten in a way that results in objectively better code? Will you actually do that rather than making it the responsibility of the person adding the next migration step?

Comment 6

[Mark Banner (:standard8)]

I think generally we have to start somewhere with complexity. I've seen functions around the 40 level being extended and increased in complexity, and its obvious they are already too complex when you hit them.

(In reply to Dão Gottwald [::dao] from comment #3)
> Can you elaborate on how the default of 20 is reasonable for our code base?

20 might be too low. However, we should aim low, and stop higher if necessary. I think working on the most complex ones first (there's a few ones that are really bad) would be best, and then re-assess it as we get down closer, and work out around cost/benefit.

(In reply to Dão Gottwald [::dao] from comment #5)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #4)
> > I think _migrateUI is an abnormal case of complexity. Most of the other
> > functions that are highlighted by this rule are complex, hard to reason
> > about, and not maintained well for fear of breaking a hidden code path. I'll
> > also add that we should not be OK with having functions that are 341 lines
> > long (measured based off of abf145ebd05f).
> 
> I have repeatedly worked with this method over the years, so have others. I
> haven't heard complaints or seen problems with others handling it. Makes me
> wonder whether the way complexity is measured here is even sane. Again, this
> method doesn't even seem complex to me. It's just long, but well structured.
> It may be "abnormal", but it's not clear to me why this should be banned.

BG_migrateUI could potentially be one of the exceptions. Though I also wonder if we're getting to the stage of do we really need the older code for upgrading profiles? Can some of it be consolidated?

> > Functions like this that have so
> > many branches become impossible to test using automation.
> 
> It's hard to test using automation (and in fact untested) because you'd need
> to set the profile to an old state and, more problematically, run startup,
> which isn't well supported by our infrastructure. This is unrelated to this
> method's "complexity."

Actually, we already have test_browserGlue_migration_loop_cleanup.js which is enough to test the "< 41" branch of the code. It isn't that simple for all of them, but we should make more of an effort to test this (e.g. if we can do things like test DB upgrades, I'm sure we can simulate enough to test profile upgrades).

> > The limit for this particular function is defined in
> > /browser/components/.eslintrc.js. As part of bug 1352069 I'm asking for the
> > limit to be lowered when that patch lands.
> 
> We'll add more migration steps and dumping a lower limit on people without a
> clear path forward isn't productive.
> 
> How do you envision this method should be rewritten in a way that results in
> objectively better code? Will you actually do that rather than making it the
> responsibility of the person adding the next migration step?

This could be a case where we decide to whitelist it, though having a review first would probably be a good idea.

I do think that for a lot of the complex methods, having the limits there will enforce that if developers are making it more complex, then they need to rethink the function at that time - which I believe is generally a good thing. The migration code is a bit of a special case.

I did mean to file bugs for all the cases where they are near the limits - so we can get people working on them in advance, unfortunately I've been tied up in other stuff, but hopefully next week.

Comment 7

[Dão Gottwald [:dao]]

(In reply to Mark Banner (:standard8) from comment #6)
> BG_migrateUI could potentially be one of the exceptions. Though I also
> wonder if we're getting to the stage of do we really need the older code for
> upgrading profiles? Can some of it be consolidated?

Absolutely, see bug 1356129 for instance. But that's really just a workaround. For photon we'll probably add more migration steps than we'd like to or even could remove.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Dão Gottwald [::dao] from comment #5)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #4)
> > I think _migrateUI is an abnormal case of complexity. Most of the other
> > functions that are highlighted by this rule are complex, hard to reason
> > about, and not maintained well for fear of breaking a hidden code path. I'll
> > also add that we should not be OK with having functions that are 341 lines
> > long (measured based off of abf145ebd05f).
> 
> I have repeatedly worked with this method over the years, so have others. I
> haven't heard complaints or seen problems with others handling it. Makes me
> wonder whether the way complexity is measured here is even sane. Again, this
> method doesn't even seem complex to me. It's just long, but well structured.
> It may be "abnormal", but it's not clear to me why this should be banned.

Well, without the complexity rule we wouldn't have bug 1356129 to clean up old migrations that we don't care about anymore. So this is actually highlighting a positive of it.
 
> > Functions like this that have so
> > many branches become impossible to test using automation.
> 
> It's hard to test using automation (and in fact untested) because you'd need
> to set the profile to an old state and, more problematically, run startup,
> which isn't well supported by our infrastructure. This is unrelated to this
> method's "complexity."

I disagree here. For example, if I look at the migration for `if (currentVersion < 29)` I see something that could be pulled out to a separate function that is easily unit-testable. The whole of _migrateUI may not be unit-testable without a large testing infrastructure setup, but the individual parts are easily unit-testable.

> > The limit for this particular function is defined in
> > /browser/components/.eslintrc.js. As part of bug 1352069 I'm asking for the
> > limit to be lowered when that patch lands.
> 
> We'll add more migration steps and dumping a lower limit on people without a
> clear path forward isn't productive.
> 
> How do you envision this method should be rewritten in a way that results in
> objectively better code? Will you actually do that rather than making it the
> responsibility of the person adding the next migration step?

This method could be written as a series of function calls for each version. Or each version could be a separate class that extends the previous class (ie, `function Firefox38Migration(state) { Firefox37Migration.call(this, state); do38SpecificMigration(); }`). Either of these implementations would allow for writing simple unit tests that verify the individual migrations that are supposed to happen continue to work.

Comment 9

[Dão Gottwald [:dao]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #8)
> Well, without the complexity rule we wouldn't have bug 1356129 to clean up
> old migrations that we don't care about anymore. So this is actually
> highlighting a positive of it.

I've regularly filed bugs on removing old steps without the complexity limit making me do it. See bug 1280999 for instance.

> I disagree here. For example, if I look at the migration for `if
> (currentVersion < 29)` I see something that could be pulled out to a
> separate function that is easily unit-testable. The whole of _migrateUI may
> not be unit-testable without a large testing infrastructure setup, but the
> individual parts are easily unit-testable.

You don't need to pull something into a separate method to make it testable, see Mark's example.

> > We'll add more migration steps and dumping a lower limit on people without a
> > clear path forward isn't productive.
> > 
> > How do you envision this method should be rewritten in a way that results in
> > objectively better code? Will you actually do that rather than making it the
> > responsibility of the person adding the next migration step?
> 
> This method could be written as a series of function calls for each version.
> Or each version could be a separate class that extends the previous class

We could do that. I would argue that it would make this code overall /more/ complex.

Comment 10

[Jim Porter (:squib)]

I think what we should do here is to make an exception just for this function, which should be pretty straightforward. Then we can get the eslint check for everything else and not have to worry about this function's complexity.