terryyin / lizard

A simple code complexity analyser without caring about the C/C++ header files or Java imports, supports most of the popular languages.
Other
1.85k stars 250 forks source link

Incorrect behavior when a variable named `get` is used in Swift #240

Closed Coeur closed 5 years ago

Coeur commented 5 years ago

I have the following Swift file, but lizard (master branch at b631dfd3561366539633a7279f755e237541f3bf) believes it is one single giant getter:

//  AFNetworkingWrapper.swift
//  Created by Antoine Cœur on 20/10/2016.
//  Copyright © 2016 EF Education. All rights reserved.

/*...*/

/// HTTP method definitions.
///
/// See https://tools.ietf.org/html/rfc7231#section-4.3
public enum HTTPMethod: String {
    case options = "OPTIONS"
    case get     = "GET"
    case head    = "HEAD"
    case post    = "POST"
    case put     = "PUT"
    case patch   = "PATCH"
    case delete  = "DELETE"
    case trace   = "TRACE"
    case connect = "CONNECT"
}

extension AFHTTPSessionManager {

    convenience init(baseURL url: URL?, requestEncoding: ParameterSerializer?, responseEncoding: ParameterSerializer? = nil) {
        self.init(baseURL: url)
        /*...*/
    }

    func foo(_ requestHeaders: [String:String]) {
        /*...*/
    }

    func bar(_ requestHeaders: [String:String]) {
        /*...*/
    }
    /*...*/
}

lizard output:

$ python lizard.py AFNetworkingWrapper.swift 
================================================
  NLOC    CCN   token  PARAM  length  location  
------------------------------------------------
     106     29    714      0     118 get@29-146@/coeur/AFNetworkingWrapper.swift
1 file analyzed.
==============================================================
NLOC    Avg.NLOC  AvgCCN  Avg.token  function_cnt    file
--------------------------------------------------------------
    115     106.0    29.0      714.0         1     /coeur/AFNetworkingWrapper.swift

=========================================================================================
!!!! Warnings (cyclomatic_complexity > 15 or length > 1000 or parameter_count > 100) !!!!
================================================
  NLOC    CCN   token  PARAM  length  location  
------------------------------------------------
     106     29    714      0     118 get@29-146@/coeur/AFNetworkingWrapper.swift
==========================================================================================
Total nloc   Avg.NLOC  AvgCCN  Avg.token   Fun Cnt  Warning cnt   Fun Rt   nloc Rt
------------------------------------------------------------------------------------------
       115     106.0    29.0      714.0        1            1      1.00    1.00
Coeur commented 5 years ago

It seems that the culprit is https://github.com/terryyin/lizard/blob/master/lizard_languages/swift.py#L35:

    if token in ('get', 'set', 'deinit'):
        self.context.start_new_function(token)
        self._state = self._expect_function_impl

Note that it is correct Python code for Swift situations like:

var hello {
    get { return "hello" }
    set {}
}

But in the following contexts it's not correct to interpret get as a function:

case get ...
var get ...
let get ...

So I advice to create a new _state for declarations. Something along the lines of:

    if token in ('var', 'let', 'case'):
        self.context.start_new_variable(token)
        self._state = self._expect_variable_impl

And you would leave the declarative state when encountering a new line, an opening brace {, a semicolon ;, a colon : or an equal sign =.

terryyin commented 5 years ago

Hi Antoine,

Thanks! In training this week so the fix will come a bit late. Is it possible to have a pull request from you?

Sent from my iPhone

On Oct 23, 2018, at 11:52, Antoine Cœur notifications@github.com wrote:

It seems that the culprit is https://github.com/terryyin/lizard/blob/master/lizard_languages/swift.py#L35:

if token in ('get', 'set', 'deinit'):
    self.context.start_new_function(token)
    self._state = self._expect_function_impl

Note that it is correct Python code for Swift situations like:

var hello { get { return "hello" } set {} } But in the following contexts it's not correct to interpret get as a function:

case get ... var get ... let get ... So I advice to create a new _state for declarations. Something along the lines of:

if token in ('var', 'let', 'case'):
    self.context.start_new_variable(token)
    self._state = self._expect_variable_impl

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

Coeur commented 5 years ago

@terryyin Ah ah, I was fearing that. OK, I'll do a pull request, but I don't know how to run unit tests from command line (I only know python lizard.py files). So it's likely that I'll break something.

Coeur commented 5 years ago

OK, I figured out how to run the tests locally:

make deps make tests make pylint brew install python3 make tests3

terryyin commented 5 years ago

Glad to see you solve this. I still have yet another day of training...

Coeur commented 5 years ago

@terryyin How was the training? :)