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

Type restriction of String#rindex #10969

Closed
straight-shoota opened this issue Jul 20, 2021 · 4 comments · Fixed by #10972
Closed

Type restriction of String#rindex #10969

straight-shoota opened this issue Jul 20, 2021 · 4 comments · Fixed by #10972
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. kind:regression Something that used to correctly work but no longer works topic:stdlib:text
Milestone

Comments

@straight-shoota
Copy link
Member

I have fixed this issue (#10956), then another issue crops up, text.rindex want to return a Int32 but the number given to it was explicitly Int64. you can see the patch https://github.com/iv-org/invidious/pull/2263/files#diff-c7fb42237f52a2684edf3d22d5f0b36f7f0ac8940f71c53886fe4a294ef73357L135

I wonder why isn't these issues showing up before. did something changed in the type inferencer? or it's simply a bug got fixed.

Originally posted by @tleydxdy in #10965 (comment)

@straight-shoota straight-shoota added kind:bug A bug in the code. Does not apply to documentation, specs, etc. kind:regression Something that used to correctly work but no longer works topic:stdlib:text labels Jul 20, 2021
@straight-shoota
Copy link
Member Author

The return type restriction to String#rindex was added in #10583 and it appears that this change was not correct. The return type actually depends on the type of the offset parameter and that's unrestricted. So if you passed in Int64 as an argument, the return type would be Int64 as well.

We should revert this return type restriction. It's a breaking change and a regression in 1.1.0.

@straight-shoota
Copy link
Member Author

This is a very common pattern, by the way: offset or similar parameters without a type restriction or with a wide restriction to Int that influence the return type of the method (or input type of a block parameter). Usually, only Int32 makes sense as a type because that's the type of collection sizes and indices. There's no real benefit from allowing a wide variety of types.

I think we should deprecate such unrestricted or broadly restricted parameters and move towards explicit Int32 restrictions.

@straight-shoota straight-shoota added this to the 1.1.1 milestone Jul 20, 2021
@asterite
Copy link
Member

I think so too.

Ideally, we would have an Int type that's used everywhere, so these issues would be much less common. Unfortunately we didn't go that route and now users have to juggle between many int types... 😢

I wonder if there would still be a way to change that for 1.x...

@straight-shoota
Copy link
Member Author

/cc #8111

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. kind:regression Something that used to correctly work but no longer works topic:stdlib:text
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants