From: "shugo (Shugo Maeda)" Date: 2022-10-20T09:25:34+00:00 Subject: [ruby-core:110438] [Ruby master Feature#19063] Hash.new with non-value objects should be less confusing Issue #19063 has been updated by shugo (Shugo Maeda). austin (Austin Ziegler) wrote in #note-4: > This mistake is much more common than I would have thought, and it appears in some fairly large projects (not everyone may have access to this result because cs.github.com is still apparently in preview): It seems that only AWS S3 SDK has invalid code except when their clients mutate Hash values. > - FactoryBot: https://github.com/thoughtbot/factory_bot/blob/ec71611954d07419c9b668be03641332443dcd78/lib/factory_bot/linter.rb#L22 valid code: `result[factory] |= errors` > - Fat Free CRM > - 1: https://github.com/fatfreecrm/fat_free_crm/blob/5b73c62335835106c25f547d9810197d8b4c0ba8/lib/fat_free_crm/callback.rb#L58 valid code: `response[op[:position]] += [op[:proc].call(caller, context)]` > - 2: https://github.com/fatfreecrm/fat_free_crm/blob/5b73c62335835106c25f547d9810197d8b4c0ba8/lib/fat_free_crm/callback.rb#L74 valid code: `@view_hooks[hook] += [{ proc: proc,` > - Instructure Canvas LMS: https://github.com/instructure/canvas-lms/blob/8bd46cb657ec925229bfcd5407c145ec121d85c7/app/models/context.rb#L182 valid code: `ids_by_type[type] += [id]` > - Chef > - Knife: https://github.com/chef/chef/blob/e85fcb8c0ad1ea4ed6f830e4b585f27c2ada4994/knife/lib/chef/knife.rb#L173 valid code: `ids_by_type[type] += [id]` > - Inspec: https://github.com/inspec/inspec/blob/af5b8335af052743914909e3833a88c7ba350ed8/lib/inspec/utils/nginx_parser.rb#L93 valid code: `agg_conf[x.key] += [x.vals]` > - Redmine: > - 1: https://github.com/redmine/redmine/blob/823080b45e58563f989b992789ed340d358ed955/app/models/user.rb#660 valid code `result[role] = Project.where(:id => ids).to_a` > - 2: https://github.com/redmine/redmine/blob/823080b45e58563f989b992789ed340d358ed955/app/models/user.rb#693 valid code: `result[role] = Project.where(:id => ids).to_a` > - Browser CMS: https://github.com/browsermedia/browsercms/blob/0a7fb9219f6e24cce4271b420c2ea07febd1b748/app/models/cms/category_type.rb#L21 valid code: `map[ct.id.to_s] = ct.category_list.map { |c| [c.path, c.id.to_s] ` > - AWS S3 SDK (feature testing, but���) > - 1: https://github.com/aws/aws-sdk-ruby/blob/2e96092cca28a0422fc3950f078b0919b636c227/gems/aws-sdk-s3/features/client/step_definitions.rb#344 invalid code: `@tracker[type.to_sym] << event` > - 2: https://github.com/aws/aws-sdk-ruby/blob/2e96092cca28a0422fc3950f078b0919b636c227/gems/aws-sdk-s3/features/client/step_definitions.rb#366 invalid code: `@tracker[type.to_sym] << event` > - Barkeep: https://github.com/ooyala/barkeep/blob/126b1b1f41f514396d3e22f9935fe4640de2a402/lib/string_filter.rb#L27 valid code: `filter_methods.merge!({ method_name => filter_methods[method_name] + [filter_block] })` > - Opal: https://github.com/opal/opal/blob/934aa61bdb80a9da6ce9c92b0eedba737bbc1f9f/lib/opal/compiler.rb#L255 valid code: `@comments = ::Parser::Source::Comment.associate_locations(first_node, comments)` > - OpenProject: > - 1: https://github.com/opf/openproject/blob/56738cbfe492dcde3ab80463bff33eed7ae765f4/lib/api/v3/activities/activity_eager_loading_wrapper.rb#L76 valid code: `hash[class_name] = class_name` > - 2: (`Hash.new({})`) https://github.com/opf/openproject/blob/56738cbfe492dcde3ab80463bff33eed7ae765f4/lib/api/v3/activities/activity_eager_loading_wrapper.rb#L92 valid code: `hash[class_name] = class_name` > - Goldiloader: https://github.com/salsify/goldiloader/blob/ead058ab65ecf845c78b51307c048a9f09f87ef7/lib/goldiloader/auto_include_context.rb#L26 valid code: `register_models(nested_models, nested_associations[association])` > - rspec-core: https://github.com/rspec/rspec-core/blob/71823ba11ec17a73b25bdc24ebab195494c270dc/lib/rspec/core/filter_manager.rb#L205-L206 valid code: ``` no_location_filters = locations[ File.expand_path(ex_metadata[:rerun_file_path]) ].empty? ``` ---------------------------------------- Feature #19063: Hash.new with non-value objects should be less confusing https://bugs.ruby-lang.org/issues/19063#change-99752 * Author: baweaver (Brandon Weaver) * Status: Open * Priority: Normal ---------------------------------------- Related to #10713 and #2764. Ruby's `Hash.new` accepts either a block or a param for its default value. In the case of non-value objects this leads to unexpected behaviors: ```ruby bad_hash_with_array_values = Hash.new([]) good_hash_with_array_values = Hash.new { |h, k| h[k] = [] } ``` While, as @hsbt has said in the past, this is behaving as intended for the Ruby language it has caused a lot of confusion in the community over the years and is a known sharp-edge. My assertion is that this is not the intended behavior, and I cannot find a legitimate usecase in which someone intends for this to happen. More often new users to Ruby are confused by this behavior and spend a lot of time debugging. We must consider the impact to Ruby users, despite what the intent of the language is, and make the language more clear where possible. Given that, I have a few potential proposals for Ruby committers. ### Proposal 1: Do What They Meant When people use `Hash.new([])` they mean `Hash.new { |h, k| h[k] = [] }`. Can we make that the case that if you pass a mutable or non-value object that the behavior will be as intended using `dup` or other techniques? When used in the above incorrect way it is likely if not always broken code. ### Proposal 2: Warn About Unexpected Behavior As mentioned above, I do not believe there are legitimate usages of `Hash.new([])`, and it is a known bug to many users as they do not intend for that behavior. It may be worthwhile to warn people if they do use it. ### Proposal 3: Require Frozen or Values This is more breaking than the above, but it may make sense to require any value passed to `Hash.new` to either be `frozen` or a value object (e.g. `1` or `true`) ## Updating RuboCop Failing this, I am considering advocating for RuboCop and similar linters to warn people against this behavior as it is not intended in most to all cases: https://github.com/rubocop/rubocop/issues/11013 ...but as @ioquatix has mentioned on the issue it would make more sense to fix Ruby rather than put a patch on top of it. I would be inclined to agree with his assessment, and would rather fix this at a language level as it is a known point of confusion. ## Final Thoughts I would ask that maintainers consider the confusion that this has caused in the community, rather than asserting this "works as intended." It does work as intended, but the intended functionality can make Ruby more difficult for beginners. We should keep this in mind. -- https://bugs.ruby-lang.org/ Unsubscribe: