Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ActiveSupport::OrderedOptions#respond_to?(:call) true - breaking check for callable objects #51469

Open
kapcod opened this issue Apr 2, 2024 · 2 comments

Comments

@kapcod
Copy link

kapcod commented Apr 2, 2024

ActiveSupport::OrderedOptions returns true for .respond_to?(:call), which breaks various duck-type approach to detect callable objects. Example is pretty popular carrierwave-aws gem which checks if one of the options is callable by checking respond_to?(:call).
There is no reason to return true for any possible key. OpenStruct also returns 'nilfor non-existing keys, butrespond_to?only returnstruefor existing keys. On the other hand someone did it on purpose, otherwise why implementrespond_to_missing?as hard-codedtrue`, so I'm not sure if there was no higher design here.

Steps to reproduce

ActiveSupport::OrderedOptions.new.respond_to?(:call) # => true
ActiveSupport::InheritableOptions.new.respond_to?(:call) # => true
# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "rails"
  # If you want to test against edge Rails replace the previous line with this:
  # gem "rails", github: "rails/rails", branch: "main"
end

require "active_support"
require "active_support/core_ext/object/blank"
require "minitest/autorun"

class BugTest < Minitest::Test
  def test_ordered_options_respond_to
    assert ActiveSupport::OrderedOptions.new.update(aaa: 1).respond_to?(:aaa)
    refute ActiveSupport::OrderedOptions.new.respond_to?(:call)
  end
end

Expected behavior

ActiveSupport::OrderedOptions.new.respond_to?(:call) # => should return false

Actual behavior

ActiveSupport::OrderedOptions.new.respond_to?(:call) # => returns true

System configuration

Rails version: 7.0.8.1
Ruby version: 3.0.4

@fatkodima
Copy link
Member

This is how it works. We can assign any value to it via options.foo= or get any value from it via options.foo (and get nil back if there is no any). There are tests for this class testing exactly this.

So, I do not think that this can or should be changed.

@kapcod
Copy link
Author

kapcod commented Apr 7, 2024

@fatkodima I see you yourself mentioned the issue of respond_to returning true to everything in this discussion.
respond_to used in many places as standard duck-type approach. I think that even though OrderedOptions returns nil on any non-existing field and any field= method works, it doesn't respond_to? to follow it. Yes, it causes inconsistency, but it helps preserve the duck-typing pattern.
I don't think there can be any code that is dependent on respond_to? being true for OrderedOptions because given it's hard-coded to true, there's no meaning to this, so I see no damage changing it to be consistent with OpenStruct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants