diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6515afb..a3010be 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -61,3 +61,65 @@ jobs: - name: Run cargo clippy run: cargo clippy --all-targets --all-features + build: + # dist builds releases on these runners, the ones its plan picks for the + # targets in dist-workspace.toml, but only when releasing. Prism is C, so + # each needs a C compiler and the libclang that bindgen uses. + name: Build (${{ matrix.target }}) + strategy: + fail-fast: false + matrix: + include: + - target: aarch64-apple-darwin + runner: macos-26 + - target: aarch64-unknown-linux-gnu + runner: ubuntu-22.04-arm + - target: x86_64-unknown-linux-gnu + runner: ubuntu-22.04 + - target: x86_64-unknown-linux-musl + runner: ubuntu-22.04 + - target: x86_64-pc-windows-msvc + runner: windows-2022 + # dist links the C runtime statically, so cc builds Prism with /MT + rustflags: -Ctarget-feature=+crt-static + runs-on: ${{ matrix.runner }} + env: + TARGET: ${{ matrix.target }} + RUSTFLAGS: ${{ matrix.rustflags }} + steps: + - name: Checkout sources + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + + # As dist does for this target + - name: Install musl-tools + if: matrix.target == 'x86_64-unknown-linux-musl' + run: sudo apt-get update && sudo apt-get install -y musl-tools + + - name: Run cargo build + shell: bash + run: | + rustup toolchain install + rustup target add "$TARGET" + cargo build --profile dist --target "$TARGET" + + # Only Test Suite runs Prism otherwise, and only on x86_64 Linux + - name: Run pks on a fixture + shell: bash + run: | + "target/$TARGET/dist/pks" --project-root tests/fixtures/simple_app \ + --no-cache list-references | grep -F '"constant_name":"::Bar"' + build-all: + # The one check for the ruleset to require, since the matrix's names change + # with its targets. A skipped required check counts as passing, so this runs + # even when a build fails, and fails then. + name: Build + needs: build + if: always() + runs-on: ubuntu-latest + steps: + - name: Check that every target built + env: + RESULT: ${{ needs.build.result }} + run: test "$RESULT" = success diff --git a/.github/workflows/dotslash-config.json b/.github/workflows/dotslash-config.json index 66b5884..708acc3 100644 --- a/.github/workflows/dotslash-config.json +++ b/.github/workflows/dotslash-config.json @@ -7,11 +7,6 @@ "path": "pks-aarch64-apple-darwin/pks", "format": "tar.xz" }, - "macos-x86_64": { - "name": "pks-x86_64-apple-darwin.tar.xz", - "path": "pks-x86_64-apple-darwin/pks", - "format": "tar.xz" - }, "linux-aarch64": { "name": "pks-aarch64-unknown-linux-gnu.tar.xz", "path": "pks-aarch64-unknown-linux-gnu/pks", diff --git a/AGENTS.md b/AGENTS.md index a7f7a1b..fabc526 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -43,9 +43,9 @@ Rustfmt is configured with `max_width=80` (`.rustfmt.toml`). CI runs clippy with - `src/packs/configuration.rs` — `Configuration` struct (the central data object passed everywhere). Built from `packwerk.yml` via `raw_configuration.rs` + `walk_directory.rs`. - `src/packs/checker.rs` — Core violation-checking engine. Defines `CheckerInterface` and `ValidatorInterface` traits, `Violation`/`ViolationIdentifier` types, and `check_all()`/`update()`/`validate_all()`. - `src/packs/checker/` — Individual checkers: `dependency`, `privacy`, `visibility`, `layer`, `folder_privacy`. Each implements `CheckerInterface`. -- `src/packs/parsing/` — File parsing. Two modes: +- `src/packs/parsing/` — File parsing, with [Prism](https://github.com/ruby/prism) through the `ruby-prism` crate. Two modes, which resolve constants differently: - **Packwerk mode** (default): Zeitwerk-based constant resolution from file paths (`parsing/ruby/zeitwerk/`) - - **Experimental mode** (`--experimental-parser`): AST-based using `lib-ruby-parser` (`parsing/ruby/experimental/`) + - **Experimental mode** (`--experimental-parser`): resolution from the definitions parsed out of every file (`parsing/ruby/experimental/`) - Both Ruby (`.rb`, `.rake`, `Gemfile`, `.gemspec`) and ERB (`.erb`) are supported. - `src/packs/pack.rs` / `pack_set.rs` — `Pack` struct (represents one `package.yml`) and `PackSet` (the collection). - `src/packs/package_todo.rs` — Read/write `package_todo.yml` files that record allowed violations. diff --git a/CHANGELOG.md b/CHANGELOG.md index aac1686..06bf76a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,36 @@ # Changelog +## Unreleased + +### Changes + +#### pks parses Ruby with Prism + +pks now parses Ruby with [Prism](https://github.com/ruby/prism), the parser +Ruby itself has used since 3.4, in place of lib-ruby-parser, which supports +Ruby syntax only up to 3.1 and is no longer maintained. Files that use newer +syntax, such as Ruby 3.2's anonymous argument forwarding (`def foo(*) = +bar(*)`), no longer fail to parse, so pks finds their references and no longer +warns about them. + +On files that lib-ruby-parser could parse, the references and definitions pks +finds are the same, except where newer Rubies read code differently, since +Prism reads Ruby as its newest version does. In a block that names no +parameters, `it` is the block's parameter, not a method call, so with +`--experimental-parser` it no longer makes a class count as defined. pks also +warns about syntax that newer Rubies reject, such as `a[k: 1] = v`. + +The syntax errors it warns about read as Prism words them, and a reference in +a file with an error can be found where it wasn't before, since Prism recovers +more of the file. + +Building pks from source now needs a C compiler and libclang. + +#### No Intel macOS binary + +Releases no longer include a binary for Intel Macs (`x86_64-apple-darwin`), +and the Apple Silicon binary is built on macOS 26. + ## 0.7.0 ### Features diff --git a/Cargo.toml b/Cargo.toml index 892c495..f1037ff 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -62,7 +62,7 @@ tracing = "0.1.37" # logging tracing-subscriber = { version = "0.3.16", features = ["env-filter"] } # logging glob = "0.3.1" # globbing globset = "0.4" # globbing -lib-ruby-parser = "4.0.6" # ruby parser +ruby-prism = "1.9.0" # ruby parser md5 = "0.7.0" # md5 hashing to take and compare md5 digests of file contents to ensure cache validity line-col = "0.2.1" # for creating source maps of violations ruby_inflector = '0.0.8' # for inflecting strings, e.g. turning `has_many :companies` into `Company` diff --git a/INSTALLATION.md b/INSTALLATION.md index 8bb2784..df5adc4 100644 --- a/INSTALLATION.md +++ b/INSTALLATION.md @@ -6,11 +6,11 @@ - Download the `pks` DotSlash file from a release, for example https://github.com/rubyatscale/pks/releases/latest/download/pks, and save it to your Ruby project's `bin/` directory. - Run `bin/pks`. -The file pins that release's binaries by hash, so everyone working on the project runs the same version. It covers macOS, Linux (x86_64 and aarch64), and Windows (x86_64). +The file pins that release's binaries by hash, so everyone working on the project runs the same version. It covers macOS (Apple Silicon), Linux (x86_64 and aarch64), and Windows (x86_64). ## Option 2: Prebuilt binary -From v0.5.0, every release publishes prebuilt binaries for macOS, Linux, and Windows, along with installer scripts that put `pks` in `$CARGO_HOME/bin` (usually `~/.cargo/bin`). +From v0.5.0, every release publishes prebuilt binaries for macOS, Linux, and Windows, along with installer scripts that put `pks` in `$CARGO_HOME/bin` (usually `~/.cargo/bin`). Releases after 0.7.0 build macOS binaries for Apple Silicon only. macOS and Linux: @@ -29,6 +29,7 @@ To pin a version, replace `latest/download` with `download/v0.5.0` (or whichever ## Option 3: Build from source - Install Rust: https://www.rust-lang.org/tools/install +- Have a C compiler and libclang. pks's Ruby parser, [Prism](https://github.com/ruby/prism), is written in C, and its Rust bindings are generated when it builds. Xcode's command line tools provide both on macOS. On Debian and Ubuntu, `build-essential` provides the C compiler, which Rust needs there too, and `libclang-dev` provides libclang. On Windows, install LLVM (`winget install LLVM.LLVM`), and set `LIBCLANG_PATH` to its `bin` directory if it isn't in `C:\Program Files\LLVM`. For other systems, see bindgen's [requirements](https://rust-lang.github.io/rust-bindgen/requirements.html). - `cargo install --git https://github.com/rubyatscale/pks` Note that `cargo install pks` installs the crates.io `pks` crate, which is the original [alexevanczuk/packs](https://github.com/alexevanczuk/packs) at 0.2.40, not this repository. diff --git a/dist-workspace.toml b/dist-workspace.toml index 2a08fba..268cdb0 100644 --- a/dist-workspace.toml +++ b/dist-workspace.toml @@ -13,7 +13,6 @@ installers = ["shell", "powershell"] targets = [ "aarch64-apple-darwin", "aarch64-unknown-linux-gnu", - "x86_64-apple-darwin", "x86_64-unknown-linux-gnu", "x86_64-unknown-linux-musl", "x86_64-pc-windows-msvc", @@ -31,6 +30,11 @@ post-announce-jobs = ["./publish-dotslash"] # dispatches one when Cargo.toml's version changes on main. dispatch-releases = true +# dist's default for this target is macos-14, which GitHub has deprecated. +# ci.yml builds on the same runner. +[dist.github-custom-runners] +aarch64-apple-darwin = "macos-26" + [dist.github-custom-job-permissions] "publish-dotslash" = { contents = "write" } diff --git a/rust-toolchain.toml b/rust-toolchain.toml index efba89c..c352c54 100644 --- a/rust-toolchain.toml +++ b/rust-toolchain.toml @@ -1,4 +1,4 @@ [toolchain] channel = "1.97.1" components = ["clippy", "rustfmt"] -targets = ["x86_64-apple-darwin", "aarch64-apple-darwin", "x86_64-unknown-linux-gnu"] +targets = ["aarch64-apple-darwin", "x86_64-unknown-linux-gnu"] diff --git a/src/packs/parsing/erb/experimental/parser.rs b/src/packs/parsing/erb/experimental/parser.rs index c728c90..8039d2f 100644 --- a/src/packs/parsing/erb/experimental/parser.rs +++ b/src/packs/parsing/erb/experimental/parser.rs @@ -6,7 +6,7 @@ use crate::packs::{ }; use std::path::Path; -use crate::packs::parsing::ruby::experimental::parser::process_from_contents as process_from_ruby_contents; +use crate::packs::parsing::ruby::experimental::parser::process_from_template_contents; pub(crate) fn process_from_path( path: &Path, @@ -23,7 +23,7 @@ pub(crate) fn process_from_contents( ) -> ProcessedFile { let ruby_contents = convert_erb_to_ruby_without_sourcemaps(contents); let processed_file = - process_from_ruby_contents(ruby_contents, path, configuration); + process_from_template_contents(ruby_contents, path, configuration); let references = processed_file.unresolved_references; // let references_without_range = references let references_without_range = references diff --git a/src/packs/parsing/erb/packwerk/parser.rs b/src/packs/parsing/erb/packwerk/parser.rs index 7236382..a07e609 100644 --- a/src/packs/parsing/erb/packwerk/parser.rs +++ b/src/packs/parsing/erb/packwerk/parser.rs @@ -6,7 +6,7 @@ use crate::packs::{ }; use std::path::Path; -use crate::packs::parsing::ruby::packwerk::parser::process_from_contents as process_from_ruby_contents; +use crate::packs::parsing::ruby::packwerk::parser::process_from_template_contents; pub(crate) fn process_from_path( path: &Path, @@ -23,7 +23,7 @@ pub(crate) fn process_from_contents( ) -> ProcessedFile { let ruby_contents = convert_erb_to_ruby_without_sourcemaps(contents); let processed_file = - process_from_ruby_contents(ruby_contents, path, configuration); + process_from_template_contents(ruby_contents, path, configuration); let references = processed_file.unresolved_references; // let references_without_range = references let references_without_range = references diff --git a/src/packs/parsing/mod.rs b/src/packs/parsing/mod.rs index b682392..b95645a 100644 --- a/src/packs/parsing/mod.rs +++ b/src/packs/parsing/mod.rs @@ -99,7 +99,7 @@ fn warn_about_syntax_error( ); } -/// The first error lib-ruby-parser reports in a file. What it can't parse, pks +/// The first error Prism reports in a file. What it can't parse, pks /// can't find references in. #[derive(Debug, PartialEq, Eq, Clone)] pub struct SyntaxError { diff --git a/src/packs/parsing/ruby/experimental.rs b/src/packs/parsing/ruby/experimental.rs index d7297ce..ca99f0f 100644 --- a/src/packs/parsing/ruby/experimental.rs +++ b/src/packs/parsing/ruby/experimental.rs @@ -349,6 +349,73 @@ end assert_eq!(expected, actual); } + fn definition_names(contents: &str) -> Vec { + process_from_contents( + contents.to_owned(), + &PathBuf::from("path/to/file.rb"), + &Configuration::default(), + ) + .definitions + .into_iter() + .map(|definition| definition.fully_qualified_name) + .collect() + } + + #[test] + fn class_definition_with_calls_that_count() { + // Each changes the class's behavior + for body in [ + "@a.b", + "BAR.[](1)", + "self.a ||= 1", + "self.a &&= 1", + "self.a += 1", + "self.a, self.b = 1, 2", + "/(?.)/ =~ BAR", + ] { + let contents = format!("class Foo\n {}\nend\n", body); + assert_eq!(definition_names(&contents), vec!["::Foo"], "{}", body); + } + } + + #[test] + fn class_definition_with_calls_that_dont_count() { + for body in ["BAR[1]", "BAR[1] = 2", "@a&.b", "@a&.b ||= 1"] { + let contents = format!("class Foo\n {}\nend\n", body); + assert!(definition_names(&contents).is_empty(), "{}", body); + } + } + + #[test] + fn constant_assignments() { + // Each defines its targets, and only the value assigned is a reference, + // not a target or its scope + for (contents, definitions) in [ + ("A = Bar", &["::A"][..]), + ("A::B = Bar", &["::A::B"]), + ("A ||= Bar", &["::A"]), + ("A &&= Bar", &["::A"]), + ("A += Bar", &["::A"]), + ("A::B ||= Bar", &["::A::B"]), + ("A::B &&= Bar", &["::A::B"]), + ("A::B += Bar", &["::A::B"]), + ("A, B::C = Bar", &["::A", "::B::C"]), + ("begin\nrescue Bar => A\nend", &["::A"]), + ] { + let references: Vec = process_from_contents( + contents.to_owned(), + &PathBuf::from("path/to/file.rb"), + &Configuration::default(), + ) + .unresolved_references + .into_iter() + .map(|reference| reference.name) + .collect(); + assert_eq!(references, vec!["Bar"], "{}", contents); + assert_eq!(definition_names(contents), definitions, "{}", contents); + } + } + #[test] fn class_definition_with_private_constant() { let contents: String = String::from( diff --git a/src/packs/parsing/ruby/experimental/parser.rs b/src/packs/parsing/ruby/experimental/parser.rs index 997cce0..5ab0be4 100644 --- a/src/packs/parsing/ruby/experimental/parser.rs +++ b/src/packs/parsing/ruby/experimental/parser.rs @@ -2,19 +2,22 @@ use crate::packs::file_utils::file_read_contents; use crate::packs::{ parsing::{ ruby::parse_utils::{ - fetch_const_const_name, fetch_const_name, fetch_node_location, + constant_name, fetch_const_name, fetch_const_path_name, first_syntax_error, get_constant_assignment_definition, get_definition_from, get_reference_from_active_record_association, - loc_to_range, + is_plain_call, loc_to_range, visit_constant_assignments, + ParseError, }, ParsedDefinition, UnresolvedReference, }, Configuration, ProcessedFile, }; -use lib_ruby_parser::{ - nodes, traverse::visitor::Visitor, Node, Parser, ParserOptions, -}; use line_col::LineColLookup; +use ruby_prism::{ + CallAndWriteNode, CallNode, CallOperatorWriteNode, CallOrWriteNode, + CallTargetNode, ClassNode, ConstantPathNode, ConstantReadNode, DefNode, + Location, ModuleNode, Visit, +}; use std::path::Path; struct ReferenceCollector<'a> { @@ -26,10 +29,58 @@ struct ReferenceCollector<'a> { pub custom_associations: Vec, } -impl Visitor for ReferenceCollector<'_> { - fn on_class(&mut self, node: &nodes::Class) { - // We're not collecting definitions, so no need to visit the class definitioname); - let namespace_result = fetch_const_name(&node.name); +impl ReferenceCollector<'_> { + fn define_constant( + &mut self, + name: Result, + location: &Location, + ) { + let definition = get_constant_assignment_definition( + name, + location, + self.current_namespaces.to_owned(), + &self.line_col_lookup, + ); + + if let Some(definition) = definition { + self.definitions.push(definition); + } + } + + fn reference_constant( + &mut self, + name: Result, + location: &Location, + ) { + let Ok(name) = name else { + return; + }; + + let namespace_path = self + .current_namespaces + .clone() + .into_iter() + .filter(|namespace| namespace != &name) + .collect::>(); + + self.references.push(UnresolvedReference { + name, + namespace_path, + location: loc_to_range(location, &self.line_col_lookup), + }) + } + + /// `a.b ||= c` and the targets of `a.b, c.d = e` call `a.b=` too + fn on_call_assignment(&mut self, safe_navigation: bool) { + if !safe_navigation { + self.behavioral_change_in_namespace = true; + } + } +} + +impl<'pr> Visit<'pr> for ReferenceCollector<'_> { + fn visit_class_node(&mut self, node: &ClassNode<'pr>) { + let namespace_result = fetch_const_name(&node.constant_path()); // For now, we simply exit and stop traversing if we encounter an error when fetching the constant name of a class // We can iterate on this if this is different than the packwerk implementation if namespace_result.is_err() { @@ -38,11 +89,13 @@ impl Visitor for ReferenceCollector<'_> { let namespace = namespace_result.unwrap(); - if let Some(inner) = node.superclass.as_ref() { - self.visit(inner); + if let Some(inner) = node.superclass() { + self.visit(&inner); } - let definition_loc = fetch_node_location(&node.name).unwrap(); - let location = loc_to_range(definition_loc, &self.line_col_lookup); + let location = loc_to_range( + &node.constant_path().location(), + &self.line_col_lookup, + ); let definition = get_definition_from( &namespace, @@ -50,7 +103,7 @@ impl Visitor for ReferenceCollector<'_> { &location, ); - // Note – is there a way to use lifetime specifiers to get rid of this and + // Note – is there a way to use lifetime specifiers to get rid of this and // just keep current namespaces as a vector of string references or something else // more efficient? self.current_namespaces.push(namespace); @@ -59,8 +112,8 @@ impl Visitor for ReferenceCollector<'_> { let previous_behavioral_change = self.behavioral_change_in_namespace; self.behavioral_change_in_namespace = false; - if let Some(inner) = &node.body { - self.visit(inner); + if let Some(inner) = node.body() { + self.visit(&inner); } if self.behavioral_change_in_namespace { @@ -74,10 +127,10 @@ impl Visitor for ReferenceCollector<'_> { self.current_namespaces.pop(); } - fn on_send(&mut self, node: &nodes::Send) { - if node.method_name == "private_constant" { - // `private_constant` is not considered to be a behavioral change - } else { + fn visit_call_node(&mut self, node: &CallNode<'pr>) { + // `private_constant` is not considered to be a behavioral change + if is_plain_call(node) && node.name().as_slice() != b"private_constant" + { self.behavioral_change_in_namespace = true; let association_reference = @@ -92,33 +145,41 @@ impl Visitor for ReferenceCollector<'_> { self.references.push(association_reference); } } - lib_ruby_parser::traverse::visitor::visit_send(self, node); + ruby_prism::visit_call_node(self, node); } - fn on_casgn(&mut self, node: &nodes::Casgn) { - let definition = get_constant_assignment_definition( - node, - self.current_namespaces.to_owned(), - &self.line_col_lookup, - ); + fn visit_call_or_write_node(&mut self, node: &CallOrWriteNode<'pr>) { + self.on_call_assignment(node.is_safe_navigation()); + ruby_prism::visit_call_or_write_node(self, node); + } - if let Some(definition) = definition { - self.definitions.push(definition); - } + fn visit_call_and_write_node(&mut self, node: &CallAndWriteNode<'pr>) { + self.on_call_assignment(node.is_safe_navigation()); + ruby_prism::visit_call_and_write_node(self, node); + } - if let Some(v) = node.value.to_owned() { - self.visit(&v); - } else { - // We don't handle constant assignments as part of a multi-assignment yet, - // e.g. A, B = 1, 2 - // See the documentation for nodes::Casgn#value for more info. - } + fn visit_call_operator_write_node( + &mut self, + node: &CallOperatorWriteNode<'pr>, + ) { + self.on_call_assignment(node.is_safe_navigation()); + ruby_prism::visit_call_operator_write_node(self, node); + } + + fn visit_call_target_node(&mut self, node: &CallTargetNode<'pr>) { + self.on_call_assignment(node.is_safe_navigation()); + ruby_prism::visit_call_target_node(self, node); } - fn on_module(&mut self, node: &nodes::Module) { - let namespace = fetch_const_name(&node.name).unwrap_or("".to_owned()); - let definition_loc = fetch_node_location(&node.name).unwrap(); - let location = loc_to_range(definition_loc, &self.line_col_lookup); + visit_constant_assignments!(); + + fn visit_module_node(&mut self, node: &ModuleNode<'pr>) { + let namespace = + fetch_const_name(&node.constant_path()).unwrap_or("".to_owned()); + let location = loc_to_range( + &node.constant_path().location(), + &self.line_col_lookup, + ); let definition = get_definition_from( &namespace, @@ -126,7 +187,7 @@ impl Visitor for ReferenceCollector<'_> { &location, ); - // Note – is there a way to use lifetime specifiers to get rid of this and + // Note – is there a way to use lifetime specifiers to get rid of this and // just keep current namespaces as a vector of string references or something else // more efficient? self.current_namespaces.push(namespace); @@ -135,8 +196,8 @@ impl Visitor for ReferenceCollector<'_> { let previous_behavioral_change = self.behavioral_change_in_namespace; self.behavioral_change_in_namespace = false; - if let Some(inner) = &node.body { - self.visit(inner); + if let Some(inner) = node.body() { + self.visit(&inner); } if self.behavioral_change_in_namespace { @@ -150,33 +211,26 @@ impl Visitor for ReferenceCollector<'_> { self.current_namespaces.pop(); } - fn on_const(&mut self, node: &nodes::Const) { - let Ok(name) = fetch_const_const_name(node) else { - return; - }; - - let namespace_path = self - .current_namespaces - .clone() - .into_iter() - .filter(|namespace| namespace != &name) - .collect::>(); - - self.references.push(UnresolvedReference { - name, - namespace_path, - location: loc_to_range(&node.expression_l, &self.line_col_lookup), - }) + fn visit_constant_read_node(&mut self, node: &ConstantReadNode<'pr>) { + self.reference_constant( + Ok(constant_name(&node.name())), + &node.location(), + ); } - fn on_def(&mut self, node: &nodes::Def) { - self.behavioral_change_in_namespace = true; - lib_ruby_parser::traverse::visitor::visit_def(self, node); + // Not visiting the parent: `Foo::Bar` is one reference, and in + // `foo(Bar)::Baz`, neither constant is one + fn visit_constant_path_node(&mut self, node: &ConstantPathNode<'pr>) { + self.reference_constant( + fetch_const_path_name(node.parent(), node.name()), + &node.location(), + ); } - fn on_defs(&mut self, node: &nodes::Defs) { + // A `def` with a receiver, as `def self.foo`, too + fn visit_def_node(&mut self, node: &DefNode<'pr>) { self.behavioral_change_in_namespace = true; - lib_ruby_parser::traverse::visitor::visit_defs(self, node); + ruby_prism::visit_def_node(self, node); } } @@ -193,29 +247,30 @@ pub(crate) fn process_from_contents( path: &Path, configuration: &Configuration, ) -> ProcessedFile { - let options = ParserOptions { - buffer_name: "".to_string(), - ..Default::default() - }; + // Rails runs a Builder template, as it does an ERB one, as a method body + let template = path.extension().is_some_and(|ext| ext == "builder"); + process(contents, path, configuration, template) +} + +/// For the Ruby that pks makes from an ERB template +pub(crate) fn process_from_template_contents( + contents: String, + path: &Path, + configuration: &Configuration, +) -> ProcessedFile { + process(contents, path, configuration, true) +} +fn process( + contents: String, + path: &Path, + configuration: &Configuration, + template: bool, +) -> ProcessedFile { let lookup = LineColLookup::new(&contents); - let parser = Parser::new(contents.clone(), options); - let parse_result = parser.do_parse(); - let syntax_error = first_syntax_error(&parse_result.diagnostics, &lookup); - - let ast_option: Option> = parse_result.ast; - - let ast = match ast_option { - Some(some_ast) => some_ast, - None => { - return ProcessedFile { - absolute_path: path.to_owned(), - unresolved_references: vec![], - definitions: vec![], - syntax_error, - } - } - }; + let parse_result = ruby_prism::parse(contents.as_bytes()); + let syntax_error = + first_syntax_error(parse_result.errors(), &lookup, template); let mut collector = ReferenceCollector { references: vec![], @@ -226,7 +281,7 @@ pub(crate) fn process_from_contents( custom_associations: configuration.custom_associations.clone(), }; - collector.visit(&ast); + collector.visit(&parse_result.node()); let unresolved_references = collector.references; diff --git a/src/packs/parsing/ruby/packwerk.rs b/src/packs/parsing/ruby/packwerk.rs index faa5181..32f6f58 100644 --- a/src/packs/parsing/ruby/packwerk.rs +++ b/src/packs/parsing/ruby/packwerk.rs @@ -867,6 +867,90 @@ FOO = BAR ) } + #[test] + fn constant_assignments_define_local_constants() { + // Only the value assigned is a reference, not a target or its scope, + // nor a later use of a target, which is defined here + for contents in [ + "A = Bar\nA", + "A::B = Bar\nA::B", + "A ||= Bar\nA", + "A &&= Bar\nA", + "A += Bar\nA", + "A::B ||= Bar\nA::B", + "A::B &&= Bar\nA::B", + "A::B += Bar\nA::B", + "A, B::C = Bar\nA\nB::C", + "begin\nrescue Bar => A\nend\nA", + ] { + let references: Vec = process_from_contents( + contents.to_owned(), + &PathBuf::from("path/to/file.rb"), + &Configuration::default(), + ) + .unresolved_references + .into_iter() + .map(|reference| reference.name) + .collect(); + assert_eq!(references, vec!["Bar"], "{}", contents); + } + } + + #[test] + fn class_or_module_named_by_an_expression() { + // pks skips the class or module, but not what follows it + for keyword in ["class", "module"] { + let contents = format!( + "module Outer\n {} self::Foo\n Bar\n end\n Baz\nend\n", + keyword + ); + let references: Vec<(String, Vec)> = process_from_contents( + contents, + &PathBuf::from("path/to/file.rb"), + &Configuration::default(), + ) + .unresolved_references + .into_iter() + .map(|reference| (reference.name, reference.namespace_path)) + .collect(); + assert_eq!( + references, + vec![ + (String::from("::Outer"), vec![]), + (String::from("Baz"), vec![String::from("Outer")]), + ], + "{}", + keyword + ); + } + } + + #[test] + fn association_calls() { + // Where each finds `Bar`. pks has never read a `&.` call as an + // association, and a block doesn't move where one starts. + for (body, bar) in [ + ("self.has_many :bars", Some((2, 2))), + ("self&.has_many :bars", None), + ("has_many :bars do\n Baz\n end", Some((2, 2))), + ] { + let contents = format!("class Foo\n {}\nend\n", body); + let references = process_from_contents( + contents, + &PathBuf::from("path/to/file.rb"), + &Configuration::default(), + ) + .unresolved_references; + let found = references + .iter() + .find(|reference| reference.name == "Bar") + .map(|reference| { + (reference.location.start_row, reference.location.start_col) + }); + assert_eq!(found, bar, "{}", body); + } + } + #[test] fn has_one_association() { let contents: String = String::from( diff --git a/src/packs/parsing/ruby/packwerk/parser.rs b/src/packs/parsing/ruby/packwerk/parser.rs index 5184f42..4779339 100644 --- a/src/packs/parsing/ruby/packwerk/parser.rs +++ b/src/packs/parsing/ruby/packwerk/parser.rs @@ -4,20 +4,22 @@ use crate::packs::{ ruby::{ namespace_calculator::possible_fully_qualified_constants, parse_utils::{ - fetch_const_const_name, fetch_const_name, fetch_node_location, + constant_name, fetch_const_name, fetch_const_path_name, first_syntax_error, get_constant_assignment_definition, get_definition_from, - get_reference_from_active_record_association, loc_to_range, + get_reference_from_active_record_association, is_plain_call, + loc_to_range, visit_constant_assignments, ParseError, }, }, ParsedDefinition, Range, UnresolvedReference, }, Configuration, ProcessedFile, }; -use lib_ruby_parser::{ - nodes, traverse::visitor::Visitor, Node, Parser, ParserOptions, -}; use line_col::LineColLookup; +use ruby_prism::{ + CallNode, ClassNode, ConstantPathNode, ConstantReadNode, Location, + ModuleNode, Visit, +}; use serde::{Deserialize, Serialize}; use std::{collections::HashMap, path::Path}; @@ -37,10 +39,74 @@ struct ReferenceCollector<'a> { pub custom_associations: Vec, } -impl Visitor for ReferenceCollector<'_> { - fn on_class(&mut self, node: &nodes::Class) { - // We're not collecting definitions, so no need to visit the class definitioname); - let namespace_result = fetch_const_name(&node.name); +impl ReferenceCollector<'_> { + fn define_constant( + &mut self, + name: Result, + location: &Location, + ) { + let definition = get_constant_assignment_definition( + name, + location, + self.current_namespaces.to_owned(), + &self.line_col_lookup, + ); + + if let Some(definition) = definition { + self.definitions.push(definition); + } + } + + fn reference_constant( + &mut self, + name: Result, + location: &Location, + ) { + let Ok(name) = name else { + return; + }; + + if self.in_superclass { + self.superclasses.push(SuperclassReference { + name: name.to_owned(), + namespace_path: self.current_namespaces.to_owned(), + }) + } + // In packwerk, NodeHelpers.enclosing_namespace_path ignores + // namespaces where a superclass OR namespace is the same as the current reference name + let matching_superclass_option = self + .superclasses + .iter() + .find(|superclass| superclass.name == name); + + let namespace_path = + if let Some(matching_superclass) = matching_superclass_option { + matching_superclass.namespace_path.to_owned() + } else { + self.current_namespaces + .clone() + .into_iter() + .filter(|namespace| { + namespace != &name + || self + .superclasses + .iter() + .any(|superclass| superclass.name == name) + }) + .collect::>() + }; + + self.references.push(UnresolvedReference { + name, + namespace_path, + location: loc_to_range(location, &self.line_col_lookup), + }) + } +} + +impl<'pr> Visit<'pr> for ReferenceCollector<'_> { + fn visit_class_node(&mut self, node: &ClassNode<'pr>) { + let namespace_result = fetch_const_name(&node.constant_path()); // For now, we simply exit and stop traversing if we encounter an error when fetching the constant name of a class // We can iterate on this if this is different than the packwerk implementation if namespace_result.is_err() { @@ -49,13 +115,15 @@ impl Visitor for ReferenceCollector<'_> { let namespace = namespace_result.unwrap(); - if let Some(inner) = node.superclass.as_ref() { + if let Some(inner) = node.superclass() { self.in_superclass = true; - self.visit(inner); + self.visit(&inner); self.in_superclass = false; } - let definition_loc = fetch_node_location(&node.name).unwrap(); - let location = loc_to_range(definition_loc, &self.line_col_lookup); + let location = loc_to_range( + &node.constant_path().location(), + &self.line_col_lookup, + ); let definition = get_definition_from( &namespace, @@ -74,61 +142,50 @@ impl Visitor for ReferenceCollector<'_> { location, }); - // Note – is there a way to use lifetime specifiers to get rid of this and + // Note – is there a way to use lifetime specifiers to get rid of this and // just keep current namespaces as a vector of string references or something else // more efficient? self.current_namespaces.push(namespace); - if let Some(inner) = &node.body { - self.visit(inner); + if let Some(inner) = node.body() { + self.visit(&inner); } self.current_namespaces.pop(); self.superclasses.pop(); } - fn on_send(&mut self, node: &nodes::Send) { - let association_reference = - get_reference_from_active_record_association( - node, - &self.current_namespaces, - &self.line_col_lookup, - &self.custom_associations, - ); - - if let Some(association_reference) = association_reference { - self.references.push(association_reference); + fn visit_call_node(&mut self, node: &CallNode<'pr>) { + if is_plain_call(node) { + let association_reference = + get_reference_from_active_record_association( + node, + &self.current_namespaces, + &self.line_col_lookup, + &self.custom_associations, + ); + + if let Some(association_reference) = association_reference { + self.references.push(association_reference); + } } - lib_ruby_parser::traverse::visitor::visit_send(self, node); + ruby_prism::visit_call_node(self, node); } - fn on_casgn(&mut self, node: &nodes::Casgn) { - let definition = get_constant_assignment_definition( - node, - self.current_namespaces.to_owned(), + visit_constant_assignments!(); + + fn visit_module_node(&mut self, node: &ModuleNode<'pr>) { + // A module whose name isn't a constant, as `module self::Foo`, or one + // in a file that doesn't parse + let Ok(namespace) = fetch_const_name(&node.constant_path()) else { + return; + }; + let location = loc_to_range( + &node.constant_path().location(), &self.line_col_lookup, ); - if let Some(definition) = definition { - self.definitions.push(definition); - } - - if let Some(v) = node.value.to_owned() { - self.visit(&v); - } else { - // We don't handle constant assignments as part of a multi-assignment yet, - // e.g. A, B = 1, 2 - // See the documentation for nodes::Casgn#value for more info. - } - } - - fn on_module(&mut self, node: &nodes::Module) { - let namespace = fetch_const_name(&node.name) - .expect("We expect no parse errors in class/module definitions"); - let definition_loc = fetch_node_location(&node.name).unwrap(); - let location = loc_to_range(definition_loc, &self.line_col_lookup); - let definition = get_definition_from( &namespace, &self.current_namespaces, @@ -146,58 +203,32 @@ impl Visitor for ReferenceCollector<'_> { location, }); - // Note – is there a way to use lifetime specifiers to get rid of this and + // Note – is there a way to use lifetime specifiers to get rid of this and // just keep current namespaces as a vector of string references or something else // more efficient? self.current_namespaces.push(namespace); - if let Some(inner) = &node.body { - self.visit(inner); + if let Some(inner) = node.body() { + self.visit(&inner); } self.current_namespaces.pop(); } - fn on_const(&mut self, node: &nodes::Const) { - let Ok(name) = fetch_const_const_name(node) else { - return; - }; - - if self.in_superclass { - self.superclasses.push(SuperclassReference { - name: name.to_owned(), - namespace_path: self.current_namespaces.to_owned(), - }) - } - // In packwerk, NodeHelpers.enclosing_namespace_path ignores - // namespaces where a superclass OR namespace is the same as the current reference name - let matching_superclass_option = self - .superclasses - .iter() - .find(|superclass| superclass.name == name); - - let namespace_path = - if let Some(matching_superclass) = matching_superclass_option { - matching_superclass.namespace_path.to_owned() - } else { - self.current_namespaces - .clone() - .into_iter() - .filter(|namespace| { - namespace != &name - || self - .superclasses - .iter() - .any(|superclass| superclass.name == name) - }) - .collect::>() - }; + fn visit_constant_read_node(&mut self, node: &ConstantReadNode<'pr>) { + self.reference_constant( + Ok(constant_name(&node.name())), + &node.location(), + ); + } - self.references.push(UnresolvedReference { - name, - namespace_path, - location: loc_to_range(&node.expression_l, &self.line_col_lookup), - }) + // Not visiting the parent: `Foo::Bar` is one reference, and in + // `foo(Bar)::Baz`, neither constant is one + fn visit_constant_path_node(&mut self, node: &ConstantPathNode<'pr>) { + self.reference_constant( + fetch_const_path_name(node.parent(), node.name()), + &node.location(), + ); } } @@ -214,29 +245,30 @@ pub(crate) fn process_from_contents( path: &Path, configuration: &Configuration, ) -> ProcessedFile { - let options = ParserOptions { - buffer_name: "".to_string(), - ..Default::default() - }; + // Rails runs a Builder template, as it does an ERB one, as a method body + let template = path.extension().is_some_and(|ext| ext == "builder"); + process(contents, path, configuration, template) +} + +/// For the Ruby that pks makes from an ERB template +pub(crate) fn process_from_template_contents( + contents: String, + path: &Path, + configuration: &Configuration, +) -> ProcessedFile { + process(contents, path, configuration, true) +} +fn process( + contents: String, + path: &Path, + configuration: &Configuration, + template: bool, +) -> ProcessedFile { let lookup = LineColLookup::new(&contents); - let parser = Parser::new(contents.clone(), options); - let parse_result = parser.do_parse(); - let syntax_error = first_syntax_error(&parse_result.diagnostics, &lookup); - - let ast_option: Option> = parse_result.ast; - - let ast = match ast_option { - Some(some_ast) => some_ast, - None => { - return ProcessedFile { - absolute_path: path.to_owned(), - unresolved_references: vec![], - definitions: vec![], - syntax_error, - } - } - }; + let parse_result = ruby_prism::parse(contents.as_bytes()); + let syntax_error = + first_syntax_error(parse_result.errors(), &lookup, template); let mut collector = ReferenceCollector { references: vec![], @@ -248,7 +280,7 @@ pub(crate) fn process_from_contents( custom_associations: configuration.custom_associations.clone(), }; - collector.visit(&ast); + collector.visit(&parse_result.node()); let mut definition_to_location_map: HashMap = HashMap::new(); diff --git a/src/packs/parsing/ruby/parse_utils.rs b/src/packs/parsing/ruby/parse_utils.rs index 2a0b2c1..155c630 100644 --- a/src/packs/parsing/ruby/parse_utils.rs +++ b/src/packs/parsing/ruby/parse_utils.rs @@ -1,7 +1,10 @@ use std::collections::HashSet; -use lib_ruby_parser::{nodes, Diagnostic, Loc, Node}; use line_col::LineColLookup; +use ruby_prism::{ + CallNode, ConstantId, Diagnostic, Diagnostics, KeywordHashNode, Location, + Node, +}; use crate::packs::parsing::{ ParsedDefinition, Range, SyntaxError, UnresolvedReference, @@ -15,18 +18,6 @@ pub enum ParseError { // Add more variants as needed for different error cases } -pub fn fetch_node_location(node: &nodes::Node) -> Result<&Loc, ParseError> { - match node { - Node::Const(const_node) => Ok(&const_node.expression_l), - node => { - panic!( - "Cannot handle other node in get_constant_node_name: {:?}", - node - ) - } - } -} - pub fn get_definition_from( current_nesting: &String, parent_nesting: &[String], @@ -50,64 +41,71 @@ pub fn get_definition_from( } } +/// The first error Prism reports, which is where it lost its way. Errors that +/// follow from it can sit earlier in the file. A template's `yield` is no +/// error, since Rails runs a template as the body of a method. pub fn first_syntax_error( - diagnostics: &[Diagnostic], + mut errors: Diagnostics, lookup: &LineColLookup, + template: bool, ) -> Option { - diagnostics - .iter() - .find(|diagnostic| diagnostic.is_error()) - .map(|diagnostic| SyntaxError { - line: Some(lookup.get(diagnostic.loc.begin).0), - message: diagnostic.render_message(), - }) + let error = errors.find(|error| { + !(template && error_message(error).as_deref() == Some("Invalid yield")) + }); + error.map(|error| SyntaxError { + line: Some(lookup.get(error.location().start_offset()).0), + message: error_message(&error) + .unwrap_or_else(|| String::from("its message isn't UTF-8")), + }) +} + +/// Some of Prism's messages quote the file, and `Diagnostic::message` panics on +/// a quote that ends mid-character, as one can when a magic comment names an +/// encoding other than UTF-8. The crate gives no other way to read a message. +fn error_message(error: &Diagnostic) -> Option { + std::panic::catch_unwind(|| error.message().to_owned()).ok() } -pub fn loc_to_range(loc: &Loc, lookup: &LineColLookup) -> Range { - let (start_row, start_col) = lookup.get(loc.begin); // There's an off-by-one difference here with packwerk - let (end_row, end_col) = lookup.get(loc.end); +pub fn loc_to_range(loc: &Location, lookup: &LineColLookup) -> Range { + let (start_row, start_col) = lookup.get(loc.start_offset()); + let (end_row, end_col) = lookup.get(loc.end_offset()); Range { start_row, + // line_col counts columns from 1, and packwerk from 0 start_col: start_col - 1, end_row, end_col, } } -pub fn fetch_const_name(node: &nodes::Node) -> Result { - match node { - Node::Const(const_node) => Ok(fetch_const_const_name(const_node)?), - Node::Cbase(_) => Ok(String::from("")), - Node::Send(_) => Err(ParseError::Metaprogramming), - Node::Lvar(_) => Err(ParseError::Metaprogramming), - Node::Ivar(_) => Err(ParseError::Metaprogramming), - Node::Self_(_) => Err(ParseError::Metaprogramming), - _node => Err(ParseError::Metaprogramming), - } +pub fn constant_name(id: &ConstantId) -> String { + String::from_utf8_lossy(id.as_slice()).into_owned() } -pub fn fetch_const_const_name( - node: &nodes::Const, -) -> Result { - match &node.scope { - Some(s) => { - let parent_namespace = fetch_const_name(s)?; - Ok(format!("{}::{}", parent_namespace, node.name)) - } - None => Ok(node.name.to_owned()), +/// `Foo`, `Foo::Bar` or `::Foo`, for a constant whose scope is only constants +pub fn fetch_const_name(node: &Node) -> Result { + if let Some(constant) = node.as_constant_read_node() { + Ok(constant_name(&constant.name())) + } else if let Some(path) = node.as_constant_path_node() { + fetch_const_path_name(path.parent(), path.name()) + } else { + Err(ParseError::Metaprogramming) } } -// TODO: Combine with fetch_const_const_name -fn fetch_casgn_name(node: &nodes::Casgn) -> Result { - match &node.scope { - Some(s) => { - let parent_namespace = fetch_const_name(s)?; - Ok(format!("{}::{}", parent_namespace, node.name)) - } - None => Ok(node.name.to_owned()), - } +/// A parent of None is `::`, as in `::Foo` +pub fn fetch_const_path_name( + parent: Option, + name: Option, +) -> Result { + // Prism leaves the name out of `Foo::` in a file that doesn't parse + let name = name.ok_or(ParseError::Metaprogramming)?; + let parent_namespace = match parent { + Some(parent) => fetch_const_name(&parent)?, + None => String::new(), + }; + Ok(format!("{}::{}", parent_namespace, constant_name(&name))) } const ASSOCIATION_METHOD_NAMES: [&str; 4] = [ @@ -118,7 +116,7 @@ const ASSOCIATION_METHOD_NAMES: [&str; 4] = [ ]; pub fn get_reference_from_active_record_association( - node: &nodes::Send, + node: &CallNode, current_namespaces: &[String], line_col_lookup: &LineColLookup, custom_associations: &[String], @@ -131,16 +129,21 @@ pub fn get_reference_from_active_record_association( .chain(ASSOCIATION_METHOD_NAMES.iter().copied().map(String::from)) .collect(); - let is_association = combined_associations.contains(&node.method_name); + let is_association = + combined_associations.contains(&constant_name(&node.name())); if is_association { - let first_arg: Option<&Node> = node.args.first(); + let args: Vec = node + .arguments() + .map(|arguments| arguments.arguments().iter().collect()) + .unwrap_or_default(); + let first_arg: Option<&Node> = args.first(); let inherited_resources = may_be_inherited_resources(current_namespaces); let mut name: Option = None; - for node in node.args.iter() { - if let Node::Kwargs(kwargs) = node { + for node in args.iter() { + if let Some(kwargs) = node.as_keyword_hash_node() { // Rails reads a polymorphic association's class from a type // column at runtime, so the declaration names no class, even // with `class_name:`. Rails 8.1 rejects that option here, and @@ -148,21 +151,21 @@ pub fn get_reference_from_active_record_association( // `counter_cache:`. With `counter_cache:`, 8.1 still loads the // class named after the association. pks counts neither load // as a reference. - if is_polymorphic(kwargs) && !inherited_resources { + if is_polymorphic(&kwargs) && !inherited_resources { return None; } - if let Some(found) = extract_class_name_from_kwargs(kwargs) { + if let Some(found) = extract_class_name_from_kwargs(&kwargs) { name = Some(found); } } } - if let Some(Node::Sym(d)) = first_arg { + if let Some(d) = first_arg.and_then(|arg| arg.as_symbol_node()) { if name.is_none() { // We singularize here because by convention Rails will singularize the class name as declared via a symbol, // e.g. `has_many :companies` will look for a class named `Company`, not `Companies` name = Some(to_class_case( - &d.name.to_string_lossy(), + &String::from_utf8_lossy(d.unescaped()), true, &HashSet::new(), // todo: pass in acronyms here )); @@ -181,7 +184,7 @@ pub fn get_reference_from_active_record_association( Some(UnresolvedReference { name: unwrapped_name, namespace_path: current_namespaces.to_owned(), - location: loc_to_range(&node.expression_l, line_col_lookup), + location: loc_to_range(&node.location(), line_col_lookup), }) } else { None @@ -191,13 +194,15 @@ pub fn get_reference_from_active_record_association( } } -fn extract_class_name_from_kwargs(kwargs: &nodes::Kwargs) -> Option { - for pair_node in kwargs.pairs.iter() { - if let Node::Pair(pair) = pair_node { - if let Node::Sym(k) = *pair.key.to_owned() { - if k.name.to_string_lossy() == *"class_name" { - if let Node::Str(v) = *pair.value.to_owned() { - return Some(v.value.to_string_lossy()); +fn extract_class_name_from_kwargs(kwargs: &KeywordHashNode) -> Option { + for pair_node in kwargs.elements().iter() { + if let Some(pair) = pair_node.as_assoc_node() { + if let Some(k) = pair.key().as_symbol_node() { + if k.unescaped() == b"class_name" { + if let Some(v) = pair.value().as_string_node() { + return Some( + String::from_utf8_lossy(v.unescaped()).into_owned(), + ); } } } @@ -207,11 +212,13 @@ fn extract_class_name_from_kwargs(kwargs: &nodes::Kwargs) -> Option { None } -fn is_polymorphic(kwargs: &nodes::Kwargs) -> bool { - for pair_node in kwargs.pairs.iter() { - if let Node::Pair(pair) = pair_node { - if let (Node::Sym(k), Node::True(_)) = (&*pair.key, &*pair.value) { - if k.name.to_string_lossy() == "polymorphic" { +fn is_polymorphic(kwargs: &KeywordHashNode) -> bool { + for pair_node in kwargs.elements().iter() { + if let Some(pair) = pair_node.as_assoc_node() { + if let (Some(k), Some(_)) = + (pair.key().as_symbol_node(), pair.value().as_true_node()) + { + if k.unescaped() == b"polymorphic" { return true; } } @@ -231,17 +238,14 @@ fn may_be_inherited_resources(current_namespaces: &[String]) -> bool { .is_none_or(|namespace| namespace.ends_with("Controller")) } +/// `name` is that of the constant assigned, as `FOO` or `Foo::BAR` pub fn get_constant_assignment_definition( - node: &nodes::Casgn, + name: Result, + location: &Location, current_namespaces: Vec, line_col_lookup: &LineColLookup, ) -> Option { - let name_result = fetch_casgn_name(node); - if name_result.is_err() { - return None; - } - - let name = name_result.unwrap(); + let name = name.ok()?; let fully_qualified_name = if !current_namespaces.is_empty() { let mut name_components = current_namespaces; name_components.push(name); @@ -252,6 +256,161 @@ pub fn get_constant_assignment_definition( Some(ParsedDefinition { fully_qualified_name, - location: loc_to_range(&node.expression_l, line_col_lookup), + location: loc_to_range(location, line_col_lookup), }) } + +/// `foo&.bar`, `foo[1]` and `foo[1] = 2` are calls in Prism's tree, but pks has +/// never counted them as calls. +pub fn is_plain_call(node: &CallNode) -> bool { + let index = matches!(node.name().as_slice(), b"[]" | b"[]=") + && node.call_operator_loc().is_none(); + !node.is_safe_navigation() && !index +} + +/// The `Visit` methods for the nodes that assign a constant, for an +/// `impl<'pr> Visit<'pr>` whose type has a `define_constant(name, location)` +/// method. They visit only the value assigned, so neither the constant nor its +/// scope is a reference. +macro_rules! visit_constant_assignments { + () => { + fn visit_constant_write_node( + &mut self, + node: &ruby_prism::ConstantWriteNode<'pr>, + ) { + self.define_constant( + Ok($crate::packs::parsing::ruby::parse_utils::constant_name( + &node.name(), + )), + &node.location(), + ); + self.visit(&node.value()); + } + + fn visit_constant_path_write_node( + &mut self, + node: &ruby_prism::ConstantPathWriteNode<'pr>, + ) { + let target = node.target(); + self.define_constant( + $crate::packs::parsing::ruby::parse_utils::fetch_const_path_name( + target.parent(), + target.name(), + ), + &node.location(), + ); + self.visit(&node.value()); + } + + fn visit_constant_or_write_node( + &mut self, + node: &ruby_prism::ConstantOrWriteNode<'pr>, + ) { + self.define_constant( + Ok($crate::packs::parsing::ruby::parse_utils::constant_name( + &node.name(), + )), + &node.name_loc(), + ); + self.visit(&node.value()); + } + + fn visit_constant_and_write_node( + &mut self, + node: &ruby_prism::ConstantAndWriteNode<'pr>, + ) { + self.define_constant( + Ok($crate::packs::parsing::ruby::parse_utils::constant_name( + &node.name(), + )), + &node.name_loc(), + ); + self.visit(&node.value()); + } + + fn visit_constant_operator_write_node( + &mut self, + node: &ruby_prism::ConstantOperatorWriteNode<'pr>, + ) { + self.define_constant( + Ok($crate::packs::parsing::ruby::parse_utils::constant_name( + &node.name(), + )), + &node.name_loc(), + ); + self.visit(&node.value()); + } + + fn visit_constant_path_or_write_node( + &mut self, + node: &ruby_prism::ConstantPathOrWriteNode<'pr>, + ) { + let target = node.target(); + self.define_constant( + $crate::packs::parsing::ruby::parse_utils::fetch_const_path_name( + target.parent(), + target.name(), + ), + &target.location(), + ); + self.visit(&node.value()); + } + + fn visit_constant_path_and_write_node( + &mut self, + node: &ruby_prism::ConstantPathAndWriteNode<'pr>, + ) { + let target = node.target(); + self.define_constant( + $crate::packs::parsing::ruby::parse_utils::fetch_const_path_name( + target.parent(), + target.name(), + ), + &target.location(), + ); + self.visit(&node.value()); + } + + fn visit_constant_path_operator_write_node( + &mut self, + node: &ruby_prism::ConstantPathOperatorWriteNode<'pr>, + ) { + let target = node.target(); + self.define_constant( + $crate::packs::parsing::ruby::parse_utils::fetch_const_path_name( + target.parent(), + target.name(), + ), + &target.location(), + ); + self.visit(&node.value()); + } + + // As in `A, B = 1, 2` or `rescue => A` + fn visit_constant_target_node( + &mut self, + node: &ruby_prism::ConstantTargetNode<'pr>, + ) { + self.define_constant( + Ok($crate::packs::parsing::ruby::parse_utils::constant_name( + &node.name(), + )), + &node.location(), + ); + } + + fn visit_constant_path_target_node( + &mut self, + node: &ruby_prism::ConstantPathTargetNode<'pr>, + ) { + self.define_constant( + $crate::packs::parsing::ruby::parse_utils::fetch_const_path_name( + node.parent(), + node.name(), + ), + &node.location(), + ); + } + }; +} +pub(crate) use visit_constant_assignments; diff --git a/tests/expose_monkey_patches_test.rs b/tests/expose_monkey_patches_test.rs index dbe9dce..39b6bd5 100644 --- a/tests/expose_monkey_patches_test.rs +++ b/tests/expose_monkey_patches_test.rs @@ -48,7 +48,8 @@ fn test_expose_monkey_patches_warns_only_about_the_apps_syntax_errors( .success() .stderr( "Warning: `app/services/broken.rb` has a syntax error at line 3 \ - (unexpected kEND), so pks may have missed references in it.\n", + (unexpected 'end'; expected a `)` to close the parameters), so pks \ + may have missed references in it.\n", ); Ok(()) diff --git a/tests/list_references_test.rs b/tests/list_references_test.rs index 1d3c61d..f80f78d 100644 --- a/tests/list_references_test.rs +++ b/tests/list_references_test.rs @@ -407,6 +407,38 @@ fn test_list_references_lists_each_use_of_a_constant_in_a_file( Ok(()) } +#[test] +fn test_list_references_in_anonymous_argument_forwarding( +) -> Result<(), Box> { + // Ruby 3.2's `*` and `**` forwarding + let file = "packs/foo/app/services/forwarder.rb"; + for args in [&[][..], &["--experimental-parser"]] { + let fixture = common::Fixture::new("simple_app"); + fs::write( + fixture.path(file), + "class Forwarder\n def a(*) = Bar.b(*)\n def c(**) = Bar.d(**)\n\ + end\n", + )?; + + assert_eq!( + list_references( + &fixture, + &[args, &["list-references", "--defining-pack", "packs/bar"]] + .concat() + ), + json!({ + "references": [ + bar_from_foo(), + to_bar_pack(file, Some((2, 13)), "::Bar"), + to_bar_pack(file, Some((3, 14)), "::Bar"), + ] + }) + ); + } + + Ok(()) +} + #[test] fn test_list_references_at_the_start_of_a_file() -> Result<(), Box> { let fixture = common::Fixture::new("simple_app"); diff --git a/tests/syntax_error_test.rs b/tests/syntax_error_test.rs index a7be7e1..8edc400 100644 --- a/tests/syntax_error_test.rs +++ b/tests/syntax_error_test.rs @@ -29,8 +29,8 @@ fn test_check_warns_about_a_ruby_file_with_a_syntax_error( ) -> Result<(), Box> { // simple_app's empty `packs/baz/app/services/baz.rb` makes no warning let warning = "Warning: `packs/foo/app/services/broken.rb` has a syntax \ - error at line 7 (unexpected kEND), so pks may have missed \ - references in it.\n"; + error at line 7 (unexpected 'end'; expected a `)` to close \ + the parameters), so pks may have missed references in it.\n"; for args in [&[][..], &["--experimental-parser"]] { let fixture = fixture_app()?; fs::write( @@ -40,6 +40,63 @@ fn test_check_warns_about_a_ruby_file_with_a_syntax_error( assert_eq!(check_stderr(&fixture, args)?, warning); assert_eq!(check_stderr(&fixture, args)?, warning); + + // The method before the error still parses, and its `Bar` is found + common::pks(&fixture, &[args, &["check"]].concat()).stdout( + predicates::str::contains("packs/foo/app/services/broken.rb:3:4\n"), + ); + } + + Ok(()) +} + +#[test] +fn test_check_warns_about_an_error_whose_message_isnt_utf_8( +) -> Result<(), Box> { + // Read as Windows-31J, the heredoc's name ends mid-character in Prism's + // message. The panic pks recovers from prints before the warning. + let warning = "Warning: `packs/foo/app/services/sjis.rb` has a syntax \ + error at line 2 (its message isn't UTF-8), so pks may have \ + missed references in it.\n"; + for args in [&[][..], &["--experimental-parser"]] { + let fixture = fixture_app()?; + fs::write( + fixture.path("packs/foo/app/services/sjis.rb"), + "# encoding: windows-31j\nx = <<~あ\n", + )?; + + // 1 for simple_app's violations, not 101 for a panic + let output = common::pks(&fixture, &[args, &["check"]].concat()) + .code(1) + .get_output() + .clone(); + assert!(String::from_utf8(output.stderr)?.ends_with(warning)); + } + + Ok(()) +} + +#[test] +fn test_check_allows_yield_in_templates() -> Result<(), Box> { + // Rails runs a template as the body of a method, so `yield` is no error + for args in [&[][..], &["--experimental-parser"]] { + let fixture = fixture_app()?; + let packwerk_yml = fixture.path("packwerk.yml"); + let contents = fs::read_to_string(&packwerk_yml)?; + fs::write( + &packwerk_yml, + format!("{}include:\n- \"**/*.{{rb,erb,builder}}\"\n", contents), + )?; + fs::write( + fixture.path("packs/foo/app/views/layout.erb"), + "

