From 5854b831f3107f6726945e84ad823485f569ff70 Mon Sep 17 00:00:00 2001 From: Calum Grant Date: Thu, 4 Mar 2021 13:43:59 +0000 Subject: [PATCH 1/8] Ruby: rb/use-detect query --- ql/src/queries/performance/UseDetect.ql | 63 +++++++++++++++++++++++++ 1 file changed, 63 insertions(+) create mode 100644 ql/src/queries/performance/UseDetect.ql diff --git a/ql/src/queries/performance/UseDetect.ql b/ql/src/queries/performance/UseDetect.ql new file mode 100644 index 00000000000..b5449f30c0c --- /dev/null +++ b/ql/src/queries/performance/UseDetect.ql @@ -0,0 +1,63 @@ +/** + * @name Use detect + * @description Use 'detect' instead of 'first' and 'last'. + * @kind problem + * @problem.severity warning + * @id rb/use-detect + * @tags performance rubocop + * @precision high + * + * This is an implementation of Rubocop rule + * https://github.com/rubocop/rubocop-performance/blob/master/lib/rubocop/cop/performance/detect.rb + */ + +import ruby + +// Extracts the first or last element of a list +abstract class EndCall extends MethodCall { + abstract string detectCall(); +} + +abstract class First extends EndCall { + override string detectCall() { result = "detect" } +} + +class FirstCall extends First { + FirstCall() { + this.getMethodName() = "first" and + this.getNumberOfArguments() = 0 + } +} + +class FirstElement extends First, ElementReference { + FirstElement() { + this.getNumberOfArguments() = 1 and + this.getArgument(0).(IntegerLiteral).getValueText() = "0" + } +} + +abstract class Last extends EndCall { + override string detectCall() { result = "reverse_detect" } +} + +class LastCall extends Last { + LastCall() { this.getMethodName() = "last" and this.getNumberOfArguments() = 0 } +} + +class LastElement extends Last, ElementReference { + LastElement() { + this.getNumberOfArguments() = 1 and + this.getArgument(0).(UnaryMinusExpr).getOperand().(IntegerLiteral).getValueText() = "1" + } +} + +class SelectBlock extends MethodCall { + SelectBlock() { + this.getMethodName() in ["select", "filter", "find_all"] and + exists(this.getBlock()) + } +} + +from EndCall call, SelectBlock selectBlock +where selectBlock = call.getReceiver() +select call, "Replace this call with '" + call.detectCall() + "'." From 522bcff79d3cc32d461e3d29e480489c4f27f5ff Mon Sep 17 00:00:00 2001 From: Calum Grant Date: Thu, 4 Mar 2021 15:38:09 +0000 Subject: [PATCH 2/8] Ruby: Initial test case --- .../performance/UseDetect/UseDetect.rb | 128 ++++++++++++++++++ 1 file changed, 128 insertions(+) create mode 100644 ql/test/query-tests/performance/UseDetect/UseDetect.rb diff --git a/ql/test/query-tests/performance/UseDetect/UseDetect.rb b/ql/test/query-tests/performance/UseDetect/UseDetect.rb new file mode 100644 index 00000000000..267643bac74 --- /dev/null +++ b/ql/test/query-tests/performance/UseDetect/UseDetect.rb @@ -0,0 +1,128 @@ +# frozen_string_literal: true + +require "cases/helper" +require "models/post" +require "models/comment" + +module ActiveRecord + module ConnectionAdapters + class Mysql2SchemaTest < ActiveRecord::Mysql2TestCase + fixtures :posts + + def setup + @connection = ActiveRecord::Base.connection + db = Post.connection_pool.db_config.database + table = Post.table_name + @db_name = db + + @omgpost = Class.new(ActiveRecord::Base) do + self.inheritance_column = :disabled + self.table_name = "#{db}.#{table}" + def self.name; "Post"; end + end + end + + def test_float_limits + @connection.create_table :mysql_doubles do |t| + t.float :float_no_limit + t.float :float_short, limit: 5 + t.float :float_long, limit: 53 + + t.float :float_23, limit: 23 + t.float :float_24, limit: 24 + t.float :float_25, limit: 25 + end + + column_no_limit = @connection.columns(:mysql_doubles).find { |c| c.name == "float_no_limit" } + column_short = @connection.columns(:mysql_doubles).find { |c| c.name == "float_short" } + column_long = @connection.columns(:mysql_doubles).find { |c| c.name == "float_long" } + + column_23 = @connection.columns(:mysql_doubles).find { |c| c.name == "float_23" } + column_24 = @connection.columns(:mysql_doubles).find { |c| c.name == "float_24" } + column_25 = @connection.columns(:mysql_doubles).find { |c| c.name == "float_25" } + + # MySQL floats are precision 0..24, MySQL doubles are precision 25..53 + assert_equal 24, column_no_limit.limit + assert_equal 24, column_short.limit + assert_equal 53, column_long.limit + + assert_equal 24, column_23.limit + assert_equal 24, column_24.limit + assert_equal 53, column_25.limit + ensure + @connection.drop_table "mysql_doubles", if_exists: true + end + + def test_schema + assert @omgpost.first + end + + def test_primary_key + assert_equal "id", @omgpost.primary_key + end + + def test_data_source_exists? + name = @omgpost.table_name + assert @connection.data_source_exists?(name), "#{name} data_source should exist" + end + + def test_data_source_exists_wrong_schema + assert_not(@connection.data_source_exists?("#{@db_name}.zomg"), "data_source should not exist") + end + + def test_dump_indexes + index_a_name = "index_key_tests_on_snack" + index_b_name = "index_key_tests_on_pizza" + index_c_name = "index_key_tests_on_awesome" + + table = "key_tests" + + indexes = @connection.indexes(table).sort_by(&:name) + assert_equal 3, indexes.size + + index_a = indexes.select { |i| i.name == index_a_name }[0] + index_b = indexes.select { |i| i.name == index_b_name }[0] + index_c = indexes.select { |i| i.name == index_c_name }[0] + assert_equal :btree, index_a.using + assert_nil index_a.type + assert_equal :btree, index_b.using + assert_nil index_b.type + + assert_nil index_c.using + assert_equal :fulltext, index_c.type + end + + unless mysql_enforcing_gtid_consistency? + def test_drop_temporary_table + @connection.transaction do + @connection.create_table(:temp_table, temporary: true) + # if it doesn't properly say DROP TEMPORARY TABLE, the transaction commit + # will complain that no transaction is active + @connection.drop_table(:temp_table, temporary: true) + end + end + end + end + end +end + +class Mysql2AnsiQuotesTest < ActiveRecord::Mysql2TestCase + def setup + @connection = ActiveRecord::Base.connection + @connection.execute("SET SESSION sql_mode='ANSI_QUOTES'") + end + + def teardown + @connection.reconnect! + end + + def test_primary_key_method_with_ansi_quotes + assert_equal "id", @connection.primary_key("topics") + end + + def test_foreign_keys_method_with_ansi_quotes + fks = @connection.foreign_keys("lessons_students") + assert_equal([["lessons_students", "students", :cascade]], + fks.map { |fk| [fk.from_table, fk.to_table, fk.on_delete] }) + end +end From ca497479c2bd31676073ec4b2b3af7b8ac0ce0e3 Mon Sep 17 00:00:00 2001 From: Calum Grant Date: Thu, 4 Mar 2021 15:44:05 +0000 Subject: [PATCH 3/8] Ruby: Finish the test for UseDetect --- .../performance/UseDetect/UseDetect.expected | 6 + .../performance/UseDetect/UseDetect.qlref | 1 + .../performance/UseDetect/UseDetect.rb | 136 ++---------------- 3 files changed, 19 insertions(+), 124 deletions(-) create mode 100644 ql/test/query-tests/performance/UseDetect/UseDetect.expected create mode 100644 ql/test/query-tests/performance/UseDetect/UseDetect.qlref diff --git a/ql/test/query-tests/performance/UseDetect/UseDetect.expected b/ql/test/query-tests/performance/UseDetect/UseDetect.expected new file mode 100644 index 00000000000..2fd8b960ed9 --- /dev/null +++ b/ql/test/query-tests/performance/UseDetect/UseDetect.expected @@ -0,0 +1,6 @@ +| UseDetect.rb:5:9:5:36 | call to first | Replace this call with 'detect'. | +| UseDetect.rb:6:9:6:35 | call to last | Replace this call with 'reverse_detect'. | +| UseDetect.rb:7:9:7:33 | ...[...] | Replace this call with 'detect'. | +| UseDetect.rb:8:9:8:34 | ...[...] | Replace this call with 'reverse_detect'. | +| UseDetect.rb:9:9:9:36 | call to first | Replace this call with 'detect'. | +| UseDetect.rb:10:9:10:37 | call to last | Replace this call with 'reverse_detect'. | diff --git a/ql/test/query-tests/performance/UseDetect/UseDetect.qlref b/ql/test/query-tests/performance/UseDetect/UseDetect.qlref new file mode 100644 index 00000000000..6d920e39cff --- /dev/null +++ b/ql/test/query-tests/performance/UseDetect/UseDetect.qlref @@ -0,0 +1 @@ +queries/performance/UseDetect.ql \ No newline at end of file diff --git a/ql/test/query-tests/performance/UseDetect/UseDetect.rb b/ql/test/query-tests/performance/UseDetect/UseDetect.rb index 267643bac74..b0053e188d7 100644 --- a/ql/test/query-tests/performance/UseDetect/UseDetect.rb +++ b/ql/test/query-tests/performance/UseDetect/UseDetect.rb @@ -1,128 +1,16 @@ -# frozen_string_literal: true -require "cases/helper" -require "models/post" -require "models/comment" +class DetectTest + def test + # These are bad + [].select { |i| true }.first + [].select { |i| true }.last + [].select { |i| true }[0] + [].select { |i| true }[-1] + [].filter { |i| true }.first + [].find_all { |i| true }.last -module ActiveRecord - module ConnectionAdapters - class Mysql2SchemaTest < ActiveRecord::Mysql2TestCase - fixtures :posts - - def setup - @connection = ActiveRecord::Base.connection - db = Post.connection_pool.db_config.database - table = Post.table_name - @db_name = db - - @omgpost = Class.new(ActiveRecord::Base) do - self.inheritance_column = :disabled - self.table_name = "#{db}.#{table}" - def self.name; "Post"; end - end - end - - def test_float_limits - @connection.create_table :mysql_doubles do |t| - t.float :float_no_limit - t.float :float_short, limit: 5 - t.float :float_long, limit: 53 - - t.float :float_23, limit: 23 - t.float :float_24, limit: 24 - t.float :float_25, limit: 25 - end - - column_no_limit = @connection.columns(:mysql_doubles).find { |c| c.name == "float_no_limit" } - column_short = @connection.columns(:mysql_doubles).find { |c| c.name == "float_short" } - column_long = @connection.columns(:mysql_doubles).find { |c| c.name == "float_long" } - - column_23 = @connection.columns(:mysql_doubles).find { |c| c.name == "float_23" } - column_24 = @connection.columns(:mysql_doubles).find { |c| c.name == "float_24" } - column_25 = @connection.columns(:mysql_doubles).find { |c| c.name == "float_25" } - - # MySQL floats are precision 0..24, MySQL doubles are precision 25..53 - assert_equal 24, column_no_limit.limit - assert_equal 24, column_short.limit - assert_equal 53, column_long.limit - - assert_equal 24, column_23.limit - assert_equal 24, column_24.limit - assert_equal 53, column_25.limit - ensure - @connection.drop_table "mysql_doubles", if_exists: true - end - - def test_schema - assert @omgpost.first - end - - def test_primary_key - assert_equal "id", @omgpost.primary_key - end - - def test_data_source_exists? - name = @omgpost.table_name - assert @connection.data_source_exists?(name), "#{name} data_source should exist" - end - - def test_data_source_exists_wrong_schema - assert_not(@connection.data_source_exists?("#{@db_name}.zomg"), "data_source should not exist") - end - - def test_dump_indexes - index_a_name = "index_key_tests_on_snack" - index_b_name = "index_key_tests_on_pizza" - index_c_name = "index_key_tests_on_awesome" - - table = "key_tests" - - indexes = @connection.indexes(table).sort_by(&:name) - assert_equal 3, indexes.size - - index_a = indexes.select { |i| i.name == index_a_name }[0] - index_b = indexes.select { |i| i.name == index_b_name }[0] - index_c = indexes.select { |i| i.name == index_c_name }[0] - assert_equal :btree, index_a.using - assert_nil index_a.type - assert_equal :btree, index_b.using - assert_nil index_b.type - - assert_nil index_c.using - assert_equal :fulltext, index_c.type - end - - unless mysql_enforcing_gtid_consistency? - def test_drop_temporary_table - @connection.transaction do - @connection.create_table(:temp_table, temporary: true) - # if it doesn't properly say DROP TEMPORARY TABLE, the transaction commit - # will complain that no transaction is active - @connection.drop_table(:temp_table, temporary: true) - end - end - end + # These are good + [].select("").first + [].select { |i| true }[1] end - end -end - -class Mysql2AnsiQuotesTest < ActiveRecord::Mysql2TestCase - def setup - @connection = ActiveRecord::Base.connection - @connection.execute("SET SESSION sql_mode='ANSI_QUOTES'") - end - - def teardown - @connection.reconnect! - end - - def test_primary_key_method_with_ansi_quotes - assert_equal "id", @connection.primary_key("topics") - end - - def test_foreign_keys_method_with_ansi_quotes - fks = @connection.foreign_keys("lessons_students") - assert_equal([["lessons_students", "students", :cascade]], - fks.map { |fk| [fk.from_table, fk.to_table, fk.on_delete] }) - end end From 20a62d169ab1c017ec962a7d5838584e5c1ed716 Mon Sep 17 00:00:00 2001 From: Calum Grant Date: Thu, 4 Mar 2021 15:48:09 +0000 Subject: [PATCH 4/8] Ruby: Update query description --- ql/src/queries/performance/UseDetect.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ql/src/queries/performance/UseDetect.ql b/ql/src/queries/performance/UseDetect.ql index b5449f30c0c..d75c4b83c25 100644 --- a/ql/src/queries/performance/UseDetect.ql +++ b/ql/src/queries/performance/UseDetect.ql @@ -1,6 +1,6 @@ /** * @name Use detect - * @description Use 'detect' instead of 'first' and 'last'. + * @description Use 'detect' instead of 'select' followed by 'first' or 'last'. * @kind problem * @problem.severity warning * @id rb/use-detect From 0f829476f4a7ecaf7c28ef190bc21c00b7a41304 Mon Sep 17 00:00:00 2001 From: Calum Grant Date: Tue, 9 Mar 2021 10:13:07 +0000 Subject: [PATCH 5/8] Ruby: Refactor EndCall to reduce number of classes --- ql/src/queries/performance/UseDetect.ql | 55 ++++++++++--------------- 1 file changed, 22 insertions(+), 33 deletions(-) diff --git a/ql/src/queries/performance/UseDetect.ql b/ql/src/queries/performance/UseDetect.ql index d75c4b83c25..262f7782548 100644 --- a/ql/src/queries/performance/UseDetect.ql +++ b/ql/src/queries/performance/UseDetect.ql @@ -13,42 +13,31 @@ import ruby -// Extracts the first or last element of a list -abstract class EndCall extends MethodCall { - abstract string detectCall(); -} +/** A call that extracts the first or last element of a list. */ +class EndCall extends MethodCall { + string detect; -abstract class First extends EndCall { - override string detectCall() { result = "detect" } -} - -class FirstCall extends First { - FirstCall() { - this.getMethodName() = "first" and - this.getNumberOfArguments() = 0 + EndCall() { + detect = "detect" and + ( + this.getMethodName() = "first" and + this.getNumberOfArguments() = 0 + or + this.getNumberOfArguments() = 1 and + this.getArgument(0).(IntegerLiteral).getValueText() = "0" + ) + or + detect = "reverse_detect" and + ( + this.getMethodName() = "last" and + this.getNumberOfArguments() = 0 + or + this.getNumberOfArguments() = 1 and + this.getArgument(0).(UnaryMinusExpr).getOperand().(IntegerLiteral).getValueText() = "1" + ) } -} -class FirstElement extends First, ElementReference { - FirstElement() { - this.getNumberOfArguments() = 1 and - this.getArgument(0).(IntegerLiteral).getValueText() = "0" - } -} - -abstract class Last extends EndCall { - override string detectCall() { result = "reverse_detect" } -} - -class LastCall extends Last { - LastCall() { this.getMethodName() = "last" and this.getNumberOfArguments() = 0 } -} - -class LastElement extends Last, ElementReference { - LastElement() { - this.getNumberOfArguments() = 1 and - this.getArgument(0).(UnaryMinusExpr).getOperand().(IntegerLiteral).getValueText() = "1" - } + string detectCall() { result = detect } } class SelectBlock extends MethodCall { From 5b4bf584a17d49dbbc4cdaf9ebe6af3a54ee6b1e Mon Sep 17 00:00:00 2001 From: Calum Grant Date: Tue, 9 Mar 2021 10:20:23 +0000 Subject: [PATCH 6/8] Ruby: Update qltest output for new select format --- .../performance/UseDetect/UseDetect.expected | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/ql/test/query-tests/performance/UseDetect/UseDetect.expected b/ql/test/query-tests/performance/UseDetect/UseDetect.expected index 2fd8b960ed9..9fa417a983d 100644 --- a/ql/test/query-tests/performance/UseDetect/UseDetect.expected +++ b/ql/test/query-tests/performance/UseDetect/UseDetect.expected @@ -1,6 +1,6 @@ -| UseDetect.rb:5:9:5:36 | call to first | Replace this call with 'detect'. | -| UseDetect.rb:6:9:6:35 | call to last | Replace this call with 'reverse_detect'. | -| UseDetect.rb:7:9:7:33 | ...[...] | Replace this call with 'detect'. | -| UseDetect.rb:8:9:8:34 | ...[...] | Replace this call with 'reverse_detect'. | -| UseDetect.rb:9:9:9:36 | call to first | Replace this call with 'detect'. | -| UseDetect.rb:10:9:10:37 | call to last | Replace this call with 'reverse_detect'. | +| UseDetect.rb:5:9:5:36 | call to first | Replace this call and $@ with 'detect'. | UseDetect.rb:5:9:5:30 | call to select | 'select' call | +| UseDetect.rb:6:9:6:35 | call to last | Replace this call and $@ with 'reverse_detect'. | UseDetect.rb:6:9:6:30 | call to select | 'select' call | +| UseDetect.rb:7:9:7:33 | ...[...] | Replace this call and $@ with 'detect'. | UseDetect.rb:7:9:7:30 | call to select | 'select' call | +| UseDetect.rb:8:9:8:34 | ...[...] | Replace this call and $@ with 'reverse_detect'. | UseDetect.rb:8:9:8:30 | call to select | 'select' call | +| UseDetect.rb:9:9:9:36 | call to first | Replace this call and $@ with 'detect'. | UseDetect.rb:9:9:9:30 | call to filter | 'select' call | +| UseDetect.rb:10:9:10:37 | call to last | Replace this call and $@ with 'reverse_detect'. | UseDetect.rb:10:9:10:32 | call to find_all | 'select' call | From 855d190800ac0b45469fbf2f95d397c2155c3e41 Mon Sep 17 00:00:00 2001 From: Calum Grant Date: Tue, 9 Mar 2021 10:25:24 +0000 Subject: [PATCH 7/8] Ruby: Test local data flow --- ql/src/queries/performance/UseDetect.ql | 17 +++++++++++++++-- .../performance/UseDetect/UseDetect.expected | 1 + .../performance/UseDetect/UseDetect.rb | 9 +++++++-- 3 files changed, 23 insertions(+), 4 deletions(-) diff --git a/ql/src/queries/performance/UseDetect.ql b/ql/src/queries/performance/UseDetect.ql index 262f7782548..a1cfc9d6b2a 100644 --- a/ql/src/queries/performance/UseDetect.ql +++ b/ql/src/queries/performance/UseDetect.ql @@ -12,6 +12,7 @@ */ import ruby +import codeql_ruby.dataflow.SSA /** A call that extracts the first or last element of a list. */ class EndCall extends MethodCall { @@ -40,6 +41,17 @@ class EndCall extends MethodCall { string detectCall() { result = detect } } +Expr getUniqueRead(Expr e) { + exists(AssignExpr ae | + e = ae.getRightOperand() and + forex(Ssa::WriteDefinition def | def.getWriteAccess() = ae.getLeftOperand() | + strictcount(def.getARead()) = 1 and + not def = any(Ssa::PhiNode phi).getAnInput() and + def.getARead() = result.getAControlFlowNode() + ) + ) +} + class SelectBlock extends MethodCall { SelectBlock() { this.getMethodName() in ["select", "filter", "find_all"] and @@ -48,5 +60,6 @@ class SelectBlock extends MethodCall { } from EndCall call, SelectBlock selectBlock -where selectBlock = call.getReceiver() -select call, "Replace this call with '" + call.detectCall() + "'." +where [selectBlock, getUniqueRead(selectBlock)] = call.getReceiver() +select call, "Replace this call and $@ with '" + call.detectCall() + "'.", selectBlock, + "'select' call" diff --git a/ql/test/query-tests/performance/UseDetect/UseDetect.expected b/ql/test/query-tests/performance/UseDetect/UseDetect.expected index 9fa417a983d..d8a5ec8dd41 100644 --- a/ql/test/query-tests/performance/UseDetect/UseDetect.expected +++ b/ql/test/query-tests/performance/UseDetect/UseDetect.expected @@ -4,3 +4,4 @@ | UseDetect.rb:8:9:8:34 | ...[...] | Replace this call and $@ with 'reverse_detect'. | UseDetect.rb:8:9:8:30 | call to select | 'select' call | | UseDetect.rb:9:9:9:36 | call to first | Replace this call and $@ with 'detect'. | UseDetect.rb:9:9:9:30 | call to filter | 'select' call | | UseDetect.rb:10:9:10:37 | call to last | Replace this call and $@ with 'reverse_detect'. | UseDetect.rb:10:9:10:32 | call to find_all | 'select' call | +| UseDetect.rb:12:9:12:24 | call to first | Replace this call and $@ with 'detect'. | UseDetect.rb:11:22:11:43 | call to select | 'select' call | diff --git a/ql/test/query-tests/performance/UseDetect/UseDetect.rb b/ql/test/query-tests/performance/UseDetect/UseDetect.rb index b0053e188d7..e1d2d9b91ba 100644 --- a/ql/test/query-tests/performance/UseDetect/UseDetect.rb +++ b/ql/test/query-tests/performance/UseDetect/UseDetect.rb @@ -8,9 +8,14 @@ class DetectTest [].select { |i| true }[-1] [].filter { |i| true }.first [].find_all { |i| true }.last + selection1 = [].select { |i| true } + selection1.first # These are good - [].select("").first - [].select { |i| true }[1] + [].select("").first # Selecting a string + [].select { |i| true }[1] # Selecting the second element + selection2 = [].select { |i| true } + selection2.first # Selection used elsewhere + selection3 = selection2 end end From cb977cb29091f3e3f4aa5704e4813262bbe6e6b7 Mon Sep 17 00:00:00 2001 From: Calum Grant Date: Wed, 10 Mar 2021 10:56:33 +0000 Subject: [PATCH 8/8] Ruby: Use getAUniqueRead TC --- ql/src/queries/performance/UseDetect.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ql/src/queries/performance/UseDetect.ql b/ql/src/queries/performance/UseDetect.ql index a1cfc9d6b2a..d17f5d6f8f9 100644 --- a/ql/src/queries/performance/UseDetect.ql +++ b/ql/src/queries/performance/UseDetect.ql @@ -60,6 +60,6 @@ class SelectBlock extends MethodCall { } from EndCall call, SelectBlock selectBlock -where [selectBlock, getUniqueRead(selectBlock)] = call.getReceiver() +where getUniqueRead*(selectBlock) = call.getReceiver() select call, "Replace this call and $@ with '" + call.detectCall() + "'.", selectBlock, "'select' call"