Forbid usage of type aliased shapes

[KaanOzkan]

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.

[vinistock]

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.

[KaanOzkan]

Good point, I changed the wording too.

[Morriar]

Can we add an example with a nested shape:

Foo = T.type_alias { [{ foo: Integer }] }

[KaanOzkan]

👍 I started checking for an array as well

[Morriar]

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.

[sambostock]

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 })]] }

[sambostock]

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 })]] }

[sambostock]

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?.

Suggested change

	# @!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

[KaanOzkan]

This is great thank you.

[sambostock]

Presumably we'd want to catch the use of shapes in other places (e.g. sigs). Should we be creating a cop specific to aliases, or just create a cop that disallows shapes, and simply have the implementation start by detecting type aliases (and then update it to also detect their use in sigs, rather than needing another cop)?

[KaanOzkan]

Presumably we'd want to catch the use of shapes in other places (e.g. sigs). Should we be creating a cop specific to aliases, or just create a cop that disallows shapes, and simply have the implementation start by detecting type aliases (and then update it to also detect their use in sigs, rather than needing another cop)?

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, T.let, T.assert_type!.

[sambostock]

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.

[Morriar]

Is this actually used?

[KaanOzkan]

😬 Oops

[Morriar]

Same question here?