-
Notifications
You must be signed in to change notification settings - Fork 123
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
Implement Rubocop DSL RBI compiler #1046
Open
sambostock
wants to merge
3
commits into
main
Choose a base branch
from
rubocop-rbi-compiler
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
# typed: strict | ||
# frozen_string_literal: true | ||
|
||
return unless defined?(RuboCop::AST::NodePattern::Macros) | ||
|
||
module Tapioca | ||
module Dsl | ||
module Compilers | ||
# `Tapioca::Dsl::Compilers::RuboCop` generates types for RuboCop cops. | ||
# RuboCop uses macros to define methods leveraging "AST node patterns". | ||
# For example, in this cop | ||
# | ||
# class MyCop < Base | ||
# def_node_matcher :matches_some_pattern?, "..." | ||
# | ||
# def on_send(node) | ||
# return unless matches_some_pattern?(node) | ||
# # ... | ||
# end | ||
# end | ||
# | ||
# the use of `def_node_matcher` will generate the method | ||
# `matches_some_pattern?`, for which this compiler will generate a `sig`. | ||
# | ||
# More complex uses are also supported, including: | ||
# | ||
# - Usage of `def_node_search` | ||
# - Parameter specification | ||
# - Default parameter specification, including generating sigs for | ||
# `without_defaults_*` methods | ||
class RuboCop < Compiler | ||
ConstantType = type_member do | ||
{ fixed: T.all(Module, Extensions::RuboCop) } | ||
end | ||
|
||
class << self | ||
extend T::Sig | ||
sig { override.returns(T::Array[T.all(Module, Extensions::RuboCop)]) } | ||
def gather_constants | ||
T.cast( | ||
extenders_of(::RuboCop::AST::NodePattern::Macros).select { |constant| name_of(constant) }, | ||
T::Array[T.all(Module, Extensions::RuboCop)], | ||
) | ||
end | ||
end | ||
|
||
sig { override.void } | ||
def decorate | ||
return if node_methods.empty? | ||
|
||
root.create_path(constant) do |cop_klass| | ||
node_methods.each do |name| | ||
create_method_from_def(cop_klass, constant.instance_method(name)) | ||
end | ||
end | ||
end | ||
|
||
private | ||
|
||
sig { returns(T::Array[Extensions::RuboCop::MethodName]) } | ||
def node_methods | ||
constant.__tapioca_node_methods | ||
end | ||
end | ||
end | ||
end | ||
end |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
# typed: strict | ||
# frozen_string_literal: true | ||
|
||
begin | ||
require "rubocop-ast" | ||
rescue LoadError | ||
return | ||
end | ||
|
||
module Tapioca | ||
module Dsl | ||
module Compilers | ||
module Extensions | ||
module RuboCop | ||
extend T::Sig | ||
|
||
MethodName = T.type_alias { T.any(String, Symbol) } | ||
|
||
sig { params(name: MethodName, _args: T.untyped, defaults: T.untyped).returns(MethodName) } | ||
def def_node_matcher(name, *_args, **defaults) | ||
__tapioca_node_methods << name | ||
__tapioca_node_methods << :"without_defaults_#{name}" unless defaults.empty? | ||
|
||
super | ||
end | ||
|
||
sig { params(name: MethodName, _args: T.untyped, defaults: T.untyped).returns(MethodName) } | ||
def def_node_search(name, *_args, **defaults) | ||
__tapioca_node_methods << name | ||
__tapioca_node_methods << :"without_defaults_#{name}" unless defaults.empty? | ||
|
||
super | ||
end | ||
|
||
sig { returns(T::Array[MethodName]) } | ||
def __tapioca_node_methods | ||
@__tapioca_node_methods ||= T.let([], T.nilable(T::Array[MethodName])) | ||
end | ||
|
||
::RuboCop::AST::NodePattern::Macros.prepend(self) | ||
end | ||
end | ||
end | ||
end | ||
end |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
## RuboCop | ||
|
||
`Tapioca::Dsl::Compilers::RuboCop` generates types for RuboCop cops. | ||
RuboCop uses macros to define methods leveraging "AST node patterns". | ||
For example, in this cop | ||
|
||
class MyCop < Base | ||
def_node_matcher :matches_some_pattern?, "..." | ||
|
||
def on_send(node) | ||
return unless matches_some_pattern?(node) | ||
# ... | ||
end | ||
end | ||
|
||
the use of `def_node_matcher` will generate the method | ||
`matches_some_pattern?`, for which this compiler will generate a `sig`. | ||
|
||
More complex uses are also supported, including: | ||
|
||
- Usage of `def_node_search` | ||
- Parameter specification | ||
- Default parameter specification, including generating sigs for | ||
`without_defaults_*` methods |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,174 @@ | ||
# typed: strict | ||
# frozen_string_literal: true | ||
|
||
require "spec_helper" | ||
|
||
module Tapioca | ||
module Dsl | ||
module Compilers | ||
class RuboCopSpec < ::DslSpec | ||
class << self | ||
extend T::Sig | ||
|
||
sig { override.returns(String) } | ||
def target_class_file | ||
# Against convention, RuboCop uses "rubocop" in its file names, so we do too. | ||
super.gsub("rubo_cop", "rubocop") | ||
end | ||
end | ||
|
||
describe "Tapioca::Dsl::Compilers::RuboCop" do | ||
sig { void } | ||
def before_setup | ||
require "rubocop" | ||
require "rubocop-sorbet" | ||
require "tapioca/dsl/extensions/rubocop" | ||
super | ||
end | ||
|
||
describe "initialize" do | ||
it "gathered constants exclude irrelevant classes" do | ||
gathered_constants = gather_constants do | ||
add_ruby_file("content.rb", <<~RUBY) | ||
class Unrelated | ||
end | ||
RUBY | ||
end | ||
assert_empty(gathered_constants) | ||
end | ||
|
||
it "gathers constants extending RuboCop::AST::NodePattern::Macros in gems" do | ||
# Sample of miscellaneous constants that should be found from Rubocop and plugins | ||
missing_constants = [ | ||
"RuboCop::Cop::Bundler::GemVersion", | ||
"RuboCop::Cop::Cop", | ||
"RuboCop::Cop::Gemspec::DependencyVersion", | ||
"RuboCop::Cop::Lint::Void", | ||
"RuboCop::Cop::Metrics::ClassLength", | ||
"RuboCop::Cop::Migration::DepartmentName", | ||
"RuboCop::Cop::Naming::MethodName", | ||
"RuboCop::Cop::Security::CompoundHash", | ||
"RuboCop::Cop::Sorbet::ValidSigil", | ||
"RuboCop::Cop::Style::YodaCondition", | ||
] - gathered_constants | ||
|
||
assert_empty(missing_constants, "expected constants to be gathered") | ||
end | ||
|
||
it "gathers constants extending RuboCop::AST::NodePattern::Macros in the host app" do | ||
gathered_constants = gather_constants do | ||
add_ruby_file("content.rb", <<~RUBY) | ||
class MyCop < ::RuboCop::Cop::Base | ||
end | ||
|
||
class MyLegacyCop < ::RuboCop::Cop::Cop | ||
end | ||
|
||
module MyMacroModule | ||
extend ::RuboCop::AST::NodePattern::Macros | ||
end | ||
|
||
module ::RuboCop | ||
module Cop | ||
module MyApp | ||
class MyNamespacedCop < Base | ||
end | ||
end | ||
end | ||
end | ||
RUBY | ||
end | ||
|
||
assert_equal( | ||
["MyCop", "MyLegacyCop", "MyMacroModule", "RuboCop::Cop::MyApp::MyNamespacedCop"], | ||
gathered_constants, | ||
) | ||
end | ||
end | ||
|
||
describe "decorate" do | ||
it "generates empty RBI when no DSL used" do | ||
add_ruby_file("content.rb", <<~RUBY) | ||
class MyCop < ::RuboCop::Cop::Base | ||
sambostock marked this conversation as resolved.
Show resolved
Hide resolved
|
||
def on_send(node);end | ||
end | ||
RUBY | ||
|
||
expected = <<~RBI | ||
# typed: strong | ||
RBI | ||
|
||
assert_equal(expected, rbi_for(:MyCop)) | ||
end | ||
|
||
it "generates correct RBI file" do | ||
add_ruby_file("content.rb", <<~RUBY) | ||
class MyCop < ::RuboCop::Cop::Base | ||
def_node_matcher :some_matcher, "(...)" | ||
def_node_matcher :some_matcher_with_params, "(%1 %two ...)" | ||
def_node_matcher :some_matcher_with_params_and_defaults, "(%1 %two ...)", two: :default | ||
Comment on lines
+107
to
+109
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that these result in different method signatures, and in the last case, in a second method being generated ( The implementation of |
||
def_node_matcher :some_predicate_matcher?, "(...)" | ||
def_node_search :some_search, "(...)" | ||
def_node_search :some_search_with_params, "(%1 %two ...)" | ||
def_node_search :some_search_with_params_and_defaults, "(%1 %two ...)", two: :default | ||
|
||
def on_send(node);end | ||
end | ||
RUBY | ||
|
||
expected = <<~RBI | ||
# typed: strong | ||
|
||
class MyCop | ||
sig { params(param0: T.untyped).returns(T.untyped) } | ||
def some_matcher(param0 = T.unsafe(nil)); end | ||
|
||
sig { params(param0: T.untyped, param1: T.untyped, two: T.untyped).returns(T.untyped) } | ||
def some_matcher_with_params(param0 = T.unsafe(nil), param1, two:); end | ||
|
||
sig { params(args: T.untyped, values: T.untyped).returns(T.untyped) } | ||
def some_matcher_with_params_and_defaults(*args, **values); end | ||
|
||
sig { params(param0: T.untyped).returns(T.untyped) } | ||
def some_predicate_matcher?(param0 = T.unsafe(nil)); end | ||
|
||
sig { params(param0: T.untyped).returns(T.untyped) } | ||
def some_search(param0); end | ||
|
||
sig { params(param0: T.untyped, param1: T.untyped, two: T.untyped).returns(T.untyped) } | ||
def some_search_with_params(param0, param1, two:); end | ||
|
||
sig { params(args: T.untyped, values: T.untyped).returns(T.untyped) } | ||
def some_search_with_params_and_defaults(*args, **values); end | ||
|
||
sig { params(param0: T.untyped, param1: T.untyped, two: T.untyped).returns(T.untyped) } | ||
def without_defaults_some_matcher_with_params_and_defaults(param0 = T.unsafe(nil), param1, two:); end | ||
|
||
sig { params(param0: T.untyped, param1: T.untyped, two: T.untyped).returns(T.untyped) } | ||
def without_defaults_some_search_with_params_and_defaults(param0, param1, two:); end | ||
end | ||
RBI | ||
|
||
assert_equal(expected, rbi_for(:MyCop)) | ||
end | ||
end | ||
|
||
private | ||
|
||
# Gathers constants introduced in the given block excluding constants that already existed prior to the block. | ||
sig { params(block: T.proc.void).returns(T::Array[String]) } | ||
def gather_constants(&block) | ||
existing_constants = T.let( | ||
Runtime::Reflection | ||
.extenders_of(::RuboCop::AST::NodePattern::Macros) | ||
.filter_map { |constant| Runtime::Reflection.name_of(constant) }, | ||
T::Array[String], | ||
) | ||
yield | ||
gathered_constants - existing_constants | ||
end | ||
end | ||
end | ||
end | ||
end | ||
end |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Originally, I'd renamed the files to
rubo_cop.rb
too, due to the loading not working, but I like this better, as it allows us to keep both the constant names and file names matching RuboCop's convention (which was the whole point of renaming the constants).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.
Do we still need this since we don't rename the file in the DSL command?
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 allows us to have
lib/tapioca/dsl/compilers/rubocop.rb
lib/tapioca/dsl/extensions/rubocop.rb
spec/tapioca/dsl/compilers/rubocop_spec.rb
instead of
lib/tapioca/dsl/compilers/rubo_cop.rb
lib/tapioca/dsl/extensions/rubo_cop.rb
spec/tapioca/dsl/compilers/rubo_cop_spec.rb
which I think is preferable, especially given it's really just limited to telling the spec file how to require the correct compiler.
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 would rather rename the constant
Rubocop
or let the file be namedrubo_cop
rather than play with this.It seems all the files under
dsl/compilers
follow the same convention.class CompilerName
->compiler_name.rb
. I'm not sure why we should deviate from this.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 was trying to match Rubocop's own naming convention (e.g.
RuboCop::Version
is defined inlib/rubocop/version.rb
, notlib/rubo_cop/version.rb
), but given the can of worms it would be to do this for the RBI files, maybe it's not worth doing for the compiler files either. It just feels weird seeingrubo_cop
TBH, but I'm glad to deal with it if you think it's not worth the extra method in the test file.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.
Okay, so circling back to this, a80c4ec makes the changes necessary to avoid this workaround, but my gut feeling is that this leads to the code being more confusing, because now we're dealing with a mix of constants that include
RuboCop
and constants that includeRubocop
, and it's not obvious why they differ.Likewise, we could go for the
rubo_cop
file naming, but then we'd have a mid of those files and ones like.rubocop.yml
, again without an obvious reason why.Therefore, I think it's overall more straightforward to keep the
RuboCop
naming to match upstream, and be consistent across our constants and theirs, and have this one workaround method with the comment documenting why it's there. I think this would be in line with the "principle of least surprise", in that this one method makes the rest of the app less surprising because things just work.