From 32fb3057027534a290f766e354558c61dfe43279 Mon Sep 17 00:00:00 2001 From: ksss Date: Sun, 11 Oct 2026 22:23:14 +0900 Subject: [PATCH] Add a RuboCop cop forbidding non-type assertions in test/stdlib Tests under test/stdlib exist to catch mismatches between RBS signatures and the runtime behavior. StdlibTest/NonTypeAssertion reports every assert*/refute* call except the TypeAssertions ones that check against RBS (and helpers defined in the same file). Rewrite the remaining assert_raises with assert_send_type_error, and drop the Proc arity checks in Hash_test that do not depend on RBS. Co-Authored-By: Claude Opus 5.5 --- .rubocop.yml | 18 ++++ rbs.gemspec | 2 +- rubocop/stdlib_test_non_type_assertion.rb | 64 ++++++++++++++ .../stdlib_test_non_type_assertion_test.rb | 86 +++++++++++++++++++ test/stdlib/Hash_test.rb | 4 - test/stdlib/uri/file_test.rb | 18 ++-- test/stdlib/uri/generic_test.rb | 24 ++---- test/stdlib/uri/ldap_test.rb | 6 +- 8 files changed, 185 insertions(+), 37 deletions(-) create mode 100644 rubocop/stdlib_test_non_type_assertion.rb create mode 100644 test/rubocop/stdlib_test_non_type_assertion_test.rb diff --git a/.rubocop.yml b/.rubocop.yml index 1eed1a8355..7dd75d8a46 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -2,6 +2,9 @@ plugins: - rubocop-rubycw - rubocop-on-rbs +require: + - ./rubocop/stdlib_test_non_type_assertion.rb + AllCops: DisabledByDefault: true Exclude: @@ -69,3 +72,18 @@ Style/FrozenStringLiteralComment: Enabled: true Include: - 'lib/**/*.rb' + +StdlibTest/NonTypeAssertion: + Enabled: true + Include: + - 'test/stdlib/**/*_test.rb' + Exclude: + # Compares RBS constant declarations with runtime constants. + - 'test/stdlib/constant_drift_test.rb' + AllowedMethods: + - assert_send_type + - assert_send_type_error + - refute_send_type + - assert_const_type + - assert_visibility + - assert_type diff --git a/rbs.gemspec b/rbs.gemspec index f699e7411d..1052bddc39 100644 --- a/rbs.gemspec +++ b/rbs.gemspec @@ -30,7 +30,7 @@ Gem::Specification.new do |spec| spec.files = Dir.chdir(File.expand_path('..', __FILE__)) do `git ls-files -z`.split("\x0").reject do |f| [ - %r{^(test|spec|features|bin|steep|benchmark|templates|rust|\.vscode)/}, + %r{^(test|spec|features|bin|steep|benchmark|templates|rust|rubocop|\.vscode)/}, /Gemfile/, ].any? {|r| f.match(r) } end diff --git a/rubocop/stdlib_test_non_type_assertion.rb b/rubocop/stdlib_test_non_type_assertion.rb new file mode 100644 index 0000000000..cdbdc6b13a --- /dev/null +++ b/rubocop/stdlib_test_non_type_assertion.rb @@ -0,0 +1,64 @@ +# frozen_string_literal: true + +require "rubocop" + +module RuboCop + module Cop + module StdlibTest + # Tests under test/stdlib exist to catch mismatches between RBS signatures and the + # runtime behavior. An assertion that does not consult RBS keeps passing when the + # signature is broken, so it does not belong there. + # + # Every `assert*`/`refute*` call is an offense unless it is listed in `AllowedMethods` + # or defined with `def` in the same file (a local helper, whose body is checked too). + # + # @example + # # bad + # assert_equal 3, 1 + 2 + # assert_raises(ArgumentError) { foo.bar } + # + # # good + # assert_send_type "(Integer) -> Integer", 1, :+, 2 + # assert_send_type_error "() -> bot", ArgumentError, foo, :bar + class NonTypeAssertion < Base + MSG = <<~MSG.tr("\n", " ").strip + `%s` does not check anything against RBS signatures. + Tests under test/stdlib must fail when the RBS definition is wrong: + use `assert_send_type` (a narrow method type also pins down which branch of the return type is taken), + `assert_send_type_error`, or `assert_const_type` instead. + `assert_type` does not read RBS either, so it is not a substitute. + MSG + + ASSERTION_NAME = /\A(assert|refute)/ + + def on_new_investigation + @local_helpers = processed_source.ast&.each_node(:def)&.map(&:method_name)&.to_set || Set.new + end + + def on_send(node) + return if node.receiver + + if node.method?(:method) && node.first_argument&.sym_type? + check(node.first_argument, node.first_argument.value) + else + check(node.loc.selector, node.method_name) + end + end + + private + + def check(location, name) + return unless ASSERTION_NAME.match?(name) + return if allowed_methods.include?(name.to_s) + return if @local_helpers.include?(name) + + add_offense(location, message: format(MSG, method: name)) + end + + def allowed_methods + cop_config.fetch("AllowedMethods", []) + end + end + end + end +end diff --git a/test/rubocop/stdlib_test_non_type_assertion_test.rb b/test/rubocop/stdlib_test_non_type_assertion_test.rb new file mode 100644 index 0000000000..c091223d49 --- /dev/null +++ b/test/rubocop/stdlib_test_non_type_assertion_test.rb @@ -0,0 +1,86 @@ +require "test_helper" + +begin + require "rubocop" +rescue LoadError + # rubocop is a development dependency; skip when running without bundler. + return +end + +require_relative "../../rubocop/stdlib_test_non_type_assertion" + +class StdlibTestNonTypeAssertionTest < Test::Unit::TestCase + ROOT = File.expand_path("../..", __dir__) + + def offenses(source, path: "test/stdlib/Foo_test.rb") + path = File.join(ROOT, path) + config = RuboCop::ConfigStore.new.for_file(path) + cop = RuboCop::Cop::StdlibTest::NonTypeAssertion.new(config) + processed_source = RuboCop::ProcessedSource.new(source.dup,RUBY_VERSION.to_f, path) + processed_source.config = config + processed_source.registry = RuboCop::Cop::Registry.new([cop.class]) + + team = RuboCop::Cop::Team.new([cop], config, raise_error: true) + team.investigate(processed_source).offenses.map {|offense| [offense.line, offense.location.source] } + end + + def test_value_assertions + assert_equal [[2, "assert_equal"], [3, "assert_nil"], [4, "assert_raises"], [5, "assert"], [6, "refute"]], offenses(<<~RUBY) + def test_foo + assert_equal 3, 1 + 2 + assert_nil foo + assert_raises(ArgumentError) { foo(1) } + assert foo + refute foo + end + RUBY + end + + def test_method_reference + assert_equal [[1, ":assert"]], offenses(<<~RUBY) + assert_fn = method(:assert) + assert_type_fn = method(:assert_type) + RUBY + end + + def test_type_assertions + assert_equal [], offenses(<<~RUBY) + def test_foo + assert_send_type "(Integer) -> Integer", 1, :+, 2 + assert_send_type_error "() -> bot", ArgumentError, foo, :bar + refute_send_type "() -> String", foo, :bar + assert_const_type "Integer", "Foo::BAR" + assert_visibility :private, :foo + assert_type "Integer", eval("1") + end + RUBY + end + + def test_local_helper + assert_equal [[3, "assert_equal"]], offenses(<<~RUBY) + def assert_foo(value) + assert_send_type "() -> Integer", value, :foo + assert_equal 1, value.foo + end + + def test_foo + assert_foo bar + end + RUBY + end + + def test_receiver + assert_equal [], offenses(<<~RUBY) + foo.assert_equal 1, 2 + RUBY + end + + def test_outside_stdlib_test + source = "assert_equal 1, 2\n" + + assert_equal [], offenses(source, path: "test/rbs/foo_test.rb") + assert_equal [], offenses(source, path: "test/stdlib/test_helper.rb") + assert_equal [], offenses(source, path: "test/stdlib/constant_drift_test.rb") + assert_equal [[1, "assert_equal"]], offenses(source, path: "test/stdlib/uri/foo_test.rb") + end +end diff --git a/test/stdlib/Hash_test.rb b/test/stdlib/Hash_test.rb index 14e1ed8b71..47c1ed1e87 100644 --- a/test/stdlib/Hash_test.rb +++ b/test/stdlib/Hash_test.rb @@ -149,10 +149,6 @@ def test_to_proc # Make sure return type is correct assert_type 'Rational', {a: 1r}.to_proc.call(:a) assert_type 'nil', {a: 1r}.to_proc.call(:not_a_key) - - # make sure it only takes one argument - assert_raises ArgumentError do {a: 1r}.to_proc.call(:a, :b) end - assert_raises ArgumentError do {a: 1r}.to_proc.call() end end def test_op_eq diff --git a/test/stdlib/uri/file_test.rb b/test/stdlib/uri/file_test.rb index bd01fb6633..943e0a326b 100644 --- a/test/stdlib/uri/file_test.rb +++ b/test/stdlib/uri/file_test.rb @@ -49,24 +49,18 @@ def test_set_port end def test_check_userinfo - assert_raises URI::InvalidURIError do - assert_send_type '(String v) -> nil', - file, :check_userinfo, 'user:pass' - end + assert_send_type_error '(String v) -> nil', URI::InvalidURIError, + file, :check_userinfo, 'user:pass' end def test_check_user - assert_raises URI::InvalidURIError do - assert_send_type '(String v) -> nil', - file, :check_user, 'user' - end + assert_send_type_error '(String v) -> nil', URI::InvalidURIError, + file, :check_user, 'user' end def test_check_password - assert_raises URI::InvalidURIError do - assert_send_type '(String v) -> nil', - file, :check_password, 'pass' - end + assert_send_type_error '(String v) -> nil', URI::InvalidURIError, + file, :check_password, 'pass' end def test_set_userinfo diff --git a/test/stdlib/uri/generic_test.rb b/test/stdlib/uri/generic_test.rb index 280b2836f3..2451e675ce 100644 --- a/test/stdlib/uri/generic_test.rb +++ b/test/stdlib/uri/generic_test.rb @@ -154,10 +154,8 @@ def test_fragment end def test_replace! - assert_raises URI::InvalidURIError do - assert_send_type '(URI::Generic oth) -> URI::Generic', - generic, :replace!, other - end + assert_send_type_error '(URI::Generic oth) -> URI::Generic', URI::InvalidURIError, + generic, :replace!, other end def test_component @@ -335,24 +333,18 @@ def test_port= end def test_check_registry - assert_raises URI::InvalidURIError do - assert_send_type '(String v) -> nil', - generic, :check_registry, 'registry' - end + assert_send_type_error '(String v) -> nil', URI::InvalidURIError, + generic, :check_registry, 'registry' end def test_set_registry - assert_raises URI::InvalidURIError do - assert_send_type '(String v) -> nil', - generic, :set_registry, 'registry' - end + assert_send_type_error '(String v) -> nil', URI::InvalidURIError, + generic, :set_registry, 'registry' end def test_registry= - assert_raises URI::InvalidURIError do - assert_send_type '(String v) -> nil', - generic, :registry=, 'registry' - end + assert_send_type_error '(String v) -> nil', URI::InvalidURIError, + generic, :registry=, 'registry' end def test_check_path diff --git a/test/stdlib/uri/ldap_test.rb b/test/stdlib/uri/ldap_test.rb index 9a4bf25c3d..b732e344e6 100644 --- a/test/stdlib/uri/ldap_test.rb +++ b/test/stdlib/uri/ldap_test.rb @@ -46,10 +46,8 @@ def test_build def test_new # fragment is passed - assert_raises URI::InvalidURIError do - assert_send_type '(String schema, String? userinfo, String host, Integer? port, String? registry, String? path, String? opaque, String query, String? fragment) -> URI::LDAP', - URI::LDAP, :new, 'ldap', nil, 'ldap.example.com', 80, nil, '/dc=example;dc=com?foo,bar,baz?sub?(t=1)?t=2', nil, 'query', 'foo' - end + assert_send_type_error '(String schema, String? userinfo, String host, Integer? port, String? registry, String? path, String? opaque, String query, String? fragment) -> URI::LDAP', URI::InvalidURIError, + URI::LDAP, :new, 'ldap', nil, 'ldap.example.com', 80, nil, '/dc=example;dc=com?foo,bar,baz?sub?(t=1)?t=2', nil, 'query', 'foo' # fragment is missing assert_send_type '(String schema, String? userinfo, String host, Integer? port, String? registry, String? path, String? opaque, String query, String? fragment) -> URI::LDAP',