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 3 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
30 changes: 30 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,30 @@
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 definition" 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
26 changes: 26 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,32 @@ 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 top level variable defined inside class is referenced" do
s = Source.new %(
class A
foo : String? = "foo"
end

puts foo
)
subject.catch(s).should be_valid
end

it "doesn't report if top level variable assigned inside class and referenced" do
s = Source.new %(
class A
foo : String? = "foo"

bar do
foo = "bar"
end
end

puts foo
)
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
23 changes: 23 additions & 0 deletions src/ameba/ast/variabling/type_def_variable.cr
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
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
var = @node.var

case 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