[GH-7682] Add ExpressionDeclaration, AssignmentsProvider, and new call-site API to eliminate raw AST usage in existing lint rules#34
Conversation
…l-site API to eliminate raw AST usage in existing lint rules
There was a problem hiding this comment.
Pull request overview
This PR expands Rubyzen’s AST-wrapping API to reduce (or eliminate) raw RuboCop AST usage in lint rules by introducing first-class “value expression” and “assignment” primitives, plus new call-site convenience APIs. It also updates the sample project with new lint rules demonstrating the new APIs and bumps the gem version to 0.3.0.
Changes:
- Add
ExpressionDeclaration(+ExpressionsCollection) to model receivers/arguments/return values with kind predicates (constant/constructor/local var/hash literal/etc.). - Add
AssignmentDeclaration(+AssignmentsProvider/AssignmentsCollection) andReturnExpressionsProviderwith collection bridges on methods/blocks. - Extend
CallSiteDeclarationwith#arguments,#receiver_expression, and#enclosing_blocks, and add sample lint rules exercising these APIs.
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/declarations/expression_declaration_spec.rb | Adds specs for expression predicates and call-site receiver/argument expression modeling. |
| spec/declarations/call_site_declaration_spec.rb | Adds specs for #receiver_expression, #arguments, and #enclosing_blocks. |
| spec/declarations/assignment_declaration_spec.rb | Adds specs for assignment name/value expression behavior (including masgn targets). |
| spec/collections/expressions_collection_spec.rb | Adds specs for expression collection filters and type-preserving filters. |
| spec/collections/assignments_collection_spec.rb | Adds specs for assignments collection name filtering and type preservation. |
| sample_project/src/tests/stubbing_test.rb | Adds sample “violation” source for stubbing Repos constants. |
| sample_project/src/tests/admin_flow_test.rb | Adds sample “violation” source for admin calls without context wrapper. |
| sample_project/src/serializers/user_serializer.rb | Adds sample class for “hash literal return” lint rule. |
| sample_project/src/questions/profile_status.rb | Adds sample class for “no side effects in questions” lint rule. |
| sample_project/spec/tests/no_stubbing_repos_lint_spec.rb | Adds lint rule using CallSiteDeclaration#arguments + ExpressionDeclaration#constant_name. |
| sample_project/spec/tests/admin_calls_use_with_admin_context_lint_spec.rb | Adds lint rule using CallSiteDeclaration#enclosing_blocks. |
| sample_project/spec/spec_helper.rb | Adds questions collection helper for question lint rules. |
| sample_project/spec/questions/no_side_effects_in_questions_lint_spec.rb | Adds lint rule using #receiver_expression + method #assignments to detect repo writes. |
| sample_project/spec/core/data_methods_return_data_objects_lint_spec.rb | Adds lint rule using #return_expressions.hash_literals. |
| lib/rubyzen/version.rb | Bumps gem version to 0.3.0. |
| lib/rubyzen/providers/return_expressions_provider.rb | Introduces provider for method/block return expressions. |
| lib/rubyzen/providers/enclosing_blocks_provider.rb | Introduces provider for retrieving enclosing blocks for a declaration. |
| lib/rubyzen/providers/assignments_provider.rb | Introduces provider for collecting local variable assignments. |
| lib/rubyzen/providers/arguments_provider.rb | Introduces provider for modeling call-site arguments as expressions. |
| lib/rubyzen/declarations/method_declaration.rb | Mixes in ReturnExpressionsProvider and AssignmentsProvider. |
| lib/rubyzen/declarations/expression_declaration.rb | Adds the new expression wrapper declaration and predicates/accessors. |
| lib/rubyzen/declarations/call_site_declaration.rb | Adds new call-site APIs and includes new providers. |
| lib/rubyzen/declarations/block_declaration.rb | Mixes in ReturnExpressionsProvider and AssignmentsProvider. |
| lib/rubyzen/declarations/assignment_declaration.rb | Adds the new assignment wrapper declaration. |
| lib/rubyzen/collections/methods_collection.rb | Adds collection bridges for return_expressions and assignments. |
| lib/rubyzen/collections/expressions_collection.rb | Adds expression collection filters (hash_literals, constants). |
| lib/rubyzen/collections/blocks_collection.rb | Adds collection bridges for return_expressions and assignments. |
| lib/rubyzen/collections/assignments_collection.rb | Adds the assignments collection type. |
| CLAUDE.md | Updates architecture docs with new APIs and data-flow. |
| CHANGELOG.md | Documents the 0.3.0 additions. |
| nodes = [] | ||
| body = node.body | ||
| nodes << (body.begin_type? ? body.children.last : body) unless body.nil? | ||
| node.each_descendant(:return).each do |return_node| | ||
| nodes << return_node.children.first | ||
| end |
There was a problem hiding this comment.
I extracted a ReturnDeclaration (see your other comment below) so value-extraction no longer happens inline in the provider.
| # Filters to only constant (or constructor-of-constant) expressions. | ||
| # | ||
| # @return [ExpressionsCollection] | ||
| def constants | ||
| filter(&:constant?) | ||
| end |
There was a problem hiding this comment.
Agreed — filters now on constant_name.
| module ArgumentsProvider | ||
| # @return [Rubyzen::Collections::ExpressionsCollection] | ||
| def arguments | ||
| Collections::ExpressionsCollection.new( |
There was a problem hiding this comment.
Is it accurate that arguments returns an ExpressionsCollection?
There was a problem hiding this comment.
Future-proofing — done. arguments now returns a dedicated ArgumentsCollection. It's a subclass of ExpressionsCollection for now.
There was a problem hiding this comment.
Allows for argument-specific filters / behaviour later without a breaking change.
| nodes = [] | ||
| body = node.body | ||
| nodes << (body.begin_type? ? body.children.last : body) unless body.nil? | ||
| node.each_descendant(:return).each do |return_node| | ||
| nodes << return_node.children.first |
There was a problem hiding this comment.
I think we shouldn't do the parsing in the provider. Maybe this is a sign that we're missing a ReturnDeclaration and Collection?
| let(:admin_calls) do | ||
| test_source_files.call_sites | ||
| .with_name('get') | ||
| .filter { |cs| cs.source_code.match?(%r{['"]/admin/}) } |
There was a problem hiding this comment.
Is there any API we can use to avoid the regex?
| # Violation: a write call via a local variable assigned a Repo | ||
| # (assignment + local-variable receiver). |
There was a problem hiding this comment.
I'm not sure if we want these Violation / Compliant comments since they're specific to one lint rule, while multiple lint rules will lint this file.
Same for the two other files below.
| - **`ExpressionDeclaration`** — a value-expression primitive wrapping any AST node, with | ||
| predicates (`constant?`, `local_variable?`, `method_call?`, `constructor?`, `hash_literal?`, | ||
| `symbol?`, `string?`) and accessors (`constant_name`, `method_name`, `name`). Surfaced through | ||
| new providers so rules can inspect values without dropping to the raw AST. |
There was a problem hiding this comment.
I would remove this line ("Surfaced through new providers so rules .."). It looks very AI-generated and it's also apparent that we're building new APIs to avoid raw AST access.
| module Providers | ||
| # Provides the chain of blocks (do..end / { }) that lexically enclose a declaration, | ||
| # innermost first. | ||
| module EnclosingBlocksProvider |
There was a problem hiding this comment.
Is the naming accurate considering we have a similar one? (BlocksProvider)?
There was a problem hiding this comment.
I think the names are accurate and complementary: BlocksProvider returns the blocks contained within a node, whereas EnclosingBlocksProvider returns the blocks that wrap it. Kept as-is, but happy to revisit if you'd prefer something else.
- Extract ReturnDeclaration + ReturnsCollection + ReturnsProvider; #returns is now the primary API and #return_expressions a shortcut for returns.expressions. Fixes two correctness bugs in the old provider: explicit begin..end (kwbegin) bodies now surface their final expression, and the `return` node itself no longer leaks into the expression collection. - ExpressionsCollection#constants now filters on constant_name, so it includes constructor-of-constant expressions (e.g. Repos::Foo.new) per its documented contract. - Introduce ArgumentsCollection (a subclass of ExpressionsCollection) as the return type of CallSiteDeclaration#arguments, so argument-specific filters can be added later without a breaking change. - Admin lint rule uses CallSiteDeclaration#strings instead of a source_code regex. - Remove rule-specific Violation/Compliant comments from sample source files. - CHANGELOG: drop AI-sounding line; document the returns API. CLAUDE.md updated.
What changed and why
Checklist
bundle exec rakepasses locally (runs both the RSpec and Minitest suites)