From e6b282d1aadc52331506edfececfa0f8d7567ec4 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Mon, 12 Oct 2026 14:17:06 -0700 Subject: [PATCH 01/10] Parse Ruby with Prism instead of lib-ruby-parser lib-ruby-parser follows Ruby 3.1's grammar, and its repository was archived in 2024, so syntax from later Rubies doesn't parse. Ruby 3.2's anonymous argument forwarding, `def foo(*) = bar(*)`, is a syntax error to it, and pks lost the references in such a file. Prism is the parser Ruby itself has used since 3.4, and the `ruby-prism` crate is published from ruby/prism. The two tree walkers collect what they did before. lib-ruby-parser gave `foo&.bar` and `foo[1]` node types of their own, while Prism makes them calls, and it made `a.b ||= c` a call where Prism doesn't. The walkers make up for each, so associations and the experimental parser's test for a class that changes behavior see the same calls as before, as unit tests now pin. Prism has ten node types for assigning a constant, which share one macro between the walkers and, as `casgn` did, visit only the value assigned. `module self::Foo` no longer panics in packwerk mode; it is skipped, as a class with such a name was. Both parsers give byte offsets, so lines and columns are unchanged. An association call with a block now ends after the block, but pks uses only where a range starts. Prism reports `yield` outside a method as an error, and the Ruby that pks makes from a template has its `yield` at the top level. Rails runs a template as the body of a method, so that error is ignored there. Otherwise the warning gives Prism's first error, in Prism's words. Its later errors can sit at earlier lines, as the missing `end` of an enclosing class does. On every fixture and three large Rails apps, under both parsers, every file gives the same references and definitions as before, and `check -o json`, `list-references` and `list-definitions` print the same. `check` takes as long and uses about 5% less memory. ruby-prism compiles Prism's C source and generates its bindings with bindgen, so building pks now needs a C compiler and libclang. --- AGENTS.md | 4 +- CHANGELOG.md | 20 ++ Cargo.toml | 2 +- INSTALLATION.md | 1 + src/packs/parsing/erb/experimental/parser.rs | 4 +- src/packs/parsing/erb/packwerk/parser.rs | 4 +- src/packs/parsing/mod.rs | 2 +- src/packs/parsing/ruby/experimental.rs | 37 +++ src/packs/parsing/ruby/experimental/parser.rs | 229 ++++++++----- src/packs/parsing/ruby/packwerk/parser.rs | 260 ++++++++------- src/packs/parsing/ruby/parse_utils.rs | 313 +++++++++++++----- tests/expose_monkey_patches_test.rs | 3 +- tests/list_references_test.rs | 32 ++ tests/syntax_error_test.rs | 24 +- 14 files changed, 637 insertions(+), 298 deletions(-) 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..5c08974 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,25 @@ # 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. 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. + ## 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..7206c1c 100644 --- a/INSTALLATION.md +++ b/INSTALLATION.md @@ -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, and the `clang` package on Linux. - `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/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..91b7e2c 100644 --- a/src/packs/parsing/ruby/experimental.rs +++ b/src/packs/parsing/ruby/experimental.rs @@ -349,6 +349,43 @@ 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 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..4793a2b 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,28 @@ pub(crate) fn process_from_contents( path: &Path, configuration: &Configuration, ) -> ProcessedFile { - let options = ParserOptions { - buffer_name: "".to_string(), - ..Default::default() - }; + process(contents, path, configuration, false) +} + +/// 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 +279,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/parser.rs b/src/packs/parsing/ruby/packwerk/parser.rs index 5184f42..5d088ca 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,28 @@ pub(crate) fn process_from_contents( path: &Path, configuration: &Configuration, ) -> ProcessedFile { - let options = ParserOptions { - buffer_name: "".to_string(), - ..Default::default() - }; + process(contents, path, configuration, false) +} + +/// 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 +278,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..e39c0e1 100644 --- a/src/packs/parsing/ruby/parse_utils.rs +++ b/src/packs/parsing/ruby/parse_utils.rs @@ -1,7 +1,9 @@ use std::collections::HashSet; -use lib_ruby_parser::{nodes, Diagnostic, Loc, Node}; use line_col::LineColLookup; +use ruby_prism::{ + CallNode, ConstantId, Diagnostics, KeywordHashNode, Location, Node, +}; use crate::packs::parsing::{ ParsedDefinition, Range, SyntaxError, UnresolvedReference, @@ -15,18 +17,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 +40,62 @@ 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() == "Invalid yield")); + error.map(|error| SyntaxError { + line: Some(lookup.get(error.location().start_offset()).0), + message: error.message().to_owned(), + }) } -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 +106,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 +119,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 +141,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 +174,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 +184,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 +202,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 +228,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 +246,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..62262c0 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( @@ -45,13 +45,29 @@ fn test_check_warns_about_a_ruby_file_with_a_syntax_error( Ok(()) } +#[test] +fn test_check_allows_yield_in_erb() -> 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()?; + fs::write( + fixture.path("packs/foo/app/views/layout.erb"), + "

