Shopify / rbi-central

MIT License
55 stars 36 forks source link

`ActionController::Parameters#require` signature needs adjustment #231

Open andyw8 opened 6 months ago

andyw8 commented 6 months ago

See https://github.com/Shopify/rbi-central/pull/224#pullrequestreview-1917467194

aprescott commented 1 month ago

Just ran into this same issue, where params.require(:foo) is a string, integer, an array, etc.

I can T.cast(params.require(:foo), ...) although interestingly it doesn't always fire runtime errors, as in cases like a T::Array[X] vs. T::Array[Y].

andyw8 commented 1 month ago

cc @amomchilov

amomchilov commented 1 month ago

@aprescott Do you know what the full possible set of returnable types is?

It should be T.any(String, Integer, T::Array[T.any(String, Integer)) at a minimum, but do you know of any other possible cases?

Whatever that type is, that's what should be used as the return value of not just #require, but also #[], which still returns T.untyped

although interestingly it doesn't always fire runtime errors, as in cases like a T::Array[X] vs. T::Array[Y].

That's a performance concession, Sorbet's runtime type-checking doesn't do deep recursive type-checks on Enumerables. The runtime type-checking calls #valid?, not #recursively_valid?

aprescott commented 1 month ago

Do you know what the full possible set of returnable types is?

I'm not sure! It would be good to match whatever Parameters can possibly support. String or Integer or Array containers of those certainly sounds reasonable. Perhaps Rails source would help confirm. I don't really know much about the rbi-central project's approach here, but perhaps T.anything is an option if the full set of output types isn't clear? The true correct typing would of course beat T.anything.

Sorbet's runtime type-checking doesn't do deep recursive type-checks on Enumerables

Found the docs explaining this right after my comment, realized my confusion!