shopify / rubocop-sorbet Goto Github PK
View Code? Open in Web Editor NEWA collection of RuboCop rules for Sorbet
License: MIT License
A collection of RuboCop rules for Sorbet
License: MIT License
Hi, was experimenting with the new ForbidTStruct cop and noticed the autocorrect does not correctly generate the initializer for nilable properties without an explicitly defined default/factory value. The initializer should have them as optional arguments
Example
class Foo < T::Struct
const :name, T.nilable(String)
end
Foo.new.name # <--- nil
Expected after autocorrect
class Foo
extend T::Sig
sig { returns(T.nilable(String)) }
attr_reader :name
sig { params(name: T.nilable(String)).void }
def initialize(name: nil)
@name = name
end
end
Foo.new.name # <--- nil
Current output of autocorrect
class Foo
extend T::Sig
sig { returns(T.nilable(String)) }
attr_reader :name
sig { params(name: T.nilable(String)).void }
def initialize(name:)
@name = name
end
end
Foo.new.name # <--- ArgumentError missing keyword: :name
The result is that name
becomes a required param
If you forget to include the parameter name in a signature, Sorbet/ParametersOrderingInSignature
fails rather than reporting an error.
# typed: true
class Test
extend T::Sig
sig { params(Integer).void }
def test(abc); end
end
flexport$ rubocop -a test.rb
Inspecting 1 file
An error occurred while Sorbet/ParametersOrderingInSignature cop was inspecting /Users/rklingler/flexport/test.rb:7:2.
To see the complete backtrace run rubocop -d.
.
1 file inspected, no offenses detected
1 error occurred:
An error occurred while Sorbet/ParametersOrderingInSignature cop was inspecting /Users/rklingler/flexport/test.rb:7:2.
I would expect the cop to not crash. Since the problem isn't exactly about parameter ordering, it may not be this cop's responsibility to report the error, but it shouldn't crash.
Rubocop: 0.80.1
Rubocop-sorbet: 0.3.7
It appears that the EnforceSignatures
cop has trouble accepting sig(:final)
as a valid signature. This might also affect other signature-based cops.
Here's a failing code example.
# typed: true
class Boundary
sig(:final) { returns(Elem) }
def call
trace { filter }
end
end
Here's the cop configuration:
Sorbet/EnforceSignatures:
Enabled: true
AutoCorrect: false
Severity: warning
Exclude:
- 'config/**/*'
...
The method doesn't trigger the cop's warning, since it has a valid signature.
Got these deprecation warnings in my logs:
/Users/ryanseys/.gem/ruby/3.3.4/gems/rubocop-sorbet-0.8.3/lib/rubocop/cop/sorbet/sigils/valid_sigil.rb:21: warning: `Cop.registry` is deprecated. Use `Registry.global` instead.
/Users/ryanseys/.gem/ruby/3.3.4/gems/rubocop-sorbet-0.8.3/lib/rubocop/cop/sorbet/sigils/has_sigil.rb:18: warning: `Cop.registry` is deprecated. Use `Registry.global` instead.
It would be great to have a cop that works like Naming/MethodName, but for T::Struct props
Personally I only plan to use snake_case for this right now, but for completeness the cop could support the same options as Naming/MethodName.
Naming/MethodName source: https://github.com/rubocop/rubocop/blob/master/lib/rubocop/cop/naming/method_name.rb
Example:
class Foo < T::Struct
const :fooConst, String # bad
const :foo_const, String # good
prop :fooProp, T::Boolean # bad
prop :foo_prop, T::Boolean # good
end
The Style/MutableConstant
cop ensures that literal constants (or all constants in strict
style) are frozen.
However, in a typed: strict
file, constants must have T.let
annotations. Sorbet does not recognize T.let
calls that have.freeze
appended to them. This largely invalidates the MutableConstant
cop (since literal constants are wrapped by T.let
calls, and the latter cannot be frozen).
I would like a sorbet-compatible version of the Style/MutableConstant
cop. It should require literal first arguments to T.let
to be frozen with EnforcedStyle
set to literals
(the default), or all first arguments when set to strict
.
Would the maintainers be amenable to such a cop?
This is not strictly an issue with rubocop-sorbet, but I've been unable to find a general solution to this problem, and it keeps us from being able to use rubocop-sorbet.
The issue being that once **/*.rbi
is added to the Include
list for rubocop, it causes all of the other configured cops to run on rbi files, making a lot of complaints (and changes). And of course rbi generation going forward will continue to create them as-is, so in my mind it's not desirable to enforce the same rubocop rules within rbi files.
My preference would be to only include rbi files for the Sorbet cops, and exclude them for all other cops, but there's no elegant way to do this.
Can anyone provide some guidance on this? I can open a PR to add to the README if we can come up with a solution.
Original context: Shopify/ruby-lsp-rails#131 (comment)
When a sig has several parameters, it's usually more readable as a multi-line block, with one parameter per line. But when it only has one or two parameters, a single-line block is usually preferred.
Let's add a cop to auto-correct this.
To repro, create a.rb
with:
# typed: strict
class A
sig { void }
private def one
1
end
end
Expected result: no Sorbet/EnforceSignatures
violation
Actual result:
$ rubocop --only Sorbet/EnforceSignatures a.rb
Inspecting 1 file
C
Offenses:
a.rb:5:11: C: [Correctable] Sorbet/EnforceSignatures: Each method is required to have a signature.
private def one ...
^^^^^^^
1 file inspected, 1 offense detected, 1 offense auto-correctable
The autocorrect here is also a bit busted:
$ rubocop --only Sorbet/EnforceSignatures -a a.rb
Inspecting 1 file
C
Offenses:
a.rb:5:11: C: [Corrected] Sorbet/EnforceSignatures: Each method is required to have a signature.
private def one ...
^^^^^^^
a.rb:6:11: C: [Corrected] Sorbet/EnforceSignatures: Each method is required to have a signature.
def one ...
^^^^^^^
1 file inspected, 2 offenses detected, 2 offenses corrected
$ cat a.rb
# typed: strict
class A
sig { void }
private sig { returns(T.untyped) }
sig { returns(T.untyped) }
def one
1
end
end
When using an argument or using the WithoutRuntime sig, the rubocop will warn that it's lacking a signature and the autocorrect will insert a new one.
# frozen_string_literal: true
# typed: strict
require 'sorbet'
class Mine
extend T::Sig
sig(:final) { void }
def test_final
end
T::Sig::WithoutRuntime.sig { void }
def test_without_runtime
end
end
Rubocop output:
Offenses:
test_sig.rb:11:3: C: [Correctable] Sorbet/EnforceSignatures: Each method is required to have a signature.
def test_final ...
^^^^^^^^^^^^^^
test_sig.rb:15:3: C: [Correctable] Sorbet/EnforceSignatures: Each method is required to have a signature.
def test_without_runtime ...
^^^^^^^^^^^^^^^^^^^^^^^^
1 file inspected, 2 offenses detected, 2 offenses auto-correctable
Corrected:
# frozen_string_literal: true
# typed: strict
require 'sorbet'
class Mine
extend T::Sig
sig(:final) { void }
sig { returns(T.untyped) }
def test_final
end
T::Sig::WithoutRuntime.sig { void }
sig { returns(T.untyped) }
def test_without_runtime
end
end
My expectation is that both these forms should pass the lint rule.
Ruby supports a few methods that allow implicit type conversions.
Since Sorbet is a nominal (not structural) type system, implicit conversion is currently unsupported.
I would like to write a cop that adds violations when implicit conversion methods are defined. Would the maintainers be amenable to this?
It seems that our cops are not catching the usage of
# # typed: true
and end up adding another sigil above that one - even though Sorbet does seem to understand the double commented sigil. We should make our cops more robust to this and also remove the extra leading comments since only one is necessary.
When autocorrecting the Sorbet/ValidSigil cop on files without any magic string comments at the top, it adds the sigil without an extra newline like so:
# typed: false
class Banana
...
This generally trips the Layout/EmptyLineAfterMagicComment cop and a subsequent auto correct needs to be run. This is okay locally, but can be annoying in CI when we're using a tool like Pronto to make suggestions; we need to run the job again to pick up this difference.
It would be great to introduce an option to this cop to insert an extra blank line when it adds the magic string up top. I can imagine some teams would not like that behavior, but an option seems reasonable.
Would love some thoughts on this, or if I'm missing an easy way around this issue. I'll work on a PR soon to implement the option.
I've attempted to introduce rubocop-sorbet
in a couple projects, and consistently encounter infinite loops with autocorrection. A fairly minimal repro is:
bundle gem foo
cd foo
# make gemspec valid:
sed -ie 's/TODO: //g' foo.gemspec
sed -i '' '/uri/d' foo.gemspec
sed -i '' '/homepage/d' foo.gemspec
bundle add rubocop rubocop-sorbet
cat << EOF >> .rubocop.yml
---
require:
- rubocop-sorbet
EOF
bundle exec rubocop -a
the last fails, with the relevant part of the output looking like this:
Infinite loop detected in /Users/dug/git/foo/Gemfile.
/Users/dug/.rbenv/versions/2.7.0/lib/ruby/gems/2.7.0/gems/rubocop-0.80.1/lib/rubocop/runner.rb:273:in `block in iterate_until_no_changes'
/Users/dug/.rbenv/versions/2.7.0/lib/ruby/gems/2.7.0/gems/rubocop-0.80.1/lib/rubocop/runner.rb:269:in `loop'
/Users/dug/.rbenv/versions/2.7.0/lib/ruby/gems/2.7.0/gems/rubocop-0.80.1/lib/rubocop/runner.rb:269:in `iterate_until_no_changes'
/Users/dug/.rbenv/versions/2.7.0/lib/ruby/gems/2.7.0/gems/rubocop-0.80.1/lib/rubocop/runner.rb:240:in `do_inspection_loop'
/Users/dug/.rbenv/versions/2.7.0/lib/ruby/gems/2.7.0/gems/rubocop-0.80.1/lib/rubocop/runner.rb:119:in `block in file_offenses'
Gemfile
ends up with 100 # frozen_string_literal: true
lines, and also each typed sigil, e.g.:
# frozen_string_literal: true
# typed: strong
# typed: strict
# typed: true
# typed: false
# typed: ignore
# typed: false
# frozen_string_literal: true
# frozen_string_literal: true
# frozen_string_literal: true
# (97 more of the previous line)
I typically configure rubocop.yml
differently, e.g. disabling strong & strict sigils, but still see infinite loops.
Also, this isn't specific to Gemfile
– even if that file is excluded, it repros on subsequent files.
This is something I've thought about a few times, and I was reminded of it by #130:
If RuboCop was able to use the Sorbet LSP to know about types, it could provide more accurate linting and safer auto-correction.
This would likely be a big change, and perhaps doesn't even belong in rubocop-sorbet
itself, but I'd love to experiment to see what could be done.
Those superclasses already have a rewriter in Sorbet that handles them explicitly, so we shouldn't be "autocorrecting" those cases in Sorbet/ForbidSuperclassConstLiteral
cop
When parsing a signature resembling sig { override(allow_incompatible: true).void }
the inspection errors as follows:
NoMethodError:
undefined method `each_node' for nil:NilClass
# ./lib/rubocop/cop/sorbet/signatures/allow_incompatible_override.rb:39:in `sig?'
# ./lib/rubocop/cop/sorbet/signatures/allow_incompatible_override.rb:32:in `allow_incompatible_override?'
# ./lib/rubocop/cop/sorbet/signatures/allow_incompatible_override.rb:41:in `on_send'
Test case added on this branch reproduces the error.
rspec ./spec/rubocop/cop/sorbet/signatures/allow_incompatible_override_spec.rb:17
Seems potentially related to a recent refactor?
As per the guidance by @jez
who works on Sorbet at Stripe, stubbing is_a?
is problematic:
https://stackoverflow.com/a/56743840
This most often occurs for mocks, but it's probably a good general guideline.
Unless the project is a gem, I believe sorbet-static-and-runtime
should always be preferred?
Hi rubocop-sorbet team, its "me" the unparser author. Can I list your use case in the unparser README?
https://github.com/mbj/unparser
Cheers,
Markus
Bad example:
sig { void }
def method_name
end
Good example:
sig { void }
def method_name
end
Right now, applying a suggestion about signatures build order will change the formatting of Sorbet's generics to use the .
notation.
For example,
sig { void.abstract.params(x: T::Array[String]) }
Is auto-corrected into:
sig { abstract.params(x: T::Array.[](String)).void }
instead of:
sig { abstract.params(x: T::Array[String]).void }
I believe this occurs because we use unparser
to transform the s expression back into a Ruby code String.
Given the code:
class Foo
ConstantType = type_member { { fixed: T.class_of(::ActiveRecord::Base) } }
end
it is obvious that there is no constant binding there thus no need for a type_alias
call. However, the BindingConstantWithoutTypeAlias
cop treats this as a violation and tries to autocorrect this to:
class Foo
ConstantType = T.type_alias { type_member { { fixed: T.class_of(::ActiveRecord::Base) } } }
end
which is wrong.
This is causing problems with the new type_member
syntax that is expecting blocks like above.
There's a bunch of overloads of #slice
/#[]
, many of which are technically nilable, but don't return nil
in practice.
We can't fix that in the general case, but of a lot uses with ranges start from 0
(or ending in -1
can be replaced by more dedicated methods like first
/drop
/last
, which aren't nilable.
A challenge here is that the APIs aren't consistent between Array
and String
(and other types), and there's no way to only make the change for particular types.
These examples assume:
a = Array(0..9)
s = Array("a".."z").join
first
Applicable for Array
, but requires a patch from ActiveSupport
for String
.
-T.must(s[0..5])
-T.must(s[ ..5])
+s.first(6)
-T.must(s[0...5])
-T.must(s[ ...5])
-T.must(s[0, 5])
+s.first(5)
last
Applicable for Array
, but not to String
.
-T.must(a[5..-1])
-T.must(a[5.. ])
-T.must(a[5... ])
+a.drop(5)
dup
Really, these likely just be s
, but we can't be sure that the callers weren't relying on the result being a new copy of the string, so we'd conservatively need to dup
it.
-T.must(s[0..])
-T.must(s[0...])
-T.must(s[..-1])
s.dup
TModel = type_member(fixed: T.untyped)
^ that code gets this error:
Sorbet/BindingConstantWithoutTypeAlias: It looks like you're trying to bind a type to a constant. To do this, you must alias the type using T.type_alias.
TModel = type_member(fixed: T.untyped)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
And autocorrects to this:
TModel = T.type_alias { type_member(fixed: T.untyped) }
Which is incorrect:
app/graphql/types/platform/basic_view.rb:4: Type variable TModel needs to be declared as `= type_member(SOMETHING)` https://srb.help/5018
4 | TModel = T.type_alias { type_member(fixed: T.untyped) }
^^^^^^
app/graphql/types/platform/basic_view.rb:4: Malformed type declaration. Unknown type syntax. Expected a ClassName or T.<func> https://srb.help/5004
4 | TModel = T.type_alias { type_member(fixed: T.untyped) }
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
According to Semantic Versioning
- Major version zero (0.y.z) is for initial development. Anything MAY change at any time. The public API SHOULD NOT be considered stable.
- Version 1.0.0 defines the public API. The way in which the version number is incremented after this release is dependent on this public API and how it changes.
We appear to be past initial development, so we should publish 1.0 and provide consumers with a better stability contract.
There are a couple changes that would make sense to include in this release:
undefined method `source' for nil:NilClass
/Users/robynelliott/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/rubocop-sorbet-0.6.2/lib/rubocop/cop/sorbet/callback_conditionals_binding.rb:128:in `on_send'
/Users/robynelliott/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/rubocop-1.21.0/lib/rubocop/cop/commissioner.rb:100:in `public_send'
/Users/robynelliott/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/rubocop-1.21.0/lib/rubocop/cop/commissioner.rb:100:in `block (2 levels) in trigger_responding_cops'
/Users/robynelliott/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/rubocop-1.21.0/lib/rubocop/cop/commissioner.rb:160:in `with_cop_error_handling'
/Users/robynelliott/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/rubocop-1.21.0/lib/rubocop/cop/commissioner.rb:99:in `block in trigger_responding_cops'
/Users/robynelliott/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/rubocop-1.21.0/lib/rubocop/cop/commissioner.rb:98:in `each'
/Users/robynelliott/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/rubocop-1.21.0/lib/rubocop/cop/commissioner.rb:98:in `trigger_responding_cops'
/Users/robynelliott/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/rubocop-1.21.0/lib/rubocop/cop/commissioner.rb:69:in `on_send'
/Users/robynelliott/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/rubocop-ast-1.11.0/lib/rubocop/ast/traversal.rb:137:in `block in on_dstr'
/Users/robynelliott/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/rubocop-ast-1.11.0/lib/rubocop/ast/traversal.rb:137:in `each'
/Users/robynelliott/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/rubocop-ast-1.11.0/lib/rubocop/ast/traversal.rb:137:in `on_dstr'
/Users/robynelliott/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/rubocop-1.21.0/lib/rubocop/cop/commissioner.rb:71:in `on_begin'
/Users/robynelliott/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/rubocop-ast-1.11.0/lib/rubocop/ast/traversal.rb:154:in `on_class'
/Users/robynelliott/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/rubocop-1.21.0/lib/rubocop/cop/commissioner.rb:71:in `on_class'
/Users/robynelliott/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/rubocop-ast-1.11.0/lib/rubocop/ast/traversal.rb:154:in `on_class'
/Users/robynelliott/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/rubocop-1.21.0/lib/rubocop/cop/commissioner.rb:71:in `on_class'
/Users/robynelliott/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/rubocop-ast-1.11.0/lib/rubocop/ast/traversal.rb:20:in `walk'
/Users/robynelliott/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/rubocop-1.21.0/lib/rubocop/cop/commissioner.rb:86:in `investigate'
/Users/robynelliott/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/rubocop-1.21.0/lib/rubocop/cop/team.rb:155:in `investigate_partial'
/Users/robynelliott/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/rubocop-1.21.0/lib/rubocop/cop/team.rb:83:in `investigate'
/Users/robynelliott/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/rubocop-1.21.0/lib/rubocop/runner.rb:309:in `inspect_file'
/Users/robynelliott/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/rubocop-1.21.0/lib/rubocop/runner.rb:253:in `block in do_inspection_loop'
/Users/robynelliott/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/rubocop-1.21.0/lib/rubocop/runner.rb:287:in `block in iterate_until_no_changes'
/Users/robynelliott/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/rubocop-1.21.0/lib/rubocop/runner.rb:280:in `loop'
/Users/robynelliott/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/rubocop-1.21.0/lib/rubocop/runner.rb:280:in `iterate_until_no_changes'
/Users/robynelliott/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/rubocop-1.21.0/lib/rubocop/runner.rb:249:in `do_inspection_loop'
/Users/robynelliott/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/rubocop-1.21.0/lib/rubocop/runner.rb:130:in `block in file_offenses'
/Users/robynelliott/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/rubocop-1.21.0/lib/rubocop/runner.rb:155:in `file_offense_cache'
/Users/robynelliott/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/rubocop-1.21.0/lib/rubocop/runner.rb:129:in `file_offenses'
/Users/robynelliott/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/rubocop-1.21.0/lib/rubocop/runner.rb:120:in `process_file'
/Users/robynelliott/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/rubocop-1.21.0/lib/rubocop/runner.rb:101:in `block in each_inspected_file'
/Users/robynelliott/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/rubocop-1.21.0/lib/rubocop/runner.rb:100:in `each'
/Users/robynelliott/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/rubocop-1.21.0/lib/rubocop/runner.rb:100:in `reduce'
/Users/robynelliott/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/rubocop-1.21.0/lib/rubocop/runner.rb:100:in `each_inspected_file'
/Users/robynelliott/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/rubocop-1.21.0/lib/rubocop/runner.rb:86:in `inspect_files'
/Users/robynelliott/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/rubocop-1.21.0/lib/rubocop/runner.rb:47:in `run'
/Users/robynelliott/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/rubocop-1.21.0/lib/rubocop/cli/command/execute_runner.rb:26:in `block in execute_runner'
/Users/robynelliott/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/rubocop-1.21.0/lib/rubocop/cli/command/execute_runner.rb:52:in `with_redirect'
/Users/robynelliott/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/rubocop-1.21.0/lib/rubocop/cli/command/execute_runner.rb:25:in `execute_runner'
/Users/robynelliott/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/rubocop-1.21.0/lib/rubocop/cli/command/execute_runner.rb:17:in `run'
/Users/robynelliott/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/rubocop-1.21.0/lib/rubocop/cli/command.rb:11:in `run'
/Users/robynelliott/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/rubocop-1.21.0/lib/rubocop/cli/environment.rb:18:in `run'
/Users/robynelliott/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/rubocop-1.21.0/lib/rubocop/cli.rb:71:in `run_command'
/Users/robynelliott/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/rubocop-1.21.0/lib/rubocop/cli.rb:78:in `execute_runners'
/Users/robynelliott/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/rubocop-1.21.0/lib/rubocop/cli.rb:47:in `run'
/Users/robynelliott/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/rubocop-1.21.0/exe/rubocop:12:in `block in <top (required)>'
/Users/robynelliott/.rbenv/versions/2.7.3/lib/ruby/2.7.0/benchmark.rb:308:in `realtime'
/Users/robynelliott/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/rubocop-1.21.0/exe/rubocop:12:in `<top (required)>'
/Users/robynelliott/.rbenv/versions/2.7.3/bin/rubocop:23:in `load'
/Users/robynelliott/.rbenv/versions/2.7.3/bin/rubocop:23:in `<main>'
rubocop --require rubocop-sorbet --format offenses
1.21.0 (using Parser 3.0.2.0, rubocop-ast 1.11.0, running on ruby 2.7.3 x86_64-darwin20)
The ValidSigil
cop considers any valid sigil value followed by any non-word non-whitespace character to be valid. Sorbet does not. This conflict causes the ValidSigil
cop to be overly permissive of functionally invalid sigil values.
A sample file began with this sigil:
# typed: false # rubocop:todo Sorbet/StrictSigil
A similar regex bug in spoom
caused spoom to erroneously bump the sigil to:
# typed: true# rubocop:todo Sorbet/StrictSigil
This new true#
sigil is functionally ignored by Sorbet, but does not cause a rubocop error despite being an invalid sigil value.
Prevent things such as #121
As the Sorbet docs themselves admit, T::Struct#serialize
has some surprising behavior, such that it is often better to implement serialization by hand.
To avoid these issues, it would be nice to have a rule that disallows T::Struct#serialize
. Ideally it would include these additional features:
T::Enum#serialize
(which is safer)I'm happy to contribute an implementation for review, but I'd love to hear any feedback about this approach in the meantime!
While T::Enum
provides a low-effort, type-safe way to construct an exhaustive set of values, it is also less performant than implementing the same functionality using data structures from Ruby (e.g. list of constants).
rubocop-sorbet should discourage using T::Enum
in ways that exacerbates performance issues or doesn't use T::Enum
for its intended purpose.
Potential cops include:
T::Enum
with one value into a constantComparable
module in T::Enum
(comparing T::Enum
values has particularly poor performance)T::Enum
values (motivation)EDIT (2024-04-22): I decided not to create a cop forbidding case statements above a certain size.
After running some benchmarks, it seems that, at runtime, case statements have exponentially decreasing performance whether they're being used with T::Enum
values or Ruby constants, and when statically type checking, time spent increases linearly. There isn't an obvious point at which performance drops off in either of these cases, making any cut off we assign totally arbitrary. We can revisit this issue if it comes up in the future.
EDIT (2024-04-23): I've just verified that these results are the same with or without YJIT on (thanks @amomchilov for prompting me to do that).
We disable Performance/RedundantBlockCall
because "[it] conflicts with Sorbet since we need to capture the block to type it."
There is precedent for RuboCop extensions patching cops defined somewhere else, and this seems like a reasonable thing to patch. If the method definition is prefixed with a sig
, this cop should not complain about capturing the block argument, even if yield
is used.
This would be something like Sorbet/EnforceRuntimeChecks
and would ensure static and runtime checks are enabled.
We're still using the old API. We should upgrade to the new one, but that will require changing cops and the documentation generation task.
For consistency, we should have a cop to ensure that
sig { returns String }
is written as
sig { returns(String) }
The first thing we often get when installing rubocop-sorbet is a mass amount of requests to change the sigil ordering in files. That's a big commit, and it might cause people to ask what the significance of the ordering means. But the documentation does not provide any of this.
We use Sorbet/EnforceSignatures
along with Pronto to ensure that newly added methods are having signatures. However, it doesn't make sense to add them on typed: false
files as they won't be validated anyway. Can an option be added to not do so? e.g.
Sorbet/EnforceSignatures:
Enabled: true
EnforcedStyle: ignore_typed_false
Ruby and Sorbet support attr
, for example:
class Foo
# ...
sig { returns(Bar) }
attr :bar
end
The EmptyLineAfterSig
cop only supports attr_*
methods.
I'm getting this warning after rubocop updated to the latest.
/usr/local/bundle/gems/rubocop-sorbet-0.8.3/lib/rubocop/cop/sorbet/sigils/valid_sigil.rb:21: warning: `Cop.registry` is deprecated. Use `Registry.global` instead.
As of sorbet/sorbet#1929, T.type_alias
takes a block and evaluates lazily. See also https://sorbet.org/docs/type-aliases
rubocop-sorbet
seems to not like this. FooTwo
and FooThree
both error with "Sorbet/BindingConstantWithoutTypeAlias: It looks like you're trying to bind a type to a constant. To do this, you must alias the type using T.type_alias."
# typed: strict
class Bar
extend T::Sig
FooOne = T.type_alias(T.any(String, Integer))
FooTwo = T.type_alias { T.any(String, Integer) }
FooThree = T.type_alias do
T.any(String, Integer)
end
end
This impacts 0.6.5 and 0.6.6 AFAIK
rubocop-sorbet/config/default.yml
Line 42 in 9b57082
I think this should be Sorbet/EnforceSingleSigil
.
Owners: @Shopify/sorbet
Service: rubocop-sorbet/production
Add the owning team's Slack channel(s) to the service.yml using the key slack_channels
.
For example:
slack_channels:
- channel1
- channel2
For components, add the same values in the component.yml
file instead.
For mobile apps, add them in the mobile_app.yml
file instead.
Hi folks!
When creating new files in Rails applications and running rubocop -a
to let it auto-correct the files, rubocop autocorrects by adding the sigil correctly, but then it can't autocorrect the missing frozen string literal comment.
I've tried this in two different repos, one on v0.4.0 and one on v0.4.1 and both behave the same way.
Here's some output:
I created the following class app/models/this_test.rb:
class ThisTest
THIS_TEST = true
end
Ran rubocop -a
:
9:02PM $ rubocop -a
Inspecting 50 files
......................C...........................
Offenses:
app/models/this_test.rb:1:1: C: [Corrected] Sorbet/FalseSigil: No Sorbet sigil found in file. Try a typed: false to start (you can also use rubocop -a to automatically add this).
class ThisTest
^^^^^
app/models/this_test.rb:1:1: C: Style/FrozenStringLiteralComment: Missing frozen string literal comment.
# typed: false
^
50 files inspected, 2 offenses detected, 1 offense corrected
Checked the file, which now had a sigil but no # frozen_string_literal: true
:
# typed: false
class ThisTest
THIS_TEST = true
end
Running rubocop
, it outputs the expected:
9:03PM $ rubocop
Inspecting 50 files
......................C...........................
Offenses:
app/models/this_test.rb:1:1: C: Style/FrozenStringLiteralComment: Missing frozen string literal comment.
# typed: false
^
50 files inspected, 1 offense detected
Running rubocop -a
a second time to see if it gets less confused now that the sigil has already been fixed, but it still can't fix it:
9:03PM $ rubocop -a
Inspecting 50 files
......................C...........................
Offenses:
app/models/this_test.rb:1:1: C: Style/FrozenStringLiteralComment: Missing frozen string literal comment.
# typed: false
^
50 files inspected, 1 offense detected
cc @Shopify/sorbet
We were recently affected by a bug where had stale times/dates for struct defaults --- which is not surprising, just easy to overlook. We ended up writing the cop below which could be generally useful.
I'd be happy to work at creating a PR, but I thought it would be great to get general feedback first. My concerns:
prop
or const
declaration is within a T::Struct
definition, so it is likely prone to false positives; it works in our code base fine but I imagine it might need to be a bit more contextual for general usage class StructDefaultTime < Cop
MSG = "Struct property defaults are evaluated only once. Use a factory for time and date defaults"
DATE_OR_TIME_METHODS = %i[since from_now after ago until before yesterday tomorrow now today].to_set.freeze
STRUCT_DECLARATIONS = %i[const prop].to_set.freeze
def on_send(node)
struct_default(node) do |_, default_node|
nested_date_or_time(default_node) do
add_offense(node)
end
end
end
private
def_node_matcher :struct_default, <<~PATTERN
(send nil? $STRUCT_DECLARATIONS
(sym _)
(const nil? _)
(hash
(pair
(sym :default) $(...))))
PATTERN
def_node_matcher :date_or_time, <<~PATTERN
(send _ $DATE_OR_TIME_METHODS)
PATTERN
def nested_date_or_time(node, &callback)
return if node.nil? || node.block_type?
node.each_child_node do |child|
nested_date_or_time(child, &callback)
end
date_or_time(node, &callback)
end
end
The rough test suite looks something like:
class StructDefaultTimeTest < ActiveSupport::TestCase
include RubocopTestHelper
def setup
@cop = RuboCop::Cop::Bourgeois::StructDefaultTime.new
end
test "ok for non date/time props and constants with or without defaults" do
assert_no_offenses("const :foo, Integer, default: 3")
assert_no_offenses("prop :bar, String, default: 'foo'")
assert_no_offenses("const :baz, Time")
end
test "ok for factory date and time props and constants" do
assert_no_offenses("const :baz, Time, factory: -> { Time.zone.now }")
end
test "offense when using current or relative date/time methods" do
assert_offense("const :foo, Date, default: Time.zone.today")
assert_offense("const :foo, Time, default: Time.zone.now")
assert_offense("const :foo, Date, default: 3.days.from_now")
assert_offense("const :foo, Date, default: Time.zone.now.yesterday")
assert_offense("const :foo, Time, default: 10.minutes.ago")
assert_offense("const :foo, Date, default: 5.days.before(Date.today)")
assert_offense("prop :foo, Date, default: Time.zone.today")
assert_offense("prop :foo, Time, default: Time.zone.now")
assert_offense("prop :foo, Date, default: 3.days.from_now")
assert_offense("prop :foo, Date, default: Time.zone.now.yesterday")
assert_offense("prop :foo, Time, default: 10.minutes.ago")
assert_offense("prop :foo, Date, default: 5.days.before(Date.today)")
end
end
The rule for ConstantsFromStrings does not catch null-safe usages. This appears to be due to the use of send
instead of csend
at line 38 of the the rule class.
Making this support catching the null safe variant would help make the rule more effective.
The sorbet docs advise gems to "include their external interfaces RBI files in an rbi/
directory". This leads to ForbidRBIOutsideOfSorbetDir
violations. (The cop would also benefit from providing guidance on structuring sorbet/
with custom rbi
files.)
Changing the autocorrect of:
class Foo
include Bar.module
end
from:
class Foo
BarInclude = Bar.module
include BarInclude
end
to:
class Foo
T.unsafe(self).include Bar.module
end
is better since that allows Sorbet to totally ignore the include
call but the runtime behaviour is exactly the same and we don't end up creating new constant names.
If users still need the methods from that include, they can add a shim RBI file to signal the inclusion of the proper module kind.
I'm trying to enable suggested strictness for sigils, but getting a warning that it isn't supported?
I'm referencing https://github.com/Shopify/rubocop-sorbet/blob/master/manual/cops_sorbet.md#sorbethassigil
rubocop-sorbet 0.6.1
.rubocop.yml
Sorbet/HasSigil:
Enabled: true
SuggestedStrictness: true
Output from rubocop -a:
Warning: Sorbet/HasSigil does not support SuggestedStrictness parameter.
Supported parameters are:
- Enabled
We have some synthetic constants generated by tapioca that shouldn't be referenced in signatures. These won't exist at runtime and even if they are used with T::Sig::WithoutRuntime.sig
runtime checks for that method is disabled which isn't preferred.
A declarative, efficient, and flexible JavaScript library for building user interfaces.
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
An Open Source Machine Learning Framework for Everyone
The Web framework for perfectionists with deadlines.
A PHP framework for web artisans
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
Some thing interesting about web. New door for the world.
A server is a program made to process requests and deliver data to clients.
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
Some thing interesting about visualization, use data art
Some thing interesting about game, make everyone happy.
We are working to build community through open source technology. NB: members must have two-factor auth.
Open source projects and samples from Microsoft.
Google ❤️ Open Source for everyone.
Alibaba Open Source for everyone
Data-Driven Documents codes.
China tencent open source team.