Skip to content

Commit

Permalink
Make abstract def return type warning an error (#9810)
Browse files Browse the repository at this point in the history
* Make abstract def return type warning an error

Fixes #9655

* Drop AbstractDefImplementationError

* Drop skip_abstract_def_check flag
  • Loading branch information
Brian J. Cardiff authored Oct 9, 2020
1 parent 8c182b8 commit 0fa9550
Show file tree
Hide file tree
Showing 3 changed files with 24 additions and 28 deletions.
30 changes: 15 additions & 15 deletions spec/compiler/semantic/abstract_def_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -395,8 +395,8 @@ describe "Semantic: abstract def" do
)
end

it "warning if missing return type" do
assert_warning <<-CR,
it "errors if missing return type" do
assert_error <<-CR,
abstract class Foo
abstract def foo : Int32
end
Expand All @@ -407,11 +407,11 @@ describe "Semantic: abstract def" do
end
end
CR
"warning in line 6\nWarning: this method overrides Foo#foo() which has an explicit return type of Int32.\n\nPlease add an explicit return type (Int32 or a subtype of it) to this method as well."
"this method overrides Foo#foo() which has an explicit return type of Int32.\n\nPlease add an explicit return type (Int32 or a subtype of it) to this method as well."
end

it "warning if different return type" do
assert_warning <<-CR,
it "errors if different return type" do
assert_error <<-CR,
abstract class Foo
abstract def foo : Int32
end
Expand All @@ -425,7 +425,7 @@ describe "Semantic: abstract def" do
end
end
CR
"warning in line 9\nWarning: this method must return Int32, which is the return type of the overridden method Foo#foo(), or a subtype of it, not Bar::Int32"
"this method must return Int32, which is the return type of the overridden method Foo#foo(), or a subtype of it, not Bar::Int32"
end

it "can return a more specific type" do
Expand Down Expand Up @@ -542,8 +542,8 @@ describe "Semantic: abstract def" do
))
end

it "is missing a return type in subclass of generic subclass" do
assert_warning <<-CR,
it "errors if missing a return type in subclass of generic subclass" do
assert_error <<-CR,
abstract class Foo(T)
abstract def foo : T
end
Expand All @@ -553,11 +553,11 @@ describe "Semantic: abstract def" do
end
end
CR
"warning in line 6\nWarning: this method overrides Foo(T)#foo() which has an explicit return type of T.\n\nPlease add an explicit return type (Int32 or a subtype of it) to this method as well."
"this method overrides Foo(T)#foo() which has an explicit return type of T.\n\nPlease add an explicit return type (Int32 or a subtype of it) to this method as well."
end

it "can't find parent return type" do
assert_warning <<-CR,
it "errors if can't find parent return type" do
assert_error <<-CR,
abstract class Foo
abstract def foo : Unknown
end
Expand All @@ -567,11 +567,11 @@ describe "Semantic: abstract def" do
end
end
CR
"warning in line 2\nWarning: can't resolve return type Unknown"
"can't resolve return type Unknown"
end

it "can't find child return type" do
assert_warning <<-CR,
it "errors if can't find child return type" do
assert_error <<-CR,
abstract class Foo
abstract def foo : Int32
end
Expand All @@ -581,7 +581,7 @@ describe "Semantic: abstract def" do
end
end
CR
"warning in line 6\nWarning: can't resolve return type Unknown"
"can't resolve return type Unknown"
end

it "doesn't crash when abstract method is implemented by supertype (#8031)" do
Expand Down
8 changes: 2 additions & 6 deletions src/compiler/crystal/semantic.cr
Original file line number Diff line number Diff line change
Expand Up @@ -70,12 +70,8 @@ class Crystal::Program
TypeDeclarationProcessor.new(self).process(node)
end

# TODO: remove this check a couple of versions after 0.30.0 once
# we are sure it's working fine for everyone
unless has_flag?("skip_abstract_def_check")
@progress_tracker.stage("Semantic (abstract def check)") do
AbstractDefChecker.new(self).run
end
@progress_tracker.stage("Semantic (abstract def check)") do
AbstractDefChecker.new(self).run
end

{node, processor}
Expand Down
14 changes: 7 additions & 7 deletions src/compiler/crystal/semantic/abstract_def_checker.cr
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,7 @@ class Crystal::AbstractDefChecker

original_base_return_type = base_type.lookup_type?(base_return_type_node)
unless original_base_return_type
report_warning(base_return_type_node, "can't resolve return type #{base_return_type_node}\n#{this_warning_will_become_an_error}")
report_error(base_return_type_node, "can't resolve return type #{base_return_type_node}")
return
end

Expand All @@ -279,24 +279,24 @@ class Crystal::AbstractDefChecker

base_return_type = base_type.lookup_type?(base_return_type_node)
unless base_return_type
report_warning(base_return_type_node, "can't resolve return type #{base_return_type_node}\n#{this_warning_will_become_an_error}")
report_error(base_return_type_node, "can't resolve return type #{base_return_type_node}")
return
end

return_type_node = method.return_type
unless return_type_node
report_warning(method, "this method overrides #{Call.def_full_name(base_type, base_method)} which has an explicit return type of #{original_base_return_type}.\n#{@program.colorize("Please add an explicit return type (#{base_return_type} or a subtype of it) to this method as well.").yellow.bold}\n\n#{this_warning_will_become_an_error}")
report_error(method, "this method overrides #{Call.def_full_name(base_type, base_method)} which has an explicit return type of #{original_base_return_type}.\n#{@program.colorize("Please add an explicit return type (#{base_return_type} or a subtype of it) to this method as well.").yellow.bold}\n")
return
end

return_type = type.lookup_type?(return_type_node)
unless return_type
report_warning(return_type_node, "can't resolve return type #{return_type_node}\n#{this_warning_will_become_an_error}")
report_error(return_type_node, "can't resolve return type #{return_type_node}")
return
end

unless return_type.implements?(base_return_type)
report_warning(return_type_node, "this method must return #{base_return_type}, which is the return type of the overridden method #{Call.def_full_name(base_type, base_method)}, or a subtype of it, not #{return_type}\n#{this_warning_will_become_an_error}")
report_error(return_type_node, "this method must return #{base_return_type}, which is the return type of the overridden method #{Call.def_full_name(base_type, base_method)}, or a subtype of it, not #{return_type}")
return
end
end
Expand All @@ -321,8 +321,8 @@ class Crystal::AbstractDefChecker
@program.colorize("The above warning will become an error in a future Crystal version.").yellow.bold
end

private def report_warning(node, message)
@program.report_warning(node, message)
private def report_error(node, message)
node.raise(message, nil)
end

class ReplacePathWithTypeVar < Visitor
Expand Down

0 comments on commit 0fa9550

Please sign in to comment.