Skip to content

fix(lint): useless assignment for type definition #351

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Feb 19, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions spec/ameba/ast/variabling/type_def_variable_spec.cr
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
require "../../../spec_helper"

module Ameba::AST
describe TypeDecVariable do
var = Crystal::Var.new("foo")
declared_type = Crystal::Path.new("String")
type_dec = Crystal::TypeDeclaration.new(var, declared_type)

describe "#initialize" do
it "creates a new type dec variable" do
variable = TypeDecVariable.new(type_dec)
variable.node.should_not be_nil
end
end

describe "#name" do
it "returns var name" do
variable = TypeDecVariable.new(type_dec)
variable.name.should eq var.name
end

it "raises if type declaration is incorrect" do
type_dec = Crystal::TypeDeclaration.new(declared_type, declared_type)

expect_raises(Exception, "Unsupported var node type: Crystal::Path") do
TypeDecVariable.new(type_dec).name
end
end
end
end
end
13 changes: 13 additions & 0 deletions spec/ameba/rule/lint/shadowing_outer_local_var_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,19 @@ module Ameba::Rule::Lint
CRYSTAL
end

it "doesn't report if it shadows type declaration" do
expect_no_issues subject, <<-CRYSTAL
class FooBar
getter index : String

def bar
3.times do |index|
end
end
end
CRYSTAL
end

it "doesn't report if it shadows throwaway arguments" do
expect_no_issues subject, <<-CRYSTAL
data = [{1, "a"}, {2, "b"}, {3, "c"}]
Expand Down
29 changes: 29 additions & 0 deletions spec/ameba/rule/lint/useless_assign_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -475,6 +475,35 @@ module Ameba::Rule::Lint
issue.location.to_s.should eq "source.cr:1:1"
issue.message.should eq "Useless assignment to variable `foo`"
end

it "doesn't report if type declaration assigned inside module and referenced" do
s = Source.new %(
module A
foo : String? = "foo"

bar do
foo = "bar"
end

p foo
end

)
subject.catch(s).should be_valid
end

it "doesn't report if type declaration assigned inside class" do
s = Source.new %(
class A
foo : String? = "foo"

def method
foo = "bar"
end
end
)
subject.catch(s).should be_valid
end
end

context "branching" do
Expand Down
24 changes: 21 additions & 3 deletions src/ameba/ast/scope.cr
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ module Ameba::AST
# Link to the instance variables used in current scope
getter ivariables = [] of InstanceVariable

# Link to the type declaration variables used in current scope
getter type_dec_variables = [] of TypeDecVariable

# Link to the outer scope
getter outer_scope : Scope?

Expand Down Expand Up @@ -74,6 +77,16 @@ module Ameba::AST
ivariables << InstanceVariable.new(node)
end

# Adds a new type declaration variable to the current scope.
#
# ```
# scope = Scope.new(class_node, nil)
# scope.add_type_dec_variable(node)
# ```
def add_type_dec_variable(node)
type_dec_variables << TypeDecVariable.new(node)
end

# Returns variable by its name or `nil` if it does not exist.
#
# ```
Expand Down Expand Up @@ -122,8 +135,13 @@ module Ameba::AST

# Returns `true` if instance variable is assigned in this scope.
def assigns_ivar?(name)
arguments.find(&.name.== name) &&
ivariables.find(&.name.== "@#{name}")
arguments.any?(&.name.== name) &&
ivariables.any?(&.name.== "@#{name}")
end

# Returns `true` if type declaration variable is assigned in this scope.
def assigns_type_dec?(name)
type_dec_variables.any?(&.name.== name) || !!outer_scope.try(&.assigns_type_dec?(name))
end

# Returns `true` if and only if current scope represents some
Expand Down Expand Up @@ -161,7 +179,7 @@ module Ameba::AST

# Returns `true` if this scope is a top level scope, `false` otherwise.
def top_level?
outer_scope.nil?
outer_scope.nil? || type_definition?
end

# Returns `true` if var is an argument in current scope, `false` otherwise.
Expand Down
21 changes: 21 additions & 0 deletions src/ameba/ast/variabling/type_def_variable.cr
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
module Ameba::AST
class TypeDecVariable
getter node : Crystal::TypeDeclaration

delegate location, to: @node
delegate end_location, to: @node
delegate to_s, to: @node

def initialize(@node)
end

def name
case var = @node.var
when Crystal::Var, Crystal::InstanceVar, Crystal::ClassVar, Crystal::Global
var.name
else
raise "Unsupported var node type: #{var.class}"
end
end
end
end
13 changes: 11 additions & 2 deletions src/ameba/ast/visitors/scope_visitor.cr
Original file line number Diff line number Diff line change
Expand Up @@ -102,10 +102,19 @@ module Ameba::AST

# :nodoc:
def visit(node : Crystal::TypeDeclaration)
return if @current_scope.type_definition?
return if !(var = node.var).is_a?(Crystal::Var)
return unless (var = node.var).is_a?(Crystal::Var)

@current_scope.add_variable(var)
@current_scope.add_type_dec_variable(node)
@current_assign = node.value unless node.value.nil?
end

def end_visit(node : Crystal::TypeDeclaration)
return unless (var = node.var).is_a?(Crystal::Var)

on_assign_end(var, node)
@current_assign = nil
on_scope_end(node) if @current_scope.eql?(node)
end

# :nodoc:
Expand Down
1 change: 1 addition & 0 deletions src/ameba/rule/lint/shadowing_outer_local_var.cr
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ module Ameba::Rule::Lint

next if variable.nil? || !variable.declared_before?(arg)
next if outer_scope.assigns_ivar?(arg.name)
next if outer_scope.assigns_type_dec?(arg.name)

issue_for arg.node, MSG % arg.name
end
Expand Down
1 change: 1 addition & 0 deletions src/ameba/rule/lint/useless_assign.cr
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ module Ameba::Rule::Lint
def test(source, node, scope : AST::Scope)
scope.variables.each do |var|
next if var.ignored? || var.used_in_macro? || var.captured_by_block?
next if scope.assigns_type_dec?(var.name)

var.assignments.each do |assign|
next if assign.referenced? || assign.transformed?
Expand Down