From d8b713dc31a4587a38a4a821130962b21aa244b2 Mon Sep 17 00:00:00 2001 From: Troy Sornson Date: Sun, 12 Jan 2025 14:38:48 -0700 Subject: [PATCH] Don't add restrictions to parent overridden methods, add warning message when it happens --- spec/compiler/apply_types_spec.cr | 19 ++++++++++ src/compiler/crystal/command/typer.cr | 30 ++++++++-------- src/compiler/crystal/tools/typer.cr | 50 ++++++++++++++------------- 3 files changed, 59 insertions(+), 40 deletions(-) diff --git a/spec/compiler/apply_types_spec.cr b/spec/compiler/apply_types_spec.cr index 8a9d63d71bcd..1af8844f9893 100644 --- a/spec/compiler/apply_types_spec.cr +++ b/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 diff --git a/src/compiler/crystal/command/typer.cr b/src/compiler/crystal/command/typer.cr index 0decce58c931..fb122b91e04c 100644 --- a/src/compiler/crystal/command/typer.cr +++ b/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,14 +51,14 @@ 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 @@ -59,14 +66,6 @@ class Crystal::Command 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 diff --git a/src/compiler/crystal/tools/typer.cr b/src/compiler/crystal/tools/typer.cr index 861c85f185af..b2fa6dc37ea3 100644 --- a/src/compiler/crystal/tools/typer.cr +++ b/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