diff options
author | Lin Jen-Shin <godfat@godfat.org> | 2018-12-26 12:58:08 +0300 |
---|---|---|
committer | Lin Jen-Shin <godfat@godfat.org> | 2018-12-26 12:58:08 +0300 |
commit | 145079b3540ca832e1d981bbc685cc8c27d47ea0 (patch) | |
tree | 099cda679151facf2af4bd55cd64f5ca3f102736 | |
parent | d0884ab6cd53e2b53339e1fe00a1919eba4a2807 (diff) | |
parent | d2851f41ba38a8292cd063aeb374f64541765bb9 (diff) |
Merge branch '42125-extend-override-check-to-also-check-arity' into 'master'
Resolve "Extend `override` check to also check arity"
Closes #42125
See merge request gitlab-org/gitlab-ce!23498
-rw-r--r-- | app/models/dashboard_group_milestone.rb | 1 | ||||
-rw-r--r-- | changelogs/unreleased/42125-extend-override-check-to-also-check-arity.yml | 5 | ||||
-rw-r--r-- | lib/gitlab/utils/override.rb | 109 | ||||
-rw-r--r-- | spec/lib/gitlab/utils/override_spec.rb | 32 |
4 files changed, 112 insertions, 35 deletions
diff --git a/app/models/dashboard_group_milestone.rb b/app/models/dashboard_group_milestone.rb index 32e8104125c..ad0bb55f0a7 100644 --- a/app/models/dashboard_group_milestone.rb +++ b/app/models/dashboard_group_milestone.rb @@ -5,7 +5,6 @@ class DashboardGroupMilestone < GlobalMilestone attr_reader :group_name - override :initialize def initialize(milestone) super(milestone.title, Array(milestone)) diff --git a/changelogs/unreleased/42125-extend-override-check-to-also-check-arity.yml b/changelogs/unreleased/42125-extend-override-check-to-also-check-arity.yml new file mode 100644 index 00000000000..9892466ca50 --- /dev/null +++ b/changelogs/unreleased/42125-extend-override-check-to-also-check-arity.yml @@ -0,0 +1,5 @@ +--- +title: Extend override check to also check arity +merge_request: 23498 +author: Jacopo Beschi @jacopo-beschi +type: added diff --git a/lib/gitlab/utils/override.rb b/lib/gitlab/utils/override.rb index c412961ea3f..c87e97d0213 100644 --- a/lib/gitlab/utils/override.rb +++ b/lib/gitlab/utils/override.rb @@ -4,16 +4,11 @@ module Gitlab module Utils module Override class Extension - def self.verify_class!(klass, method_name) - instance_method_defined?(klass, method_name) || - raise( - NotImplementedError.new( - "#{klass}\##{method_name} doesn't exist!")) - end - - def self.instance_method_defined?(klass, name, include_super: true) - klass.instance_methods(include_super).include?(name) || - klass.private_instance_methods(include_super).include?(name) + def self.verify_class!(klass, method_name, arity) + extension = new(klass) + parents = extension.parents_for(klass) + extension.verify_method!( + klass: klass, parents: parents, method_name: method_name, sub_method_arity: arity) end attr_reader :subject @@ -22,35 +17,77 @@ module Gitlab @subject = subject end - def add_method_name(method_name) - method_names << method_name - end - - def add_class(klass) - classes << klass + def parents_for(klass) + index = klass.ancestors.index(subject) + klass.ancestors.drop(index + 1) end def verify! classes.each do |klass| - index = klass.ancestors.index(subject) - parents = klass.ancestors.drop(index + 1) - - method_names.each do |method_name| - parents.any? do |parent| - self.class.instance_method_defined?( - parent, method_name, include_super: false) - end || - raise( - NotImplementedError.new( - "#{klass}\##{method_name} doesn't exist!")) + parents = parents_for(klass) + + method_names.each_pair do |method_name, arity| + verify_method!( + klass: klass, + parents: parents, + method_name: method_name, + sub_method_arity: arity) end end end + def verify_method!(klass:, parents:, method_name:, sub_method_arity:) + overridden_parent = parents.find do |parent| + instance_method_defined?(parent, method_name) + end + + raise NotImplementedError.new("#{klass}\##{method_name} doesn't exist!") unless overridden_parent + + super_method_arity = find_direct_method(overridden_parent, method_name).arity + + unless arity_compatible?(sub_method_arity, super_method_arity) + raise NotImplementedError.new("#{subject}\##{method_name} has arity of #{sub_method_arity}, but #{overridden_parent}\##{method_name} has arity of #{super_method_arity}") + end + end + + def add_method_name(method_name, arity = nil) + method_names[method_name] = arity + end + + def add_class(klass) + classes << klass + end + + def verify_override?(method_name) + method_names.has_key?(method_name) + end + private + def instance_method_defined?(klass, name) + klass.instance_methods(false).include?(name) || + klass.private_instance_methods(false).include?(name) + end + + def find_direct_method(klass, name) + method = klass.instance_method(name) + method = method.super_method until method && klass == method.owner + method + end + + def arity_compatible?(sub_method_arity, super_method_arity) + if sub_method_arity >= 0 && super_method_arity >= 0 + # Regular arguments + sub_method_arity == super_method_arity + else + # It's too complex to check this case, just allow sub-method having negative arity + # But we don't allow sub_method_arity > 0 yet super_method_arity < 0 + sub_method_arity < 0 + end + end + def method_names - @method_names ||= [] + @method_names ||= {} end def classes @@ -80,11 +117,21 @@ module Gitlab def override(method_name) return unless ENV['STATIC_VERIFICATION'] + Override.extensions[self] ||= Extension.new(self) + Override.extensions[self].add_method_name(method_name) + end + + def method_added(method_name) + super + + return unless ENV['STATIC_VERIFICATION'] + return unless Override.extensions[self]&.verify_override?(method_name) + + method_arity = instance_method(method_name).arity if is_a?(Class) - Extension.verify_class!(self, method_name) + Extension.verify_class!(self, method_name, method_arity) else # We delay the check for modules - Override.extensions[self] ||= Extension.new(self) - Override.extensions[self].add_method_name(method_name) + Override.extensions[self].add_method_name(method_name, method_arity) end end diff --git a/spec/lib/gitlab/utils/override_spec.rb b/spec/lib/gitlab/utils/override_spec.rb index fc08ebcfc6d..9e7c97f8095 100644 --- a/spec/lib/gitlab/utils/override_spec.rb +++ b/spec/lib/gitlab/utils/override_spec.rb @@ -25,11 +25,21 @@ describe Gitlab::Utils::Override do let(:klass) { subject } - def good(mod) + def good(mod, bad_arity: false, negative_arity: false) mod.module_eval do override :good - def good - super.succ + + if bad_arity + def good(num) + end + elsif negative_arity + def good(*args) + super.succ + end + else + def good + super.succ + end end end @@ -56,6 +66,14 @@ describe Gitlab::Utils::Override do described_class.verify! end + it 'checks ok for overriding method using negative arity' do + good(subject, negative_arity: true) + result = instance.good + + expect(result).to eq(1) + described_class.verify! + end + it 'raises NotImplementedError when it is not overriding anything' do expect do bad(subject) @@ -63,6 +81,14 @@ describe Gitlab::Utils::Override do described_class.verify! end.to raise_error(NotImplementedError) end + + it 'raises NotImplementedError when overriding a method with different arity' do + expect do + good(subject, bad_arity: true) + instance.good(1) + described_class.verify! + end.to raise_error(NotImplementedError) + end end shared_examples 'checking as intended, nothing was overridden' do |