add detect_cyclomatic_complexity

[xjdrew]

detect McCabe’s complexity, warning if function's complexity > 10

[xjdrew]

hello @mpeterv , I add detect_cyclomatic_complexity.lua for detecting McCabe’s complexity. It detects all closure's complexity and emit warnings, then filters by max_cyclomatic_complexity parameter (defaults 10). The way is similay with checking line length.

But some test cases call check directy, skip filter, so these cases fails.

For example:

Failure → spec/check_spec.lua @ 33
check detects global set in nested functions
spec/check_spec.lua:34: Expected objects to be the same.
Passed in:
(table) {
 *[1] = {
    [code] = '711'
    [column] = 7
    [complexity] = 1
    [end_column] = 7
   *[line] = 1
    [name] = 'bar' }
  [2] = {
    [code] = '111'
    [column] = 4
    [end_column] = 6
    [indexing] = {
      [1] = 'foo' }
    [line] = 2
    [name] = 'foo' } }
Expected:
(table) {
 *[1] = {
    [code] = '111'
    [column] = 4
    [end_column] = 6
    [indexing] = {
      [1] = 'foo' }
   *[line] = 2
    [name] = 'foo' } }

Would you merge this PR if I fix all failed cases properly? And what's your suggestion about how to fix this issue?

best regards

[mpeterv]

Hello @xjdrew and thank you for the PR! I agree that having cyclomatic complexity check in Luacheck is useful. I'm not sure yet if it's better to have it enabled by default or not, as it's a less common check and can require some research to fix the warnings.

I'll deal with tests failing due to the new warning after merging the PR. However, there should be some new tests ensuring that this new warning is working properly. Would you mind adding some tests in spec/check_ spec.lua?

There are also some other minor issues:

• The new file needs to be added to lists in luacheck-scm-1.rockspec and install.lua.
• Warnings about unnamed closures should be handled a bit differently to avoid extra space or empty quoted string with --formatter=plain in warning messages. Such warnings should not have name property at all, and format message should be chosen conditionally based on its presence. See warnings like 111 for example.

I don't mind fixing these issues myself after merging but it may be slower that way.

[xjdrew]

@mpeterv I have make detect_cyclomatic_complexity disabled by default, and ignore 711 warning in spec/check_spec.lua.

So there is only one case failed by now:

Failure → spec/cli_spec.lua @ 850
cli caching caches results
spec/cli_spec.lua:923: Cache string is:
23
spec/samples/good_code.lua
1512618974
local A="711";return {{{A,"helper",3,7,7},{A,"",7,1,1}},{},{19,0,23,17,3,0,30,25,26,3,0,15},{[4]="comment"}}
spec/samples/bad_code.lua
1512618974
local A,B,C,D,E="package","helper","711","embrace","hepler";return {{{"112",A,1,1,7,[23]={A,"loaded",true}},{C,B,3,7,7},{"211",B,3,16,21,[10]=true},{"212","...",3,23,25},{C,"",7,1,1},{"111",D,7,10,16,[11]=true,[23]={D}},{"412","opt",8,10,12,7,18},{"113",E,9,11,16,[23]={E}}},{},{24,0,26,9,3,0,21,31,26,3,0},{[4]="comment"}}
spec/samples/python_code.lua
1512618974
return {{{"011",[3]=1,[4]=6,[5]=15,[12]="expected '=' near '__future__'"}},{},{}}

Expected objects to be the same.
Passed in:
(nil)
Expected:
type number

[xjdrew]

@mpeterv I add 6 tests for detect_cyclomatic_complexity, and format unnamed closures properly

by now, only local functions can get name, other functions, such as functions assigned to a table field, can't capture name. Do you have any suggestion?

-- can't capture name
function A()
end

lcoal m = {}

-- can't capture name
function m.B()
end

-- can't capture name
function m:C()
end

[mpeterv]

@xjdrew yes, there should be a way to attach a full name to functions like those. It's not done currently as right now names are only needed for unused local functions. It doesn't have to be done in this PR though.

Thanks for adding the tests. Some more things:

• I think that the warning could use 5xx code, like 561. I'd like to conserve root warning categories and it makes sense to put cyclomatic complexity into the category that has unreachable code already, as both are about control flow.
• Do you think it should check complexity of the implicit "main chunk" function? Right now it doesn't as line.lines does not include line itself. So if there is some overly complex initialization in the top level of a module, it won't catch it. However, if the main chunk is checked, it should be reported with a special message, e.g. Main chunk is too complicated instead of something like Function 'main chunk' is too complicated. That requires a special flag in the warning object and another branch when selecting warning message format.
• Please use 3 spaces per indentation level for consistency with the rest of the code.

[xjdrew]

@mpeterv thanks for you suggestions.

in future, we can check more metrics of the code quality, for example max depth, max statments of function and so no; can use code 7xx for this purpose. so I think 7xx is necessary.

I'll fix the main chunk and indentation problem.

[mpeterv]

@xjdrew I've pushed your commits to cyclomatic_complexity branch in this repo and added some minor fixes. If you have follow up PRs for this, target that branch. I'll decide about the warning code and maybe add some more minor style fixes and then merge the branch into master,

[xjdrew]

@mpeterv when do you plan to merge this feature？

[mpeterv]

Sorry for this huge delay @xjdrew =( Merged into master, still want to adjust some things before a release.

[LPGhatguy]

Were there any docs changes merged with this PR? It appears that the feature doesn't appear in the online documentation!

[mpeterv]

@LPGhatguy looks like for some reason readthedocs had stable docs pointing to 0.21.2 instead of 0.22.0, should be fixed now.