<%= yield %>

\n<%= yield(:footer) %>\n", + )?; + + assert_eq!(check_stderr(&fixture, args)?, ""); + } + + Ok(()) +} + #[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 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( From daba8b9d1d7f7fe581166b124028f4572ca818e2 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Mon, 12 Oct 2026 14:17:13 -0700 Subject: [PATCH 02/10] Build every release target in CI dist builds the release binaries only when releasing, so a dependency that doesn't build for one target shows up only then. Prism is C, compiled with `cc` and bound with bindgen, so each runner needs a C compiler and libclang. This builds each target in dist-workspace.toml on the runner dist's plan uses, installing musl-tools for musl as dist does, so a pull request, Dependabot's included, shows whether pks still builds everywhere. --- .github/workflows/ci.yml | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6515afb..4e08eb9 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -61,3 +61,43 @@ 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-14 + - target: x86_64-apple-darwin + runner: macos-15-intel + - 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 + runs-on: ${{ matrix.runner }} + env: + TARGET: ${{ matrix.target }} + 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 target add "$TARGET" + cargo build --release --target "$TARGET" From 4f3e4be1326c5f587e6b0efabef6a565fef1bdb2 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Mon, 12 Oct 2026 14:57:48 -0700 Subject: [PATCH 03/10] Drop the Intel macOS target and build on a current macOS dist built the Apple Silicon binary on macos-14 by default, an image GitHub has deprecated, and Intel Macs no longer need supporting. Releases now leave out x86_64-apple-darwin, and dist builds aarch64-apple-darwin on macos-26. That is the newest arm64 macOS that GitHub's runner images call generally available, named explicitly rather than as macos-latest so that it changes only when we change it. The CI build job uses the same runner, so it keeps building in the release's environment. `dist generate` leaves release.yml as it was, since dist picks runners when it plans a release, and `dist plan` now lists macos-26 for that target and no Intel one. The DotSlash file and rust-toolchain.toml drop the Intel target too. --- .github/workflows/ci.yml | 4 +--- .github/workflows/dotslash-config.json | 5 ----- CHANGELOG.md | 5 +++++ dist-workspace.toml | 6 +++++- rust-toolchain.toml | 2 +- 5 files changed, 12 insertions(+), 10 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4e08eb9..5867ebf 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -71,9 +71,7 @@ jobs: matrix: include: - target: aarch64-apple-darwin - runner: macos-14 - - target: x86_64-apple-darwin - runner: macos-15-intel + runner: macos-26 - target: aarch64-unknown-linux-gnu runner: ubuntu-22.04-arm - target: x86_64-unknown-linux-gnu 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/CHANGELOG.md b/CHANGELOG.md index 5c08974..6894c7d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,11 @@ 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/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"] From 933ced7201e6575bde082e37749eba6331598f7d Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Mon, 12 Oct 2026 16:08:02 -0700 Subject: [PATCH 04/10] Test more of what the switch to Prism keeps and changes Prism has ten node types that assign a constant, which the walkers handle in one macro, but only `FOO = BAR` in packwerk mode was tested. Each is now, under both parsers: it defines its targets, and only the value assigned is a reference, not a target or its scope. Visiting one of them with Prism's default instead fails these tests. Packwerk mode gains tests that a class or module named by an expression, as `module self::Foo`, is skipped without upsetting the namespaces after it, and that a `&.` call is no association while a call with a block still starts where it did. A file with a syntax error now has to give the reference that Prism recovers before the error, and a template's error has to be reported when a `yield` comes before it. --- src/packs/parsing/ruby/experimental.rs | 30 +++++++++ src/packs/parsing/ruby/packwerk.rs | 84 ++++++++++++++++++++++++++ tests/syntax_error_test.rs | 10 ++- 3 files changed, 122 insertions(+), 2 deletions(-) diff --git a/src/packs/parsing/ruby/experimental.rs b/src/packs/parsing/ruby/experimental.rs index 91b7e2c..ca99f0f 100644 --- a/src/packs/parsing/ruby/experimental.rs +++ b/src/packs/parsing/ruby/experimental.rs @@ -386,6 +386,36 @@ end } } + #[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/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/tests/syntax_error_test.rs b/tests/syntax_error_test.rs index 62262c0..a93791a 100644 --- a/tests/syntax_error_test.rs +++ b/tests/syntax_error_test.rs @@ -40,6 +40,11 @@ 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(()) @@ -64,7 +69,8 @@ fn test_check_allows_yield_in_erb() -> Result<(), Box> { #[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; expected a `)` to close the \ arguments), so pks may have missed references in it.\n"; @@ -72,7 +78,7 @@ fn test_check_warns_about_an_erb_file_with_a_syntax_error( 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); From 196b430816bbd12d34a0d7a94f923ca16ee82587 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Mon, 12 Oct 2026 16:08:56 -0700 Subject: [PATCH 05/10] Don't warn about `yield` in a Builder template Rails runs a Builder template as the body of a method, as it does an ERB one, so `yield` is valid in a Builder layout. pks parses `.builder` files as Ruby, though, and Prism reports a `yield` outside a method as an error, so an app that includes them got a warning that lib-ruby-parser never gave. Such a file is now parsed as a template. --- src/packs/parsing/ruby/experimental/parser.rs | 4 +++- src/packs/parsing/ruby/packwerk/parser.rs | 4 +++- tests/syntax_error_test.rs | 12 +++++++++++- 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/src/packs/parsing/ruby/experimental/parser.rs b/src/packs/parsing/ruby/experimental/parser.rs index 4793a2b..5ab0be4 100644 --- a/src/packs/parsing/ruby/experimental/parser.rs +++ b/src/packs/parsing/ruby/experimental/parser.rs @@ -247,7 +247,9 @@ pub(crate) fn process_from_contents( path: &Path, configuration: &Configuration, ) -> ProcessedFile { - process(contents, path, configuration, false) + // 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 diff --git a/src/packs/parsing/ruby/packwerk/parser.rs b/src/packs/parsing/ruby/packwerk/parser.rs index 5d088ca..4779339 100644 --- a/src/packs/parsing/ruby/packwerk/parser.rs +++ b/src/packs/parsing/ruby/packwerk/parser.rs @@ -245,7 +245,9 @@ pub(crate) fn process_from_contents( path: &Path, configuration: &Configuration, ) -> ProcessedFile { - process(contents, path, configuration, false) + // 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 diff --git a/tests/syntax_error_test.rs b/tests/syntax_error_test.rs index a93791a..db1b655 100644 --- a/tests/syntax_error_test.rs +++ b/tests/syntax_error_test.rs @@ -51,14 +51,24 @@ fn test_check_warns_about_a_ruby_file_with_a_syntax_error( } #[test] -fn test_check_allows_yield_in_erb() -> Result<(), Box> { +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)?, ""); } From fe712ebdb77587e9699daa27ef69d3a1958f125b Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Mon, 12 Oct 2026 16:12:35 -0700 Subject: [PATCH 06/10] Don't panic on a Prism message that isn't UTF-8 Some of Prism's messages quote the file, as an unterminated heredoc's quotes its name. When a magic comment names an encoding other than UTF-8, such as Windows-31J, Prism reads the file in it, and the quote can end partway through a UTF-8 character. ruby-prism's `Diagnostic::message` then panics, which stopped the whole run without saying which file it was in. lib-ruby-parser warned that it didn't support the encoding and went on. The crate offers no other way to read a message, so pks catches that panic and warns without Prism's message. Rust's panic handler still prints the panic before the warning. --- src/packs/parsing/ruby/parse_utils.rs | 18 ++++++++++++++---- tests/syntax_error_test.rs | 26 ++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 4 deletions(-) diff --git a/src/packs/parsing/ruby/parse_utils.rs b/src/packs/parsing/ruby/parse_utils.rs index e39c0e1..155c630 100644 --- a/src/packs/parsing/ruby/parse_utils.rs +++ b/src/packs/parsing/ruby/parse_utils.rs @@ -2,7 +2,8 @@ use std::collections::HashSet; use line_col::LineColLookup; use ruby_prism::{ - CallNode, ConstantId, Diagnostics, KeywordHashNode, Location, Node, + CallNode, ConstantId, Diagnostic, Diagnostics, KeywordHashNode, Location, + Node, }; use crate::packs::parsing::{ @@ -48,14 +49,23 @@ pub fn first_syntax_error( lookup: &LineColLookup, template: bool, ) -> Option { - let error = - errors.find(|error| !(template && error.message() == "Invalid yield")); + 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().to_owned(), + 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: &Location, lookup: &LineColLookup) -> Range { let (start_row, start_col) = lookup.get(loc.start_offset()); let (end_row, end_col) = lookup.get(loc.end_offset()); diff --git a/tests/syntax_error_test.rs b/tests/syntax_error_test.rs index db1b655..8edc400 100644 --- a/tests/syntax_error_test.rs +++ b/tests/syntax_error_test.rs @@ -50,6 +50,32 @@ fn test_check_warns_about_a_ruby_file_with_a_syntax_error( 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 From fa29cb09452c171a22bbfdd5ee3ce36f9c9dc645 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Mon, 12 Oct 2026 16:13:03 -0700 Subject: [PATCH 07/10] Build in CI as dist does, and run what's built dist builds with the dist profile, and on Windows links the C runtime statically, so cc compiles Prism with /MT. The build job now does both, so it compiles what a release ships. It also runs each binary on a fixture, since otherwise only Test Suite runs Prism, on x86_64 Linux, and a target could build a pks that crashes. Installing the toolchain before adding the target stops rustup's warning that installing it automatically is deprecated. --- .github/workflows/ci.yml | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5867ebf..f636dd3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -80,9 +80,12 @@ jobs: 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 @@ -97,5 +100,13 @@ jobs: - name: Run cargo build shell: bash run: | + rustup toolchain install rustup target add "$TARGET" - cargo build --release --target "$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"' From 9a7187f1acc8a70df37f3d9d5c5eeab85cb23926 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Mon, 12 Oct 2026 16:13:12 -0700 Subject: [PATCH 08/10] Say how to get libclang on Windows, and what Prism's grammar changes A Windows build needs LLVM for its libclang, which Visual Studio's build tools don't install, and bindgen's requirements cover other systems. Prism reads Ruby as its newest version does, so pks now warns about syntax that only older Rubies accept, such as `a[k: 1] = v`, and with `--experimental-parser` no longer counts `it` in a block as a method call. The changelog's claim that the references and definitions pks finds are the same now says so. --- CHANGELOG.md | 12 +++++++++--- INSTALLATION.md | 2 +- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6894c7d..06bf76a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,9 +14,15 @@ 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. 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. +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. diff --git a/INSTALLATION.md b/INSTALLATION.md index 7206c1c..9cb8331 100644 --- a/INSTALLATION.md +++ b/INSTALLATION.md @@ -29,7 +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, and the `clang` package on Linux. +- 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, and the `clang` package on Debian and Ubuntu. 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. From c2eba1689a384e5819e1f512455178beb98db170 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Mon, 12 Oct 2026 16:19:51 -0700 Subject: [PATCH 09/10] Say the macOS binaries are for Apple Silicon only The Intel macOS target is gone from releases and the DotSlash file, but the installation guide still said both cover macOS. --- INSTALLATION.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/INSTALLATION.md b/INSTALLATION.md index 9cb8331..0fec2bc 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: From 5ebbfba352aca6809ed95140006ba5ed547b15a3 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Mon, 12 Oct 2026 16:37:46 -0700 Subject: [PATCH 10/10] Give the builds one check to require, and say what Debian needs Main's ruleset requires Lints, Test Suite and Check, so with auto-merge a pull request that breaks a release target can merge, and nobody learns of it until a release fails. The matrix's job names change with its targets, so a new Build job reports whether every target built, giving the ruleset one name to require. GitHub counts a skipped required check as passing, so it runs even when a build fails, and fails then. The installation guide said the `clang` package gives Debian and Ubuntu libclang, which no build had tried. bindgen's requirements name `libclang-dev`, and `build-essential` gives the C compiler that Rust needs there anyway. --- .github/workflows/ci.yml | 13 +++++++++++++ INSTALLATION.md | 2 +- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f636dd3..a3010be 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -110,3 +110,16 @@ jobs: 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/INSTALLATION.md b/INSTALLATION.md index 0fec2bc..df5adc4 100644 --- a/INSTALLATION.md +++ b/INSTALLATION.md @@ -29,7 +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, and the `clang` package on Debian and Ubuntu. 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). +- 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.