Tools to better explain complexity

[MichaelChirico]

Now, we can be hit with hard-to-use information about changes in code complexity.

Example:

r-lib/lintr@d740159

We refactored a linter, and suddenly the complexity jumped up. It's not immediately clear why / where to focus efforts on reducing complexity. The complexity is now "20" but it really requires a deep understanding of the metric to reason about what that means.

One idea is to offer a function helping diagnose complexity. For example, it was helpful in my case to examine leave-one-out complexity, i.e. the complexity of individual expressions and of the tree excluding each expression:

# source of x in <details> below to reduce noise
sapply(x, cyclocomp)
# [1]  1  1  1  1  1  1  2 11  1  1  9
sapply(seq_along(x), \(i) cyclocomp(x[-i]))
# [1] 20 20 20 20 20 20 19 10 20 20 12

With this output it's clear I need to focus on expression 8 and/or 11 -- excluding those gives much lower complexity.

x <- quote({
    allow_file_path <- match.arg(allow_file_path)
    paste_sep_xpath <- "\n  //SYMBOL_FUNCTION_CALL[text() = 'paste']\n    /parent::expr\n    /following-sibling::SYMBOL_SUB[text() = 'sep' and following-sibling::expr[1][STR_CONST]]\n    /parent::expr\n  "
    to_string_xpath <- "\n  //SYMBOL_FUNCTION_CALL[text() = 'paste' or text() = 'paste0']\n    /parent::expr\n    /parent::expr[\n      count(expr) = 3\n      and SYMBOL_SUB[text() = 'collapse']/following-sibling::expr[1][STR_CONST]\n    ]\n  "
    paste0_sep_xpath <- "\n  //SYMBOL_FUNCTION_CALL[text() = 'paste0']\n    /parent::expr\n    /following-sibling::SYMBOL_SUB[text() = 'sep']\n    /parent::expr\n  "
    paste_strrep_xpath <- "\n  //SYMBOL_FUNCTION_CALL[text() = 'paste' or text() = 'paste0']\n    /parent::expr[\n      count(following-sibling::expr) = 2\n      and following-sibling::expr[1][expr[1][SYMBOL_FUNCTION_CALL[text() = 'rep']] and expr[2][STR_CONST]]\n      and following-sibling::SYMBOL_SUB[text() = 'collapse']\n    ]\n    /parent::expr\n  "
    if (allow_file_path %in% c("always", "double_slash")) {
        check_double_slash <- function(str) any(grepl("//", str, 
            fixed = TRUE))
    }
    else {
        check_double_slash <- function(str) any(grepl("^//|//$", 
            str))
    }
    check_is_not_file_path <- function(expr) {
        args <- xml_find_all(expr, "expr[position() > 1]")
        is_string <- !is.na(xml_find_first(args, "STR_CONST"))
        string_values <- character(length(args))
        string_values[is_string] <- get_r_string(args[is_string])
        not_start_slash <- which(!startsWith(string_values, "/"))
        not_end_slash <- which(!endsWith(string_values, "/"))
        string_values[1L] == "/" || string_values[length(string_values)] == 
            "/" || check_double_slash(string_values) || any(!is_string[c(not_end_slash + 
            1L, not_start_slash - 1L)], na.rm = TRUE) || any(not_start_slash %in% 
            (not_end_slash + 1L))
    }
    paste0_file_path_xpath <- xp_strip_comments("\n  //SYMBOL_FUNCTION_CALL[text() = 'paste0']\n    /parent::expr\n    /parent::expr[\n      (: exclude paste0(x) :)\n      count(expr) > 2\n      (: An expression matching _any_ of these conditions is _not_ a file path :)\n      and not(\n        (: Any numeric input :)\n        expr/NUM_CONST\n        (: A call using collapse= :)\n        or SYMBOL_SUB[text() = 'collapse']\n        (: Consecutive non-strings like paste0(x, y) :)\n        or expr[(SYMBOL or expr) and following-sibling::expr[1][SYMBOL or expr]]\n      )\n    ]\n  ")
    empty_paste_note <- "Note that paste() converts empty inputs to \"\", whereas file.path() leaves it empty."
    Linter(function(source_expression) {
        if (!is_lint_level(source_expression, "expression")) {
            return(list())
        }
        xml <- source_expression$xml_parsed_content
        optional_lints <- list()
        if (!allow_empty_sep || allow_file_path %in% c("double_slash", 
            "never")) {
            paste_sep_expr <- xml_find_all(xml, paste_sep_xpath)
            paste_sep_value <- get_r_string(paste_sep_expr, xpath = "./SYMBOL_SUB[text() = 'sep']/following-sibling::expr[1]")
        }
        if (!allow_empty_sep) {
            optional_lints <- c(optional_lints, xml_nodes_to_lints(paste_sep_expr[!nzchar(paste_sep_value)], 
                source_expression = source_expression, lint_message = "paste0(...) is better than paste(..., sep = \"\").", 
                type = "warning"))
        }
        if (!allow_to_string) {
            to_string_expr <- xml_find_all(xml, to_string_xpath)
            collapse_value <- get_r_string(to_string_expr, xpath = "./SYMBOL_SUB[text() = 'collapse']/following-sibling::expr[1]")
            optional_lints <- c(optional_lints, xml_nodes_to_lints(to_string_expr[collapse_value == 
                ", "], source_expression = source_expression, 
                lint_message = paste("toString(.) is more expressive than paste(., collapse = \", \").", 
                  "Note also glue::glue_collapse() and and::and()", 
                  "for constructing human-readable / translation-friendly lists"), 
                type = "warning"))
        }
        paste0_sep_expr <- xml_find_all(xml, paste0_sep_xpath)
        paste0_sep_lints <- xml_nodes_to_lints(paste0_sep_expr, 
            source_expression = source_expression, lint_message = "sep= is not a formal argument to paste0(); did you mean to use paste(), or collapse=?", 
            type = "warning")
        paste_strrep_expr <- xml_find_all(xml, paste_strrep_xpath)
        collapse_arg <- get_r_string(paste_strrep_expr, "SYMBOL_SUB/following-sibling::expr[1]/STR_CONST")
        paste_strrep_expr <- paste_strrep_expr[!nzchar(collapse_arg)]
        paste_call <- xp_call_name(paste_strrep_expr)
        paste_strrep_lints <- xml_nodes_to_lints(paste_strrep_expr, 
            source_expression = source_expression, lint_message = sprintf("strrep(x, times) is better than %s(rep(x, times), collapse = \"\").", 
                paste_call), type = "warning")
        if (allow_file_path %in% c("double_slash", "never")) {
            paste_sep_slash_expr <- paste_sep_expr[paste_sep_value == 
                "/"]
            optional_lints <- c(optional_lints, xml_nodes_to_lints(paste_sep_slash_expr[is.na(xml_find_first(paste_sep_slash_expr, 
                "./SYMBOL_SUB[text() = 'collapse']"))], source_expression = source_expression, 
                lint_message = paste("Construct file paths with file.path(...) instead of paste(..., sep = \"/\").", 
                  "If you are using paste(sep = \"/\") to construct a date,", 
                  "consider using format() or lubridate helpers instead.", 
                  empty_paste_note), type = "warning"))
            paste0_file_path_expr <- xml_find_all(xml, paste0_file_path_xpath)
            is_file_path <- !vapply(paste0_file_path_expr, check_is_not_file_path, 
                logical(1L))
            optional_lints <- c(optional_lints, xml_nodes_to_lints(paste0_file_path_expr[is_file_path], 
                source_expression = source_expression, lint_message = paste("Construct file paths with file.path(...) instead of paste0(x, \"/\", y, \"/\", z).", 
                  empty_paste_note), type = "warning"))
        }
        c(optional_lints, paste0_sep_lints, paste_strrep_lints)
    })
})