Ameba is a linter for Crystal.
I wrote a bit of Crystal code recently, so let's see how it goes.
I'll run it in both default settings and with --all. Hopefully there will be few false positives with default settings.
Shebang issue
ameba was ignoring Crystal scripts starting with #!/usr/bin/env crystal - it was only looking at files with .cr extension. This is not Crystal's main use case, but it's a valid use case.
Crystal's VSCode extension has the same issue.
The issue can be solved with .ameba.yml config file.
crystal-z3 and Lint/UselessAssign
In default settings it finds one issue:
$ ameba
Inspecting 25 files
......................F..
src/z3/api.cr:89:7
[W] Lint/UselessAssign: Useless assignment to variable `var`
> var = LibZ3.mk_const(Context, name_sym, sort)
^-^
Finished in 223.71 milliseconds
25 inspected, 1 failure
It doesn't show any context, but it's from this method:
def mk_const(name, sort)
name_sym = LibZ3.mk_string_symbol(Context, name)
var = LibZ3.mk_const(Context, name_sym, sort)
end
That's definitely real.
crystal-z3 with --all and Lint/ComparisonToBoolean
This generates a huge number of Lint/ComparisonToBoolean issues, some examples here:
spec/bool_spec.cr:10:6 [Correctable]
[W] Lint/ComparisonToBoolean: Comparison to a boolean is pointless
> [a == true, b == true, c == (a & b)].should have_solution({c => true})
^--------^
spec/model_spec.cr:15:19 [Correctable]
[W] Lint/ComparisonToBoolean: Comparison to a boolean is pointless
> solver.assert a == true
^-------^
[W] Lint/ComparisonToBoolean: Comparison to a boolean is pointless
> raise Z3::Exception.new("Cannot evaluate") unless result == true
^------------^
Well, crystal-z3 overrides Z3::BoolExpr#== so semantically this lint rule is a miss, but I cannot blame ameba for not supporting such unusual code.
I'm not sure this is a good rule in general. As long as x can be anything else than a boolean, x == true is absolutely not the same thing as x.
Thue Interptetter and Performance/CompactAfterMap
There are two issues it found with Thue interpretter from the series:
./episode-65-crystal-thue-randomized-finite-automaton/thue_rfa.cr:162:27
[C] Performance/CompactAfterMap: Use `compact_map {...}` instead of `map {...}.compact`
> next_tries = active.map{|t| t[char]}.compact
^-----------------------^
compact_map is a decent suggestion, as these merged common operations tend to be more performant, but methods like that keep getting added every minor version of every language, it's hard to remember which language has which combinations.
./episode-65-crystal-thue-randomized-finite-automaton/thue_rfa.cr:191:13
[W] Lint/UselessAssign: Useless assignment to variable `line_no`
> line, line_no = lines.shift
^-----^
I don't like this one. Useless single variable assignment can be removed, so that makes sense, but in array destructuring, the alternatives are ugly code like one of:
line, _ = lines.shift
line, _line_no = lines.shift
line = lines.shift[0]
I'd rather stick with the original.
But that's not specific to ameba, most linters' "Useless assignment to variable" check don't have exception for multiple assignment.
For a note, if it was a named tuple, we could do some equivalent of (JavaScript here):
let {line} = lines.shift()
For destructuring a hash / named tuple / object, there's no reason to include these extras.
Thue Interpretter and --all
As we're not using --all, false positives are expected. Here's one example, there's more cases like that:
./episode-65-crystal-thue-randomized-finite-automaton/thue_rfa.cr:221:5 [Correctable]
[C] Style/GuardClause: Use a guard clause (`return unless debug`) instead of wrapping the code inside a conditional expression.
> if debug
^^
So linter wants us to replace this:
def run(debug)
@state = @initial
if debug
@rules.each do |rule|
STDERR.puts rule
end
end
while match = @rfa.not_nil!.random_match(@state)
rule = match[:rule]
idx = match[:idx]
if debug
STDERR.puts "Applying rule #{rule} at #{idx} to #{@state.inspect}"
end
@state = rule.apply(@state, idx)
end
if debug
STDERR.puts "No more matches. Final state: #{@state.inspect}"
end
end
With this:
def run(debug)
@state = @initial
if debug
@rules.each do |rule|
STDERR.puts rule
end
end
while match = @rfa.not_nil!.random_match(@state)
rule = match[:rule]
idx = match[:idx]
if debug
STDERR.puts "Applying rule #{rule} at #{idx} to #{@state.inspect}"
end
@state = rule.apply(@state, idx)
end
return unless debug
STDERR.puts "No more matches. Final state: #{@state.inspect}"
end
That would not be an improvement. Guard clauses at the beginning of a method or loop body are common pattern, but putting them after the main body is weird.
Open Source Adventures and Style/VerboseBlock
To get ameba to see all the files I need to run it with weird:
$ ameba `git grep -l '#!/usr/bin/env crystal'` `git ls "*.cr"`
Here are some of the suggestions:
episode-65/minesweeper.cr:40:25 [Correctable]
[C] Style/VerboseBlock: Use short block notation instead: `map(&.ite(1, 0))`
> neighbourhood(x, y).map{|v| v.ite(1, 0)}.reduce{|a,b| a+b}
^------------------^
That's Crystal code not directly doable in Ruby. It's a reasonable suggestion.
episode-70/aquarium:17:25 [Correctable]
[C] Style/VerboseBlock: Use short block notation instead: `map(&.[2..].chars)`
> @board = lines[2..].map{|l| l[2..].chars}
^-------------------^
This feels like maybe a bit much, but maybe I could get used to it.
episode-72/dominosa:65:23
[W] Lint/UnusedArgument: Unused argument `type`. If it's necessary, use `_` as an argument name to indicate that it won't be used.
> @dominos.each do |type, vars|
^
This one can use .each_value, but just like with multiple assignment, I don't like the suggestion of @dominos.each do |_type, vars| or @dominos.each do |_, vars|.
episode-68/switches:50:67 [Correctable]
[C] Performance/ChainedCallWithNoBang: Use bang method variant `sort!` after chained `select` call
> puts @nodes.select{|n| model[@switches[n]].to_s == "true" }.sort.join(" ")
^--^
I see why it makes sense for performance, but .sort! in the middle of a chain is so weird, I'd rather not.
There weren't any new types of suggestions with --all.
Should you use ameba?
Overall it seems like a decent linter, and defaults don't generate too many false positives.
It doesn't do any kind of deep analysis. For example I know I had some if checks that are always true due to type issue (Z3 results LBool enum, not Bool, so result is always true), but ameba never picked that up, as that doesn't show in a purely syntactic check.
Coming next
OK, that's really enough Crystal for now. In the next episode we'll try another technology as promised.
Top comments (0)