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

added keyword_init? to classes generated by Struct.new, Ruby 3.1 support #2909

Closed
wants to merge 1 commit into from

Conversation

moste00
Copy link
Contributor

@moste00 moste00 commented Mar 6, 2023

As required by #2733, every class returned by Struct.new now has a keyword_init? method that returns nil if the optional argument to Struct.new of the same name was omitted, and returns true or false if that argument was provided and was truthy or falsey, respectively.

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Mar 6, 2023
@@ -33,7 +33,16 @@ class << self
alias_method :subclass_new, :new
end

def self.new(klass_name, *attrs, keyword_init: false, &block)
NOT_PROVIDED = BasicObject.new
Copy link
Member

@andrykonchin andrykonchin Mar 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's common to use already defined magic singleton object undefined that is used as a default value of an optional argument.

For performance reason its value should be checked with a primitive Primitive.undefined?(obj).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Using BasicObject was indeed ugly, can you expand on how to use "undefined" ? I can't do it, a literal mention of "undefined" is just a NameError.

Anyway, I noticed that CRuby considers passing "keyword_init: nil" to Struct.new as if keyword_init was omitted (i.e. keyword_init? returns nil in this case, as if keyword_init was never passed), so I had the much simpler idea to just initialize keyword_init to nil by default : every boolean test on keyword_init is unaffected because nil is falsey, but we can still distinguish the ommited case by Primitive.nil?. Much more elegant than what I had before.

Copy link
Member

@andrykonchin andrykonchin Mar 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, we don't need to distinguish cases when keyword_init isn't passed and when passed nil.

Regarding undefined - it's a magic object, that is available by default only in Ruby Core library source files (and it shouldn't be available in irb, for instance). Actually it's handled magically on the parser level so it isn't declared explicitly in Ruby source code. It's widely used in the Core library:

def self.foreach(name, separator=undefined, limit=undefined, **options, &block)
return to_enum(:foreach, name, separator, limit, **options) unless block
name = Truffle::Type.coerce_to_path name
separator = $/ if Primitive.undefined?(separator)

def self.keyword_init?
nil
end
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems simpler and shorted to me to calculate value in the keyword_init? method:

def self.keyword_init?
   return nil if Primitive.undefined?(KEYWORD_INIT) || Primitive.nil?(KEYWORD_INIT)
   KEYWORD_INIT != false
end

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍 .

minor: self::KEYWORD_INIT looks a bit shorter than self.const_get(:KEYWORD_INIT) but should work too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, self::KEYWORD_INIT is better, const_get is a few more indirections and less readable too.

@andrykonchin
Copy link
Member

Probably I would add one missing spec for keyword_init: nil case.

Primitive.as_boolean(kw_init)
end

end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra empty line?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should indented two spaces less

Copy link
Member

@andrykonchin andrykonchin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

There are minor formatting issues only.

@eregon
Copy link
Member

eregon commented Mar 14, 2023

@moste00 There is a feature freeze this Friday for truffleruby, so we need to get this PR and #2907 and #2908 merged before Friday. We will do the remaining changes (mostly style and git fixes) to make sure it's merged in time.

@eregon
Copy link
Member

eregon commented Mar 14, 2023

Merged in 8d6a531

@eregon eregon closed this Mar 14, 2023
@moste00
Copy link
Contributor Author

moste00 commented Mar 16, 2023

Thank you so much for taking care of this @eregon, I greatly apologize for leaving it hanging the past week, things were a bit busy for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants