From f0f85106c77cd192ede5ef2f866a0e2ed01532b5 Mon Sep 17 00:00:00 2001 From: Ryan Davis Date: Mon, 14 May 2018 17:48:46 -0700 Subject: [PATCH 1/3] Added post_filter option to CCFlay (essentially flay 2.11's filtering). This is wired up via a new config option "post_filters". Post filtering happens BEFORE prune, so they get run before conservative/liberal pruning happens, which essentially needs be last to make sense. --- lib/cc/engine/analyzers/analyzer_base.rb | 8 ++++++++ lib/cc/engine/analyzers/engine_config.rb | 6 ++++++ lib/cc/engine/analyzers/reporter.rb | 1 + lib/ccflay.rb | 19 +++++++++++++++++++ 4 files changed, 34 insertions(+) diff --git a/lib/cc/engine/analyzers/analyzer_base.rb b/lib/cc/engine/analyzers/analyzer_base.rb index c4409124..fda293c6 100644 --- a/lib/cc/engine/analyzers/analyzer_base.rb +++ b/lib/cc/engine/analyzers/analyzer_base.rb @@ -58,6 +58,10 @@ def filters engine_config.filters_for(language) | default_filters end + def post_filters + engine_config.post_filters_for(language) | default_post_filters + end + def language self.class::LANGUAGE end @@ -108,6 +112,10 @@ def default_filters [] end + def default_post_filters + [] + end + def points_per_overage self.class::POINTS_PER_OVERAGE end diff --git a/lib/cc/engine/analyzers/engine_config.rb b/lib/cc/engine/analyzers/engine_config.rb index 781a6e35..a1d2fe9d 100644 --- a/lib/cc/engine/analyzers/engine_config.rb +++ b/lib/cc/engine/analyzers/engine_config.rb @@ -42,6 +42,12 @@ def filters_for(language) end end + def post_filters_for(language) + fetch_language(language).fetch("post_filters", []).map do |filter| + Sexp::Matcher.parse filter + end + end + def minimum_mass_threshold_for(language) [ mass_threshold_for(language, IDENTICAL_CODE_CHECK), diff --git a/lib/cc/engine/analyzers/reporter.rb b/lib/cc/engine/analyzers/reporter.rb index 2fee7917..c22c71e6 100644 --- a/lib/cc/engine/analyzers/reporter.rb +++ b/lib/cc/engine/analyzers/reporter.rb @@ -133,6 +133,7 @@ def flay_options changes = { mass: language_strategy.mass_threshold, filters: language_strategy.filters, + post_filters: language_strategy.post_filters, } CCFlay.default_options.merge changes diff --git a/lib/ccflay.rb b/lib/ccflay.rb index a93a42d7..4bf0bcea 100644 --- a/lib/ccflay.rb +++ b/lib/ccflay.rb @@ -18,6 +18,25 @@ def initialize(option = nil) self.identical = Concurrent::Hash.new self.masses = Concurrent::Hash.new end + + def post_filter *patterns + return if patterns.empty? + + self.hashes.delete_if { |_, sexps| + sexps.any? { |sexp| + patterns.any? { |pattern| + # pattern =~ sexp + pattern.satisfy? sexp + } + } + } + end + + def prune + post_filter(*option[:post_filters]) + + super + end end class Sexp From 3890258a2c24de30bdea2ae5d4474940822d76c4 Mon Sep 17 00:00:00 2001 From: Ryan Davis Date: Mon, 14 May 2018 17:51:18 -0700 Subject: [PATCH 2/3] Added sexp filtering and transforming for javascript to improve results. Added DEFAULT_POST_FILTERS to remove NUKE pseudo-nodes and Program nodes. Added transform_sexp to transform top-level :body node to NUKE. Wrapped the top level sexp so the whole tree gets processed. This is probably a bug in CCD and/or flay. --- lib/cc/engine/analyzers/javascript/main.rb | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/lib/cc/engine/analyzers/javascript/main.rb b/lib/cc/engine/analyzers/javascript/main.rb index d02bfa94..8f7f2d7d 100644 --- a/lib/cc/engine/analyzers/javascript/main.rb +++ b/lib/cc/engine/analyzers/javascript/main.rb @@ -14,9 +14,14 @@ class Main < CC::Engine::Analyzers::Base LANGUAGE = "javascript" DEFAULT_MASS_THRESHOLD = 45 DEFAULT_FILTERS = [ + "(directives (Directive (value (DirectiveLiteral ___))))", "(ImportDeclaration ___)".freeze, "(VariableDeclarator _ (init (CallExpression (_ (Identifier require)) ___)))".freeze, ].freeze + DEFAULT_POST_FILTERS = [ + "(NUKE ___)", + "(Program _ ___)", + ].freeze POINTS_PER_OVERAGE = 30_000 REQUEST_PATH = "/javascript".freeze @@ -24,6 +29,19 @@ def use_sexp_lines? false end + ## + # Transform sexp as such: + # + # s(:Program, :module, s(:body, ... )) + # => s(:NUKE, s(:Program, :module, s(:NUKE, ... ))) + + def transform_sexp(sexp) + sexp.body.sexp_type = :NUKE # negate top level body + sexp = s(:NUKE, sexp) # wrap with extra node to force full process + + sexp + end + private def process_file(file) @@ -33,6 +51,10 @@ def process_file(file) def default_filters DEFAULT_FILTERS.map { |filter| Sexp::Matcher.parse filter } end + + def default_post_filters + DEFAULT_POST_FILTERS.map { |filter| Sexp::Matcher.parse filter } + end end end end From cf270702ad309821785e9e28c5c7d001abaf5bc3 Mon Sep 17 00:00:00 2001 From: Ryan Davis Date: Mon, 14 May 2018 17:53:39 -0700 Subject: [PATCH 3/3] NASTY spec for the latest transforming/pruning changes. I am completely OK if this doesn't make it in. I'm not sure how to reduce it more to make it un-gross. --- .../engine/analyzers/javascript/main_spec.rb | 88 +++++++++++++++++++ 1 file changed, 88 insertions(+) diff --git a/spec/cc/engine/analyzers/javascript/main_spec.rb b/spec/cc/engine/analyzers/javascript/main_spec.rb index 835d5bc5..fb8666ae 100644 --- a/spec/cc/engine/analyzers/javascript/main_spec.rb +++ b/spec/cc/engine/analyzers/javascript/main_spec.rb @@ -174,6 +174,94 @@ expect(issues).to be_empty end + it "ignores imports and reports proper line number boundaries" do + create_source_file("foo.js", <<~EOJS) + 'use strict'; + + import React from 'react'; + + import Translator from '../../../i18n/translator-tag.jsx'; + import { gettingSeniorID } from '../../../helpers/data/senior'; + import { choosingReducedFee } from '../../../helpers/data/reduced-fee'; + import { correctID } from '../../../helpers/data/card-type'; + + + const Senior = (props) => { + if (!gettingSeniorID(props.IDApp)) { return null; } + return + }; + + const Reduced = (props) => { + if (!choosingReducedFee(props.IDApp)) { return null; } + return + }; + + const Regular = (props) => { + if (gettingSeniorID(props.IDApp) || choosingReducedFee(props.IDApp)) { return null; } + return + }; + + + const CorrectingIDInfo = (props) => { + if(!correctID(props)) { return null; } + return ( +
+ + + +
+ ); + }; + + export default CorrectingIDInfo; + EOJS + + create_source_file("bar.js", <<~EOJS) + 'use strict'; + + import React from 'react'; + import { updateID } from '../../../helpers/data/card-type'; + import { gettingSeniorID } from '../../../helpers/data/senior'; + import { choosingReducedFee } from '../../../helpers/data/reduced-fee'; + import Translator from '../../../i18n/translator-tag.jsx'; + + const Senior = (props) => { + if (!gettingSeniorID(props.IDApp)) { return null; } + return + }; + + const Reduced = (props) => { + if (!choosingReducedFee(props.IDApp)) { return null; } + return + }; + + const Regular = (props) => { + if (gettingSeniorID(props.IDApp) || choosingReducedFee(props.IDApp)) { return null; } + return + }; + + const UpdatingIDInfo = (props) => { + if (!updateID(props)) { return null; } + return ( +
+ + + +
+ ); + }; + + export default UpdatingIDInfo; + EOJS + + issues = run_engine(engine_conf).strip.split("\0") + issues = issues.map { |issue| JSON.parse issue } + + infected = issues.any? { |i| i.dig("location", "lines", "begin") == 1 } + + expect(infected).to be false + end + it "outputs the correct line numbers for ASTs missing line details (codeclimate/app#6227)" do create_source_file("foo.js", <<~EOJS) `/movie?${getQueryString({ movie_id: movieId })}`