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

Add cop to forbid T::Struct uses and autocorrect them to bare classes #178

Merged
merged 1 commit into from Sep 25, 2023

Conversation

Morriar
Copy link
Contributor

@Morriar Morriar commented Sep 22, 2023

This cop creates offenses for usages of any kind of T::Struct (T::Struct, T::InexactStrict, T::ImmutableStruct) or T::Props (T::Props, T::Props::Constructor, T::Props::WeakConstructor etc.).

For T::Struct, it provides an unsafe autocorrect that translate the struct to a plain old Ruby class so this:

class MyStruct < T::Struct
  const :foo, String
  prop :bar, Integer, default: 0
end

Gets corrected into this:

class MyStruct
  extend T::Sig

  sig { returns(String) }
  attr_reader :foo

  sig { returns(Integer) }
  attr_accessor :bar

  sig { params(foo: String, bar: Integer) }
  def initialize(foo:, bar: 0)
    @foo = foo
    @bar = bar
  end
end

The autocorrect is unsafe as the resulting class does not provide a 1:1 replacement to all the T::Struct behavior and may break existing code expectations.

cc. @rafaelfranca

@Morriar Morriar added the feature Add a new feature label Sep 22, 2023
@Morriar Morriar requested a review from a team as a code owner September 22, 2023 13:31
@Morriar Morriar self-assigned this Sep 22, 2023
Copy link
Contributor

@KaanOzkan KaanOzkan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏

spec/rubocop/cop/sorbet/forbid_t_struct_spec.rb Outdated Show resolved Hide resolved
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
@Morriar Morriar merged commit d245285 into main Sep 25, 2023
10 checks passed
@Morriar Morriar deleted the at-tstruct-autocorrect branch September 25, 2023 14:36
@shopify-shipit shopify-shipit bot temporarily deployed to production September 25, 2023 14:39 Inactive
@KaanOzkan
Copy link
Contributor

⚠️ This cop is disabled by default and it's not recommended for use by other projects. Disabling T::Struct was something our team thought was best for Shopify's large monolith after investigating the usages and the impact on performance. Here is the internal justification for this decision to give more context:

T::Struct and its associated implementations provide substantial runtime type safety to our code, but this comes at the expense of performance when compared to a standard Ruby class.

Typically, we mitigate any runtime overhead of other Sorbet features by disabling it in production. However, with T::Struct, people have been relying on these runtime behaviors in their code. Disabling the overhead would consequently alter the behavior in production, potentially leading to unexpected results.

Given these circumstances, we believe it's more prudent to deprecate T::Struct usage and rely solely on static checks for type safety. We encourage developers to convert their T::Struct classes into plain old Ruby classes and use signatures on their getter and setter methods to maintain type safety.

By deprecating T::Struct, we want developers to move away from these runtime behaviors and instead use the right tool for the right reason. For instance, Active Record or SmartProperties should be used for data validation, while Active Model is suitable for serialization, among others.

Despite discouraging the continued use of T::Struct, we still strongly encourage you to use signatures to leverage the benefits of type checking.

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

Successfully merging this pull request may close these issues.

None yet

2 participants