Don't add restrictions to parent overridden methods, add warning mess…

…age when it happens

spec/compiler/apply_types_spec.cr

@@ -462,6 +462,25 @@ describe Crystal::SourceTyper do
     INPUT
   end
 
+  it "doesn't type ancestor methods that are inherited" do
+    run_source_typer_spec(<<-INPUT, nil, line_number: -1)
+    class Foo
+      def test(arg)
+        arg
+      end
+    end
+
+    class Bar < Foo
+      def test(arg)
+        1
+      end
+    end
+
+    Bar.new.test(3)
+    Foo.new.test(2)
+    INPUT
+  end
+
   it "runs prelude and types everything" do
     run_source_typer_spec(<<-INPUT, <<-OUTPUT, line_number: -1, prelude: "prelude")
     # This file tries to capture each type of definition format

src/compiler/crystal/command/typer.cr

@@ -1,7 +1,6 @@
-# Implementation of the `crystal tool format` command
+# Implementation of the `crystal tool apply-types` command
 #
-# This is just the command-line part. The formatter
-# logic is in `crystal/tools/formatter.cr`.
+# This provides the CLI interface for `crystal/tools/typer.cr`
 
 class Crystal::Command
   private def apply_types
@@ -36,6 +35,14 @@ class Crystal::Command
         exit
       end
 
+      opts.on("--exclude [DIRECTORY]", "Exclude a directory from being typed") do |ex|
+        excludes << ex
+      end
+
+      opts.on("--error-trace", "Show full error trace") do
+        error_trace = true
+      end
+
       opts.on("--prelude [PRELUDE]", "Use given file as prelude. Use empty string to skip prelude entirely.") do |new_prelude|
         prelude = new_prelude
       end
@@ -44,29 +51,21 @@ class Crystal::Command
         type_blocks = true
       end
 
-      opts.on("--include-splats", "Enable adding types to splats") do
-        type_splats = true
-      end
-
       opts.on("--include-double-splats", "Enable adding types to double splats") do
         type_double_splats = true
       end
 
+      opts.on("--include-splats", "Enable adding types to splats") do
+        type_splats = true
+      end
+
       opts.on("--stats", "Enable statistics output") do
         stats = true
       end
 
       opts.on("--progress", "Enable progress output") do
         progress = true
       end
-
-      opts.on("--error-trace", "Show full error trace") do
-        error_trace = true
-      end
-
-      opts.on("--exclude [DIRECTORY]", "Exclude a directory from being typed") do |ex|
-        excludes << ex
-      end
     end
 
     parser.parse(options)
@@ -95,7 +94,6 @@ class Crystal::Command
       puts "No type restrictions added"
     else
       results.each do |filename, file_contents|
-        # pp! filename, file_contents
         File.write(filename, file_contents)
       end
     end

src/compiler/crystal/tools/typer.cr

@@ -110,14 +110,6 @@ module Crystal
       formatter.added_types? ? formatter.finish : nil
     end
 
-    # If a def is already fully typed, we don't need to check / write it
-    private def fully_typed?(d : Def) : Bool
-      ret = true
-      ret &= d.args.all?(&.restriction)
-      ret &= !!d.return_type
-      ret
-    end
-
     @_signatures : Hash(String, Signature)?
 
     # Signatures represents a mapping of location => Signature for def at that location
@@ -148,11 +140,12 @@ module Crystal
 
       program.types.each { |_, t| types << t }
 
-      overridden_method_locations = [] of String
+      overridden_method_locations = {} of String => String
       while type = types.shift?
         type.types?.try &.each { |_, t| types << t }
-        # pp! type, def_overrides_parent_def(type)
-        def_overrides_parent_def(type).each { |loc| overridden_method_locations << loc }
+        def_overrides_parent_def(type).each do |child_def_loc, ancestor_def_loc|
+          overridden_method_locations[child_def_loc] = ancestor_def_loc
+        end
 
         # Check for class instance 'def's
         if type.responds_to?(:def_instances)
@@ -175,29 +168,29 @@ module Crystal
       end
 
       # Now remove all overridden methods
-      overridden_method_locations.each do |loc|
-        ret.delete(loc)
+      overridden_method_locations.each do |child_loc, ancestor_loc|
+        if ret.delete(child_loc)
+          @warnings << "Not adding type restrictions to definition at #{child_loc} as it overrides definition #{ancestor_loc}"
+        end
       end
 
       ret
     end
 
-    private def def_overrides_parent_def(type) : Array(String)
-      overriden_locations = [] of String
+    private def def_overrides_parent_def(type) : Hash(String, String)
+      overriden_locations = {} of String => String
       type.defs.try &.each_value do |defs_with_metadata|
         defs_with_metadata.each do |def_with_metadata|
-          next if def_with_metadata.def.location.to_s.starts_with?("expanded macro:")
+          next if def_with_metadata.def.location.to_s.starts_with?("expanded macro:") || def_with_metadata.def.name == "initialize"
           type.ancestors.each do |ancestor|
-            other_defs_with_metadata = ancestor.defs.try &.[def_with_metadata.def.name]?
-            other_defs_with_metadata.try &.each do |other_def_with_metadata|
-              next if other_def_with_metadata.def.location.to_s.starts_with?("expanded macro:")
+            ancestor_defs_with_metadata = ancestor.defs.try &.[def_with_metadata.def.name]?
+            ancestor_defs_with_metadata.try &.each do |ancestor_def_with_metadata|
+              next if ancestor_def_with_metadata.def.location.to_s.starts_with?("expanded macro:")
               found_def_with_same_name = true
 
-              if def_with_metadata.compare_strictness(other_def_with_metadata, self_owner: type, other_owner: ancestor) == 0
-                # puts "Method #{type}##{def_with_metadata.def.name} overrides #{ancestor}##{def_with_metadata.def.name}"
-                # Found a method with the same name and same, stricter or weaker restriction,
-                # so it overrides
-                overriden_locations << def_with_metadata.def.location.to_s
+              if def_with_metadata.compare_strictness(ancestor_def_with_metadata, self_owner: type, other_owner: ancestor) == 0
+                overriden_locations[def_with_metadata.def.location.to_s] = ancestor_def_with_metadata.def.location.to_s
+                overriden_locations[ancestor_def_with_metadata.def.location.to_s] = def_with_metadata.def.location.to_s
               end
             end
           end
@@ -576,6 +569,7 @@ module Crystal
       def visit(node : Crystal::Def)
         return false unless loc = node.location
         return false unless loc.filename && loc.line_number && loc.column_number
+        return false if fully_typed?(node)
         if node_in_def_locators(loc)
           all_defs << node
           files << loc.filename.to_s
@@ -605,6 +599,14 @@ module Crystal
         # Whelp, nothing matched, skip this location
         false
       end
+
+      # If a def is already fully typed, we don't need to check / write it
+      private def fully_typed?(d : Def) : Bool
+        ret = true
+        ret &= d.args.all?(&.restriction)
+        ret &= (d.name == "initialize" || !!d.return_type)
+        ret
+      end
     end
   end
 end