Project

General

Profile

Actions

Feature #18007

open

Help developers of C extensions meet requirements in "doc/extension.rdoc"

Added by mdalessio (Mike Dalessio) about 1 month ago. Updated 19 days ago.

Status:
Open
Priority:
Normal
Assignee:
-
Target version:
-
[ruby-core:104401]

Description

A pull request for this feature has been submitted at https://github.com/ruby/ruby/pull/4604

Problem being solved

This option is intended to help developers of C extensions to check if their code meets the requirements explained in "doc/extension.rdoc". Specifically, I want to ensure that T_DATA object classes undefine or redefine the allocate method.

There is currently no easy way for an author of a C extension to easily see where they have made the mistake of letting the default allocate class method remain.

Description of the solution

Compiled with this option, Ruby will warn when a T_DATA object is created whose class has not undefined or redefined the allocate method.

A new function is defined, rb_data_object_check. That function is called from rb_data_object_wrap() and
rb_data_typed_object_wrap() (which implement the Data_Wrap_Struct family of macros).

The warning, when emitted, looks like this:

warning: T_DATA class Nokogiri::XML::Document should undefine or redefine the allocate method, please see doc/extension.rdoc

Examples of this problem in the wild

Using this option, I found that many of Nokogiri's classes needed to undefine allocate.

This PR also updates these core Ruby classes by undefining allocate:

  • ObjectSpace::InternalObjectWrapper
  • Socket::Ifaddr

Questions for reviewers

Does this check really need to be behind a configuration option? Performance impact is very small (see benchmarks below), but I put it behind a flag because I am worried that there may be a many C extensions that would emit warnings at runtime, and the users of those extensions cannot fix the problem and so would mostly just be annoyed.

Should this warning be emitted with the deprecated category?

Benchmarking

I benchmarked this code by allocating Nokogiri::XML::NodeSets in a loop. This is a class with a relatively simple allocate function.

The runs cover the four combinations of enabled/disabled, and warnings/no-warnings.

ruby 3.1.0dev (2021-06-25T04:02:18Z flavorjones-extens.. de943189aa) [x86_64-linux]
Warming up --------------------------------------
disabled, warn=false   490.143k i/100ms
Calculating -------------------------------------
disabled, warn=false      4.863M (± 1.5%) i/s -     49.014M in  10.081177s

ruby 3.1.0dev (2021-06-25T04:02:18Z flavorjones-extens.. de943189aa) [x86_64-linux]
Warming up --------------------------------------
 disabled, warn=true   483.070k i/100ms
Calculating -------------------------------------
 disabled, warn=true      4.839M (± 1.4%) i/s -     48.790M in  10.083899s

Comparison:
disabled, warn=false:  4863064.0 i/s
 disabled, warn=true:  4839310.1 i/s - same-ish: difference falls within error


ruby 3.1.0dev (2021-06-25T04:02:18Z flavorjones-extens.. de943189aa) [x86_64-linux]
Warming up --------------------------------------
 enabled, warn=false   484.398k i/100ms
Calculating -------------------------------------
 enabled, warn=false      4.840M (± 1.9%) i/s -     48.440M in  10.011854s

Comparison:
disabled, warn=false:  4863064.0 i/s
 enabled, warn=false:  4840123.2 i/s - same-ish: difference falls within error
 disabled, warn=true:  4839310.1 i/s - same-ish: difference falls within error


ruby 3.1.0dev (2021-06-25T04:02:18Z flavorjones-extens.. de943189aa) [x86_64-linux]
Warming up --------------------------------------
  enabled, warn=true   492.200k i/100ms
Calculating -------------------------------------
  enabled, warn=true      4.866M (± 2.1%) i/s -     48.728M in  10.017455s

Comparison:
  enabled, warn=true:  4866434.8 i/s
disabled, warn=false:  4863064.0 i/s - same-ish: difference falls within error
 enabled, warn=false:  4840123.2 i/s - same-ish: difference falls within error
 disabled, warn=true:  4839310.1 i/s - same-ish: difference falls within error

My conclusion is that the performance impact is very small, and we could omit the option if the Ruby core maintainers decide this behavior should be on by default.

Updated by nobu (Nobuyoshi Nakada) about 1 month ago

I don't think the configuration option is needed.
And Check_Type(klass, T_CLASS); can be in rb_data_object_check.

Updated by mdalessio (Mike Dalessio) about 1 month ago

nobu (Nobuyoshi Nakada) wrote in #note-1:

I don't think the configuration option is needed.
And Check_Type(klass, T_CLASS); can be in rb_data_object_check.

Thank you for your review. I've updated the pull request.

Updated by naruse (Yui NARUSE) 19 days ago

I want Ruby 3.1 to be compatible with 3.0. Therefore even if it is accepted, you can merge this in 3.2.

Updated by mdalessio (Mike Dalessio) 19 days ago

Naruse, thank you for your time.

Would you consider for 3.1 if this change was behind a configuration option that defaulted to disabled?

Actions

Also available in: Atom PDF