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

Polish URI::Generic parameter signatures #849

Merged
merged 2 commits into from
Dec 24, 2021
Merged

Conversation

ksss
Copy link
Collaborator

@ksss ksss commented Dec 17, 2021

Many parameters of URI::Generic are nilable.

For example, the relative path scheme is nil.

URI.parse('./').scheme
#=> nil

I loosened the type restrictions for some methods.

@ksss
Copy link
Collaborator Author

ksss commented Dec 17, 2021

CI has failed.
Indeed, the following is true.

URI.parse("").send(:userinfo=, 'user:pass')
#=> ["user", "pass"]

But I think the following result should be written as a type

URI.parse("").userinfo = 'user:pass'
#=> "user:pass"

How should I test this case?

@soutaro
Copy link
Member

soutaro commented Dec 23, 2021

@ksss Wow! 😮 I think the return type of userinfo= method is a tuple, and the value of x.userinfo = y is a behavior of the Ruby language.

@ksss
Copy link
Collaborator Author

ksss commented Dec 24, 2021

@soutaro Thank you for reviewing.

As you said, the Ruby language specification seems to return the argument itself for methods ending with =, regardless of the implementation of the method.

I checked with steep, and this behavior is implemented.

(Maybe here?)
https://github.com/soutaro/steep/blob/89d42e06f5a2c77c55756c97a40e1a6c4b8fb3a4/lib/steep/type_construction.rb#L2739-L2744

However, there seems to be a little bit of confusion since it returns a different type than the documentation.

# Run with the type of `::URI::Generic#userinfo=` before this modification.

# uri: ::URI::Generic
uri = URI::Generic.new(nil, nil, nil, nil, nil, nil, nil, nil, nil)

# ::URI::Generic#userinfo= ~> (::String | nil)
# Sets userinfo, argument is string like 'name:pass'.
# ・(::String? userinfo) -> ::Array[::String | nil]?
userinfo = uri.userinfo = 'user:pass'

# userinfo: (::String | nil)
userinfo

I suggest that the returned type should be the argument type.

And how about the following modification to test this behavior?

diff --git a/test/stdlib/test_helper.rb b/test/stdlib/test_helper.rb
index cfeae24c..aac42bd7 100644
--- a/test/stdlib/test_helper.rb
+++ b/test/stdlib/test_helper.rb
@@ -245,6 +245,10 @@ module TypeAssertions

     begin
       result = spy.wrapped_object.__send__(method, *args, &block)
+      case method.to_s
+      when "[]=", /\w=\Z/
+        result = args.last
+      end
     rescue Exception => exn
       exception = exn
     end

@soutaro
Copy link
Member

soutaro commented Dec 24, 2021

@ksss

However, there seems to be a little bit of confusion since it returns a different type than the documentation.

It makes sense... I think what we should fix is the implementation of uri library.
How about skipping the test of the method for now?

IMO, the implementation of `#userinfo=` is broken that it returns an array instead of the rhs value.
@soutaro soutaro merged commit 7d51529 into ruby:master Dec 24, 2021
@soutaro
Copy link
Member

soutaro commented Dec 24, 2021

@ksss I merged this PR to include the change to RBS 2.0.0, which will be bundled to Ruby 3.1.0! 🙇‍♂️ 🎉

@ksss
Copy link
Collaborator Author

ksss commented Dec 25, 2021

@soutaro Thank you and GJ 🎄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants