mirror of
https://gitea.invidious.io/iv-org/shard-ameba.git
synced 2024-08-15 00:53:29 +00:00
Merge pull request #201 from crystal-ameba/feature/verbose-block-rule
Add Style/VerboseBlock rule
This commit is contained in:
commit
a169b12b99
7 changed files with 444 additions and 8 deletions
|
@ -120,7 +120,7 @@ module Ameba
|
||||||
it "returns list of sources" do
|
it "returns list of sources" do
|
||||||
config.sources.size.should be > 0
|
config.sources.size.should be > 0
|
||||||
config.sources.first.should be_a Source
|
config.sources.first.should be_a Source
|
||||||
config.sources.any? { |s| s.fullpath == __FILE__ }.should be_true
|
config.sources.any?(&.fullpath.==(__FILE__)).should be_true
|
||||||
end
|
end
|
||||||
|
|
||||||
it "returns a list of sources mathing globs" do
|
it "returns a list of sources mathing globs" do
|
||||||
|
@ -130,7 +130,7 @@ module Ameba
|
||||||
|
|
||||||
it "returns a lisf of sources excluding 'Excluded'" do
|
it "returns a lisf of sources excluding 'Excluded'" do
|
||||||
config.excluded = %w(**/config_spec.cr)
|
config.excluded = %w(**/config_spec.cr)
|
||||||
config.sources.any? { |s| s.fullpath == __FILE__ }.should be_false
|
config.sources.any?(&.fullpath.==(__FILE__)).should be_false
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
|
214
spec/ameba/rule/style/verbose_block_spec.cr
Normal file
214
spec/ameba/rule/style/verbose_block_spec.cr
Normal file
|
@ -0,0 +1,214 @@
|
||||||
|
require "../../../spec_helper"
|
||||||
|
|
||||||
|
module Ameba::Rule::Style
|
||||||
|
subject = VerboseBlock.new
|
||||||
|
|
||||||
|
describe VerboseBlock do
|
||||||
|
it "passes if there is no potential performance improvements" do
|
||||||
|
source = Source.new %(
|
||||||
|
(1..3).any?(&.odd?)
|
||||||
|
(1..3).join('.', &.to_s)
|
||||||
|
(1..3).each_with_index { |i, idx| i * idx }
|
||||||
|
(1..3).map { |i| typeof(i) }
|
||||||
|
(1..3).map { |i| i || 0 }
|
||||||
|
(1..3).map { |i| :foo }
|
||||||
|
(1..3).map { |i| :foo.to_s.split.join('.') }
|
||||||
|
(1..3).map { :foo }
|
||||||
|
)
|
||||||
|
subject.catch(source).should be_valid
|
||||||
|
end
|
||||||
|
|
||||||
|
it "passes if the block argument is used within the body" do
|
||||||
|
source = Source.new %(
|
||||||
|
(1..3).map { |i| i * i }
|
||||||
|
(1..3).map { |j| j * j.to_i64 }
|
||||||
|
(1..3).map { |k| k.to_i64 * k }
|
||||||
|
(1..3).map { |l| l.to_i64 * l.to_i64 }
|
||||||
|
(1..3).map { |m| m.to_s[start: m.to_i64, count: 3]? }
|
||||||
|
(1..3).map { |n| n.to_s.split.map { |z| n.to_i * z.to_i }.join }
|
||||||
|
)
|
||||||
|
subject.catch(source).should be_valid
|
||||||
|
end
|
||||||
|
|
||||||
|
it "reports if there is a call with a collapsible block" do
|
||||||
|
source = Source.new %(
|
||||||
|
(1..3).any? { |i| i.odd? }
|
||||||
|
)
|
||||||
|
subject.catch(source).should_not be_valid
|
||||||
|
end
|
||||||
|
|
||||||
|
it "reports if there is a call with an argument + collapsible block" do
|
||||||
|
source = Source.new %(
|
||||||
|
(1..3).join('.') { |i| i.to_s }
|
||||||
|
)
|
||||||
|
subject.catch(source).should_not be_valid
|
||||||
|
end
|
||||||
|
|
||||||
|
it "reports if there is a call with a collapsible block (with chained call)" do
|
||||||
|
source = Source.new %(
|
||||||
|
(1..3).map { |i| i.to_s.split.reverse.join.strip }
|
||||||
|
)
|
||||||
|
subject.catch(source).should_not be_valid
|
||||||
|
end
|
||||||
|
|
||||||
|
context "properties" do
|
||||||
|
it "#exclude_calls_with_block" do
|
||||||
|
source = Source.new %(
|
||||||
|
(1..3).in_groups_of(1) { |i| i.map(&.to_s) }
|
||||||
|
)
|
||||||
|
rule = VerboseBlock.new
|
||||||
|
rule
|
||||||
|
.tap(&.exclude_calls_with_block = true)
|
||||||
|
.catch(source).should be_valid
|
||||||
|
rule
|
||||||
|
.tap(&.exclude_calls_with_block = false)
|
||||||
|
.catch(source).should_not be_valid
|
||||||
|
end
|
||||||
|
|
||||||
|
it "#exclude_multiple_line_blocks" do
|
||||||
|
source = Source.new %(
|
||||||
|
(1..3).any? do |i|
|
||||||
|
i.odd?
|
||||||
|
end
|
||||||
|
)
|
||||||
|
rule = VerboseBlock.new
|
||||||
|
rule
|
||||||
|
.tap(&.exclude_multiple_line_blocks = true)
|
||||||
|
.catch(source).should be_valid
|
||||||
|
rule
|
||||||
|
.tap(&.exclude_multiple_line_blocks = false)
|
||||||
|
.catch(source).should_not be_valid
|
||||||
|
end
|
||||||
|
|
||||||
|
it "#exclude_prefix_operators" do
|
||||||
|
source = Source.new %(
|
||||||
|
(1..3).sum { |i| +i }
|
||||||
|
(1..3).sum { |i| -i }
|
||||||
|
)
|
||||||
|
rule = VerboseBlock.new
|
||||||
|
rule
|
||||||
|
.tap(&.exclude_prefix_operators = true)
|
||||||
|
.catch(source).should be_valid
|
||||||
|
rule
|
||||||
|
.tap(&.exclude_prefix_operators = false)
|
||||||
|
.catch(source).should_not be_valid
|
||||||
|
end
|
||||||
|
|
||||||
|
it "#exclude_operators" do
|
||||||
|
source = Source.new %(
|
||||||
|
(1..3).sum { |i| i * 2 }
|
||||||
|
)
|
||||||
|
rule = VerboseBlock.new
|
||||||
|
rule
|
||||||
|
.tap(&.exclude_operators = true)
|
||||||
|
.catch(source).should be_valid
|
||||||
|
rule
|
||||||
|
.tap(&.exclude_operators = false)
|
||||||
|
.catch(source).should_not be_valid
|
||||||
|
end
|
||||||
|
|
||||||
|
it "#exclude_setters" do
|
||||||
|
source = Source.new %(
|
||||||
|
Char::Reader.new("abc").tap { |reader| reader.pos = 0 }
|
||||||
|
)
|
||||||
|
rule = VerboseBlock.new
|
||||||
|
rule
|
||||||
|
.tap(&.exclude_setters = true)
|
||||||
|
.catch(source).should be_valid
|
||||||
|
rule
|
||||||
|
.tap(&.exclude_setters = false)
|
||||||
|
.catch(source).should_not be_valid
|
||||||
|
end
|
||||||
|
|
||||||
|
it "#max_line_length" do
|
||||||
|
source = Source.new %(
|
||||||
|
(1..3).tap &.tap &.tap &.tap &.tap &.tap &.tap do |i|
|
||||||
|
i.to_s.reverse.strip.blank?
|
||||||
|
end
|
||||||
|
)
|
||||||
|
rule = VerboseBlock.new
|
||||||
|
rule
|
||||||
|
.tap(&.max_line_length = 60)
|
||||||
|
.catch(source).should be_valid
|
||||||
|
rule
|
||||||
|
.tap(&.max_line_length = nil)
|
||||||
|
.catch(source).should_not be_valid
|
||||||
|
end
|
||||||
|
|
||||||
|
it "#max_length" do
|
||||||
|
source = Source.new %(
|
||||||
|
(1..3).tap { |i| i.to_s.split.reverse.join.strip.blank? }
|
||||||
|
)
|
||||||
|
rule = VerboseBlock.new
|
||||||
|
rule
|
||||||
|
.tap(&.max_length = 30)
|
||||||
|
.catch(source).should be_valid
|
||||||
|
rule
|
||||||
|
.tap(&.max_length = nil)
|
||||||
|
.catch(source).should_not be_valid
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
context "macro" do
|
||||||
|
it "reports in macro scope" do
|
||||||
|
source = Source.new %(
|
||||||
|
{{ (1..3).any? { |i| i.odd? } }}
|
||||||
|
)
|
||||||
|
subject.catch(source).should_not be_valid
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
it "reports call args and named_args" do
|
||||||
|
short_block_variants = {
|
||||||
|
%|map(&.to_s.[start: 0.to_i64, count: 3]?)|,
|
||||||
|
%|map(&.to_s.[0.to_i64, count: 3]?)|,
|
||||||
|
%|map(&.to_s.[0.to_i64, 3]?)|,
|
||||||
|
%|map(&.to_s.[start: 0.to_i64, count: 3]=("foo"))|,
|
||||||
|
%|map(&.to_s.[0.to_i64, count: 3]=("foo"))|,
|
||||||
|
%|map(&.to_s.[0.to_i64, 3]=("foo"))|,
|
||||||
|
%|map(&.to_s.camelcase(lower: true))|,
|
||||||
|
%|map(&.to_s.camelcase)|,
|
||||||
|
%|map(&.to_s.gsub('_', '-'))|,
|
||||||
|
%|map(&.in?(*{1, 2, 3}, **{foo: :bar}))|,
|
||||||
|
%|map(&.in?(1, *foo, 3, **bar))|,
|
||||||
|
%|join(separator: '.', &.to_s)|,
|
||||||
|
}
|
||||||
|
|
||||||
|
source = Source.new path: "source.cr", code: %(
|
||||||
|
(1..3).map { |i| i.to_s[start: 0.to_i64, count: 3]? }
|
||||||
|
(1..3).map { |i| i.to_s[0.to_i64, count: 3]? }
|
||||||
|
(1..3).map { |i| i.to_s[0.to_i64, 3]? }
|
||||||
|
(1..3).map { |i| i.to_s[start: 0.to_i64, count: 3] = "foo" }
|
||||||
|
(1..3).map { |i| i.to_s[0.to_i64, count: 3] = "foo" }
|
||||||
|
(1..3).map { |i| i.to_s[0.to_i64, 3] = "foo" }
|
||||||
|
(1..3).map { |i| i.to_s.camelcase(lower: true) }
|
||||||
|
(1..3).map { |i| i.to_s.camelcase }
|
||||||
|
(1..3).map { |i| i.to_s.gsub('_', '-') }
|
||||||
|
(1..3).map { |i| i.in?(*{1, 2, 3}, **{foo: :bar}) }
|
||||||
|
(1..3).map { |i| i.in?(1, *foo, 3, **bar) }
|
||||||
|
(1..3).join(separator: '.') { |i| i.to_s }
|
||||||
|
)
|
||||||
|
subject.catch(source).should_not be_valid
|
||||||
|
source.issues.size.should eq(short_block_variants.size)
|
||||||
|
|
||||||
|
source.issues.each_with_index do |issue, i|
|
||||||
|
issue.message.should eq(VerboseBlock::MSG % short_block_variants[i])
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
it "reports rule, pos and message" do
|
||||||
|
source = Source.new path: "source.cr", code: %(
|
||||||
|
(1..3).any? { |i| i.odd? }
|
||||||
|
)
|
||||||
|
subject.catch(source).should_not be_valid
|
||||||
|
source.issues.size.should eq 1
|
||||||
|
|
||||||
|
issue = source.issues.first
|
||||||
|
issue.rule.should_not be_nil
|
||||||
|
issue.location.to_s.should eq "source.cr:1:8"
|
||||||
|
issue.end_location.to_s.should eq "source.cr:1:26"
|
||||||
|
|
||||||
|
issue.message.should eq "Use short block notation instead: `any?(&.odd?)`"
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
|
@ -4,9 +4,7 @@ module Ameba
|
||||||
describe Severity do
|
describe Severity do
|
||||||
describe "#symbol" do
|
describe "#symbol" do
|
||||||
it "returns the symbol for each severity in the enum" do
|
it "returns the symbol for each severity in the enum" do
|
||||||
Severity.values.each do |severity|
|
Severity.values.each(&.symbol.should_not(be_nil))
|
||||||
severity.symbol.should_not be_nil
|
|
||||||
end
|
|
||||||
end
|
end
|
||||||
|
|
||||||
it "returns symbol for Error" do
|
it "returns symbol for Error" do
|
||||||
|
|
|
@ -78,7 +78,7 @@ module Ameba::AST
|
||||||
# scope.find_variable("foo")
|
# scope.find_variable("foo")
|
||||||
# ```
|
# ```
|
||||||
def find_variable(name : String)
|
def find_variable(name : String)
|
||||||
variables.find { |v| v.name == name } || outer_scope.try &.find_variable(name)
|
variables.find(&.name.==(name)) || outer_scope.try &.find_variable(name)
|
||||||
end
|
end
|
||||||
|
|
||||||
# Creates a new assignment for the variable.
|
# Creates a new assignment for the variable.
|
||||||
|
|
|
@ -11,7 +11,7 @@ module Ameba
|
||||||
rejected = rejected_globs(globs)
|
rejected = rejected_globs(globs)
|
||||||
selected = globs - rejected
|
selected = globs - rejected
|
||||||
|
|
||||||
expand(selected) - expand(rejected.map! { |p| p[1..-1] })
|
expand(selected) - expand(rejected.map!(&.[1..-1]))
|
||||||
end
|
end
|
||||||
|
|
||||||
# Expands globs. Globs can point to files or even directories.
|
# Expands globs. Globs can point to files or even directories.
|
||||||
|
|
224
src/ameba/rule/style/verbose_block.cr
Normal file
224
src/ameba/rule/style/verbose_block.cr
Normal file
|
@ -0,0 +1,224 @@
|
||||||
|
module Ameba::Rule::Style
|
||||||
|
# This rule is used to identify usage of single expression blocks with
|
||||||
|
# argument as a receiver, that can be collapsed into a short form.
|
||||||
|
#
|
||||||
|
# For example, this is considered invalid:
|
||||||
|
#
|
||||||
|
# ```
|
||||||
|
# (1..3).any? { |i| i.odd? }
|
||||||
|
# ```
|
||||||
|
#
|
||||||
|
# And it should be written as this:
|
||||||
|
#
|
||||||
|
# ```
|
||||||
|
# (1..3).any?(&.odd?)
|
||||||
|
# ```
|
||||||
|
#
|
||||||
|
# YAML configuration example:
|
||||||
|
#
|
||||||
|
# ```
|
||||||
|
# Style/VerboseBlock:
|
||||||
|
# Enabled: true
|
||||||
|
# ExcludeMultipleLineBlocks: true
|
||||||
|
# ExcludeCallsWithBlocks: false
|
||||||
|
# ExcludePrefixOperators: true
|
||||||
|
# ExcludeOperators: false
|
||||||
|
# ExcludeSetters: false
|
||||||
|
# MaxLineLength: ~
|
||||||
|
# MaxLength: 50 # use ~ to disable
|
||||||
|
# ```
|
||||||
|
class VerboseBlock < Base
|
||||||
|
properties do
|
||||||
|
description "Identifies usage of collapsible single expression blocks."
|
||||||
|
|
||||||
|
exclude_calls_with_block true
|
||||||
|
exclude_multiple_line_blocks false
|
||||||
|
exclude_prefix_operators true
|
||||||
|
exclude_operators false
|
||||||
|
exclude_setters false
|
||||||
|
|
||||||
|
max_line_length : Int32? = nil # 100
|
||||||
|
max_length : Int32? = 50
|
||||||
|
end
|
||||||
|
|
||||||
|
MSG = "Use short block notation instead: `%s`"
|
||||||
|
CALL_PATTERN = "%s(%s&.%s)"
|
||||||
|
|
||||||
|
protected def same_location_lines?(a, b)
|
||||||
|
return unless a_location = a.name_location
|
||||||
|
return unless b_location = b.location
|
||||||
|
|
||||||
|
a_location.line_number == b_location.line_number
|
||||||
|
end
|
||||||
|
|
||||||
|
private PREFIX_OPERATORS = {"+", "-"}
|
||||||
|
private OPERATOR_CHARS =
|
||||||
|
{'[', ']', '!', '=', '>', '<', '~', '+', '-', '*', '/', '%', '^', '|', '&'}
|
||||||
|
|
||||||
|
protected def prefix_operator?(node)
|
||||||
|
node.name.in?(PREFIX_OPERATORS) && node.args.empty?
|
||||||
|
end
|
||||||
|
|
||||||
|
protected def operator?(name)
|
||||||
|
name.each_char do |char|
|
||||||
|
return false unless char.in?(OPERATOR_CHARS)
|
||||||
|
end
|
||||||
|
!name.empty?
|
||||||
|
end
|
||||||
|
|
||||||
|
protected def setter?(name)
|
||||||
|
!name.empty? && name[0].letter? && name.ends_with?('=')
|
||||||
|
end
|
||||||
|
|
||||||
|
protected def valid_length?(code)
|
||||||
|
if max_length = self.max_length
|
||||||
|
return code.size <= max_length
|
||||||
|
end
|
||||||
|
true
|
||||||
|
end
|
||||||
|
|
||||||
|
protected def valid_line_length?(node, code)
|
||||||
|
if max_line_length = self.max_line_length
|
||||||
|
if location = node.name_location
|
||||||
|
final_line_length = location.column_number + code.size
|
||||||
|
return final_line_length <= max_line_length
|
||||||
|
end
|
||||||
|
end
|
||||||
|
true
|
||||||
|
end
|
||||||
|
|
||||||
|
protected def reference_count(node, obj : Crystal::Var)
|
||||||
|
i = 0
|
||||||
|
case node
|
||||||
|
when Crystal::Call
|
||||||
|
i += reference_count(node.obj, obj)
|
||||||
|
i += reference_count(node.block, obj)
|
||||||
|
|
||||||
|
node.args.each do |arg|
|
||||||
|
i += reference_count(arg, obj)
|
||||||
|
end
|
||||||
|
node.named_args.try &.each do |arg|
|
||||||
|
i += reference_count(arg.value, obj)
|
||||||
|
end
|
||||||
|
when Crystal::Block
|
||||||
|
i += reference_count(node.body, obj)
|
||||||
|
when Crystal::Var
|
||||||
|
i += 1 if node == obj
|
||||||
|
end
|
||||||
|
i
|
||||||
|
end
|
||||||
|
|
||||||
|
protected def args_to_s(io : IO, node : Crystal::Call, skip_last_arg = false)
|
||||||
|
node.args.dup.tap do |args|
|
||||||
|
args.pop? if skip_last_arg
|
||||||
|
args.join io, ", "
|
||||||
|
node.named_args.try do |named_args|
|
||||||
|
io << ", " unless args.empty? || named_args.empty?
|
||||||
|
named_args.join io, ", " do |arg, inner_io|
|
||||||
|
inner_io << arg.name << ": " << arg.value
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
protected def node_to_s(node : Crystal::Call)
|
||||||
|
String.build do |str|
|
||||||
|
case name = node.name
|
||||||
|
when "[]"
|
||||||
|
str << '['
|
||||||
|
args_to_s(str, node)
|
||||||
|
str << ']'
|
||||||
|
when "[]?"
|
||||||
|
str << '['
|
||||||
|
args_to_s(str, node)
|
||||||
|
str << "]?"
|
||||||
|
when "[]="
|
||||||
|
str << '['
|
||||||
|
args_to_s(str, node, skip_last_arg: true)
|
||||||
|
str << "]=(" << node.args.last? << ')'
|
||||||
|
else
|
||||||
|
str << name
|
||||||
|
if !node.args.empty? || (node.named_args && !node.named_args.try(&.empty?))
|
||||||
|
str << '('
|
||||||
|
args_to_s(str, node)
|
||||||
|
str << ')'
|
||||||
|
end
|
||||||
|
str << " {...}" if node.block
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
protected def call_code(call, body)
|
||||||
|
args = String.build { |io| args_to_s(io, call) }.presence
|
||||||
|
args += ", " if args
|
||||||
|
|
||||||
|
call_chain = %w[].tap do |arr|
|
||||||
|
obj = body.obj
|
||||||
|
while obj.is_a?(Crystal::Call)
|
||||||
|
arr << node_to_s(obj)
|
||||||
|
obj = obj.obj
|
||||||
|
end
|
||||||
|
arr.reverse!
|
||||||
|
arr << node_to_s(body)
|
||||||
|
end
|
||||||
|
|
||||||
|
name =
|
||||||
|
call_chain.join('.')
|
||||||
|
|
||||||
|
CALL_PATTERN % {call.name, args, name}
|
||||||
|
end
|
||||||
|
|
||||||
|
# ameba:disable Metrics/CyclomaticComplexity
|
||||||
|
protected def issue_for_valid(source, call : Crystal::Call, body : Crystal::Call)
|
||||||
|
return if exclude_calls_with_block && body.block
|
||||||
|
return if exclude_multiple_line_blocks && !same_location_lines?(call, body)
|
||||||
|
return if exclude_prefix_operators && prefix_operator?(body)
|
||||||
|
return if exclude_operators && operator?(body.name)
|
||||||
|
return if exclude_setters && setter?(body.name)
|
||||||
|
|
||||||
|
call_code =
|
||||||
|
call_code(call, body)
|
||||||
|
|
||||||
|
return unless valid_line_length?(call, call_code)
|
||||||
|
return unless valid_length?(call_code)
|
||||||
|
|
||||||
|
issue_for call.name_location, call.end_location,
|
||||||
|
MSG % call_code
|
||||||
|
end
|
||||||
|
|
||||||
|
def test(source, node : Crystal::Call)
|
||||||
|
# we are interested only in calls with block taking a single argument
|
||||||
|
#
|
||||||
|
# ```
|
||||||
|
# (1..3).any? { |i| i.to_i64.odd? }
|
||||||
|
# ^--- ^ ^------------
|
||||||
|
# block arg body
|
||||||
|
# ```
|
||||||
|
return unless (block = node.block) && block.args.size == 1
|
||||||
|
|
||||||
|
arg = block.args.first
|
||||||
|
|
||||||
|
# we skip auto-generated blocks - `(1..3).any?(&.odd?)`
|
||||||
|
return if arg.name.starts_with?("__arg")
|
||||||
|
|
||||||
|
# we filter out the blocks that are of call type - `i.to_i64.odd?`
|
||||||
|
return unless (body = block.body).is_a?(Crystal::Call)
|
||||||
|
|
||||||
|
# we need to "unwind" the chain challs, so the final receiver object
|
||||||
|
# ends up being a variable - `i`
|
||||||
|
obj = body.obj
|
||||||
|
while obj.is_a?(Crystal::Call)
|
||||||
|
obj = obj.obj
|
||||||
|
end
|
||||||
|
|
||||||
|
# only calls with a first argument used as a receiver are the valid game
|
||||||
|
return unless obj == arg
|
||||||
|
|
||||||
|
# we bail out if the block node include the block argument
|
||||||
|
return if reference_count(body, arg) > 1
|
||||||
|
|
||||||
|
# add issue if the given nodes pass all of the checks
|
||||||
|
issue_for_valid source, node, body
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
|
@ -126,7 +126,7 @@ module Ameba
|
||||||
@sources.all? do |source|
|
@sources.all? do |source|
|
||||||
source.issues
|
source.issues
|
||||||
.reject(&.disabled?)
|
.reject(&.disabled?)
|
||||||
.none? { |issue| issue.rule.severity <= @severity }
|
.none?(&.rule.severity.<=(@severity))
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
|
Loading…
Reference in a new issue