Skip to content

Accept nil state parameter in to_json type signatures#2691

Merged
ksss merged 1 commit intoruby:masterfrom
vpellan:to-json-accept-nil
Nov 11, 2025
Merged

Accept nil state parameter in to_json type signatures#2691
ksss merged 1 commit intoruby:masterfrom
vpellan:to-json-accept-nil

Conversation

@vpellan
Copy link
Copy Markdown
Contributor

@vpellan vpellan commented Oct 29, 2025

This PR adds nil state parameter to the type signature for all to_json methods. cState_from_state_s will return an unconfigured instance if nil is passed. A lot of implementations of to_json have nil as the default value.

@ksss
Copy link
Copy Markdown
Collaborator

ksss commented Oct 30, 2025

@vpellan Thank you for your request.
Please tell me the specific use cases that will be improved by this change.
This is a breaking change, so we need information to make a decision.

@vpellan
Copy link
Copy Markdown
Contributor Author

vpellan commented Oct 30, 2025

Hey @ksss, thanks for your quick reply ! We're currently trying to improve typing in dd-trace-rb, and one of the first step is to re-enable the disabled rules in our Steepfile. When re-enabling Ruby::IncompatibleAssignment, we stumbled on an error on the parameter of a to_json method that we've implemented.
If we change its signature to def to_json: (?JSON::State? state) -> String, a new Ruby::ArgumentTypeMismatch error will appear at line 140 saying that Hash#to_json does not accept nil. Of course we can skip it by changing the parameter to arg = _ = nil or adding a # steep:ignore comment (and even if this PR gets merged, we'll still have to do it until the next release anyway) but I thought that fixing the root cause could be a great overall improvement.

@ksss
Copy link
Copy Markdown
Collaborator

ksss commented Nov 5, 2025

@vpellan
Thank you for the use case.
We plan to discuss it at our next developer meeting.

@ksss ksss added this to the RBS 4.0 milestone Nov 11, 2025
@ksss
Copy link
Copy Markdown
Collaborator

ksss commented Nov 11, 2025

@vpellan We have decided to accept this PR.
Please wait for RBS v4.
Thanks!

@ksss ksss added this pull request to the merge queue Nov 11, 2025
Merged via the queue into ruby:master with commit c6c4015 Nov 11, 2025
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants