-
Notifications
You must be signed in to change notification settings - Fork 27
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
Forbid usage of type aliased shapes #181
Conversation
bf4110d
to
d7e999f
Compare
# end | ||
# end | ||
class ForbidTypeAliasedShapes < RuboCop::Cop::Base | ||
MSG = "Type aliases cannot contain shapes" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's worth expanding the message here to explain the performance implications. It might be confusing to users given that Sorbet does allow you to add type aliases with shapes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I changed the wording too.
d7e999f
to
440c43d
Compare
440c43d
to
224d487
Compare
RUBY | ||
end | ||
|
||
it("disallows defining type aliases that contain shapes") do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add an example with a nested shape:
Foo = T.type_alias { [{ foo: Integer }] }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I started checking for an array as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this approach will work to catch a shape deeply nested inside the type alias. Consider this: T.nilable({ foo: Integer })
.
I think we need to descend in the type alias block recursively.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can use a node pattern descendant search to generically find hash
children (i.e. shapes, in this context).
It would be good to add a couple tests for different nesting:
A = T.type_alias { [{ foo: Integer }] }
B = T.type_alias { T.nilable({ foo: Integer }) }
C = T.type_alias { T::Hash[Symbol, { foo: Integer }] }
D = T.type_alias { T::Hash[Symbol, T::Array[T.any(String, { foo: Integer })]] }
RUBY | ||
end | ||
|
||
it("disallows defining type aliases that contain shapes") do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can use a node pattern descendant search to generically find hash
children (i.e. shapes, in this context).
It would be good to add a couple tests for different nesting:
A = T.type_alias { [{ foo: Integer }] }
B = T.type_alias { T.nilable({ foo: Integer }) }
C = T.type_alias { T::Hash[Symbol, { foo: Integer }] }
D = T.type_alias { T::Hash[Symbol, T::Array[T.any(String, { foo: Integer })]] }
# @!method type_alias?(node) | ||
def_node_matcher(:type_alias?, <<-PATTERN) | ||
(block | ||
(send | ||
(const nil? :T) :type_alias) | ||
(args) | ||
(hash ...) | ||
) | ||
PATTERN | ||
|
||
# @!method nested_type_alias?(node) | ||
def_node_matcher(:nested_type_alias?, <<-PATTERN) | ||
(block | ||
(send | ||
(const nil? :T) :type_alias) | ||
(args) | ||
(array | ||
(hash ...) | ||
) | ||
) | ||
PATTERN | ||
|
||
def on_block(node) | ||
add_offense(node) if type_alias?(node) || nested_type_alias?(node) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following up on https://github.com/Shopify/rubocop-sorbet/pull/181/files#r1356962786, I think checking for a hash
descendant would handle nesting.
We could also improve the naming: we're not matching a type_alias?
, we're matching a shape_type_alias?
.
# @!method type_alias?(node) | |
def_node_matcher(:type_alias?, <<-PATTERN) | |
(block | |
(send | |
(const nil? :T) :type_alias) | |
(args) | |
(hash ...) | |
) | |
PATTERN | |
# @!method nested_type_alias?(node) | |
def_node_matcher(:nested_type_alias?, <<-PATTERN) | |
(block | |
(send | |
(const nil? :T) :type_alias) | |
(args) | |
(array | |
(hash ...) | |
) | |
) | |
PATTERN | |
def on_block(node) | |
add_offense(node) if type_alias?(node) || nested_type_alias?(node) | |
end | |
# @!method shape_type_alias?(node) | |
def_node_matcher(:shape_type_alias?, <<-PATTERN) | |
(block | |
(send (const {nil? cbase} :T) :type_alias) | |
(args) | |
`hash | |
) | |
PATTERN | |
def on_block(node) | |
add_offense(node) if shape_type_alias?(node) | |
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great thank you.
Presumably we'd want to catch the use of shapes in other places (e.g. |
I'm open to having them all in one cop but maybe community prefers not banning shapes completely. I kept this one specific because this cop is direct result of a big performance overhead we observed and we can get it merged quicker. But I'd be happy to have a cop or cops that looks at sigs, |
I think it makes sense to have it be a single cop, and make it configurable to allow some behaviours, if there's a use case for it. |
class ForbidTypeAliasedShapes < RuboCop::Cop::Base | ||
MSG = "Type aliases shouldn't contain shapes because of significant performance overhead" | ||
|
||
# @!method type_alias?(node) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this actually used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😬 Oops
) | ||
PATTERN | ||
|
||
# @!method nested_type_alias?(node) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question here?
52fc3f2
to
7b99145
Compare
7b99145
to
5bb7bfe
Compare
Adds a cop to forbid usage of shapes inside type aliases.
Shapes are an unfinished experimental feature and developers might want to reduce its usages.
We also noticed significant performance overhead while using this approach in our monolith compared to a bare class.