Bug #20943: Constant defined in `Data.define` block

• Status: Open
• Assignee: -
• Target version: -
• Backport: 3.1: UNKNOWN, 3.2: UNKNOWN, 3.3: UNKNOWN

[nobu (Nobuyoshi Nakada)]

From https://github.com/ruby/ruby/pull/12274:

A couple times in code review I've seen constants inadvertently leak to top level from within a Struct or Data do block. I think it would be nice to show reopening the Data class when a constant is defined, so the constant is defined within the namespace. In this case, Measure::NONE instead of top level Object::NONE. It would also show readers that it's okay to reopen a Data class, which seems nice since some folk might not realize. Thanks for considering!

However, I think that NONE probably might be intended to be defined under Measure.

Current:

Measure = Data.define(:amount, :unit) do
  NONE = Data.define
end
p NONE #=> NONE

Another:

Measure = Data.define(:amount, :unit) do
  NONE = Data.define
  p NONE #=> Measure::NONE
end
p NONE # uninitialized constant NONE (NameError)

@zverok (Victor Shepelev) How do think?

[nobu (Nobuyoshi Nakada)]

• Subject changed from Constant defined in `Data` block to Constant defined in `Data.define` block

[byroot (Jean Boussier)]

Yeah, that's a common mistake with Struct.new / Data.define / Class.new

That's why with Struct you'd often see:

class Something < Struct.new(:a, :b)
  ...
end

Which is a bit wasteful as you define two classes instead of one, but not a big deal.

I kinda wish this would be valid syntax:

class Something = Struct.new(:a, :b)
  ...
end

[matheusrich (Matheus Richard)]

FYI: That also happens with Class.new. (maybe all blocks?)

[shan (Shannon Skipper)]

byroot (Jean Boussier) wrote in #note-2:

Which is a bit wasteful as you define two classes instead of one, but not a big deal.

I agree the extra class being created is minor, but I'm slightly more bothered by the anonymous class in the ancestry.

[Something,
 #<Class:0x00000001438d5dd0>,
 Struct,
...
]

I kinda wish this would be valid syntax:

class Something = Struct.new(:a, :b)
  ...
end

I'd like that too. That same pattern would be great for both Struct and Data from my vantage. Or if both could keep constants in scope within do blocks, but would that be consider breaking? Too late for Data? I'd like it.

It seems like options include:

1. Change Data.define do block scope for constants
2. Add a new Something = Data.define or similar syntax
3. Recommend reopening classes in docs
4. Recommend inheritance in docs
5. Keep the status quo of defining a constant outside the scope in docs

That ^ list happens to be roughly in my personal order of preference from top to bottom. :) Having both 1 and 2 would also be an option.

I wonder if changing documentation to something that keeps NONE inside the module is worth doing in the short term? If syntax adjustments are decided against, I'd rather just recommend reopening Data and Struct classes rather than the slight back bending with existing solutions like self:: prefix or < Data.define. On the other hand, I'd love to see a syntax adjustment to make it easier to define a constant within a Data without reopening the class.

[matz (Yukihiro Matsumoto)]

Blocks do not introduce new scope even with instance_eval nor class_eval (along with Data.define etc. with blocks). Changing this behavior might cause serious compatibility problems.
If someone is willing to survey and estimate how much influence it could cause, we'd like to discuss. Otherwise, it's safe to keep the behavior as it is.

Matz.

[zverok (Victor Shepelev)]

TBH, for bigger Struct/Data-based classes I typically prefer a regular inheritance instead of using class definition block—both for just aestetics (“it looks like a class definition”) and to avoid behavioral differences like constant definition discussed here, or being able to treat Data like a proper base class:

For example, .new for Data-based classes accepts both positional and keyword arguments. But if one is sure they want to change this behavior, with regular inheritance, it is extremely easy:

class Unit < Data.define(:value)
  def self.new(value, **nil) = super(value:)
end

Unit.new(1) #=> #<data Unit value=1>
Unit.new(value: 1)
# no keywords accepted (ArgumentError)

Such easy redefinition (using super) is not available with Data.define(...) { ... }.

For all I know, there are two counter-arguments for regular inheritance for those cases:

1. Creation of unnecessary anonymous immediate classes
2. Problems with code reloading

In my dayjob (large regular Rails applications) I find the former negligible, and never met with the latter, but this experience might not be universal.

Yet theoretically, I’d rather thought in the direction of changes that would make regular inheritance from Data.define/Struct.new less problematic (if it really is, and it is not just “how we are used to think of it”).

Just to notice here: Other than Struct/Data, there are other libraries that make use of “inheriting of dynamically produced classes” approach (though I didn’t look into the implementation details for many years, so I am not sure how it is implemented currently), like Sequel:

class Post < Sequel::Model(:my_posts); end

or, IIRC, some of the dry-rb gems.

So, it might be, that “optimizing of inheritance from dynamic classes/modules” is more desirable decision than complication of block scopes.