<%= yield %>

\n<%= yield(:footer) %>\n", + )?; + fs::write( + fixture.path("packs/foo/app/views/feed.xml.builder"), + "xml.feed do\n yield\nend\n", + )?; + + assert_eq!(check_stderr(&fixture, args)?, ""); } Ok(()) @@ -48,15 +105,16 @@ fn test_check_warns_about_a_ruby_file_with_a_syntax_error( #[test] fn test_check_warns_about_an_erb_file_with_a_syntax_error( ) -> Result<(), Box> { - // pks keeps no ERB positions, so the warning gives no line + // pks keeps no ERB positions, so the warning gives no line. The `yield` is + // no error, but what follows it is. let warning = "Warning: `packs/foo/app/views/broken.erb` has a syntax \ - error (unexpected END_OF_INPUT), so pks may have missed \ - references in it.\n"; + error (unexpected end-of-input; expected a `)` to close the \ + arguments), so pks may have missed references in it.\n"; for args in [&[][..], &["--experimental-parser"]] { let fixture = fixture_app()?; fs::write( fixture.path("packs/foo/app/views/broken.erb"), - "<%= Bar.bar( %>\n", + "<%= yield %>\n<%= Bar.bar( %>\n", )?; assert_eq!(check_stderr(&fixture, args)?, warning);