diff --git a/.github/dependabot.yml b/.github/dependabot.yml new file mode 100644 index 0000000..82377e9 --- /dev/null +++ b/.github/dependabot.yml @@ -0,0 +1,28 @@ +version: 2 +updates: + - package-ecosystem: "cargo" + directory: "/" + schedule: + interval: "monthly" + # Cargo.lock isn't tracked, so raise the minimum versions in Cargo.toml + # even when they already allow the new release + versioning-strategy: "increase" + groups: + cargo: + patterns: + - "*" + cooldown: + default-days: 7 + - package-ecosystem: "github-actions" + directory: "/" + # dist generates release.yml and pins its actions in dist-workspace.toml + exclude-paths: + - ".github/workflows/release.yml" + schedule: + interval: "monthly" + groups: + github-actions: + patterns: + - "*" + cooldown: + default-days: 7 diff --git a/CHANGELOG.md b/CHANGELOG.md index 9ddb384..aac1686 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,6 @@ # Changelog -## Unreleased +## 0.7.0 ### Features @@ -16,12 +16,40 @@ files alone can't. Unlike `check`, it exits 0 when references have violations. `--defining-pack` and `--referencing-pack` limit the list to references to or from the packs they name, and each can be given more than once. Given both, a -reference must match both. +reference must match both. With `--referencing-pack`, pks reads only those +packs' files, so it takes less time and memory in a large app. pks keeps no source positions in ERB, so a reference in an ERB file has a null `line` and `column`, and the file's references to one constant are listed once. `schema/list-references-output.json` describes the output. +### Fixes + +#### pks warns about files it can't parse + +A file that pks couldn't parse gave it some or none of its references, and +nothing said so, so `pks check` could pass with violations in that file. pks +now warns on stderr, naming the file and its first syntax error, with the line +in a Ruby file. A file with a syntax error isn't cached, so the warning comes +back on every run until the file is fixed. + +pks's parser can't read some newer Ruby syntax, such as Ruby 3.2's anonymous +argument forwarding (`def foo(*) = bar(*)`), so files that use it get this +warning too. + +#### References after a `<%==` tag are no longer missed + +pks read Rails' `<%== %>` tag, which outputs without escaping, as Ruby that +starts with `=`. That doesn't parse, so in an ERB file with one, the tag's +references and every reference after it were missed. `<%==` is now read like +`<%=`, so `pks check` can report violations it missed before. + +#### `--print-files` no longer breaks JSON output + +`-p` printed its "Started processing" and "Finished processing" lines to +stdout, ahead of the JSON from `check -o json` and `list-references`, so the +output didn't parse. They now go to stderr. + ## 0.6.0 ### Features diff --git a/Cargo.toml b/Cargo.toml index 6a4fd7a..892c495 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -2,7 +2,7 @@ [package] name = "pks" -version = "0.6.0" +version = "0.7.0" edition = "2021" description = "Welcome! Please see https://github.com/rubyatscale/pks for more information!" license = "MIT" diff --git a/src/packs.rs b/src/packs.rs index c0061bd..3047639 100644 --- a/src/packs.rs +++ b/src/packs.rs @@ -42,6 +42,7 @@ pub(crate) use self::parsing::process_files_with_cache; pub(crate) use self::parsing::ruby::experimental::get_experimental_constant_resolver; pub(crate) use self::parsing::ruby::zeitwerk::get_zeitwerk_constant_resolver; pub(crate) use self::parsing::ParsedDefinition; +pub(crate) use self::parsing::SyntaxError; pub(crate) use self::parsing::UnresolvedReference; use anyhow::bail; use cli::ColorChoice; @@ -276,6 +277,9 @@ pub struct ProcessedFile { pub absolute_path: PathBuf, pub unresolved_references: Vec, pub definitions: Vec, + /// Never cached, since a file with a syntax error isn't + #[serde(skip)] + pub syntax_error: Option, } #[derive( diff --git a/src/packs/caching/per_file_cache.rs b/src/packs/caching/per_file_cache.rs index 4621c8e..e8b7594 100644 --- a/src/packs/caching/per_file_cache.rs +++ b/src/packs/caching/per_file_cache.rs @@ -252,6 +252,7 @@ mod tests { } }], definitions: vec![], + syntax_error: None, } }; @@ -306,6 +307,7 @@ mod tests { absolute_path: path.clone(), unresolved_references: vec![], definitions: vec![], + syntax_error: None, }; let CacheResult::Miss(empty_cache_entry) = cache.get(&path)? else { diff --git a/src/packs/file_utils.rs b/src/packs/file_utils.rs index 929ddb6..f22889a 100644 --- a/src/packs/file_utils.rs +++ b/src/packs/file_utils.rs @@ -104,7 +104,8 @@ pub fn user_inputted_paths_to_absolute_filepaths( pub(crate) fn convert_erb_to_ruby_without_sourcemaps( contents: String, ) -> String { - let regex_pattern = r"(?s)<%=?-?\s*(.*?)\s*-?%>"; + // `<%==` is Rails' tag for output that isn't escaped + let regex_pattern = r"(?s)<%(?:==?)?-?\s*(.*?)\s*-?%>"; let regex = Regex::new(regex_pattern).unwrap(); let extracted_contents: Vec<&str> = regex diff --git a/src/packs/parsing/erb/experimental/parser.rs b/src/packs/parsing/erb/experimental/parser.rs index 62f9bd1..c728c90 100644 --- a/src/packs/parsing/erb/experimental/parser.rs +++ b/src/packs/parsing/erb/experimental/parser.rs @@ -1,6 +1,7 @@ use crate::packs::file_utils::file_read_contents; use crate::packs::{ - file_utils::convert_erb_to_ruby_without_sourcemaps, parsing::Range, + file_utils::convert_erb_to_ruby_without_sourcemaps, + parsing::{Range, SyntaxError}, Configuration, ProcessedFile, UnresolvedReference, }; use std::path::Path; @@ -40,5 +41,9 @@ pub(crate) fn process_from_contents( absolute_path: path.to_path_buf(), unresolved_references: references_without_range, definitions: vec![], + syntax_error: processed_file.syntax_error.map(|error| SyntaxError { + line: None, + ..error + }), } } diff --git a/src/packs/parsing/erb/packwerk.rs b/src/packs/parsing/erb/packwerk.rs index ee5c8bb..b936606 100644 --- a/src/packs/parsing/erb/packwerk.rs +++ b/src/packs/parsing/erb/packwerk.rs @@ -327,6 +327,39 @@ mod tests { ); } + #[test] + fn raw_output_tag_is_ruby() { + // Read as `= Bar`, the tag hid its own references and every one after + let contents: String = + String::from("<%= Foo %>\n<%== Bar %>\n<%= Baz %>\n"); + let configuration = Configuration::default(); + assert_eq!( + vec![ + UnresolvedReference { + name: String::from("Foo"), + namespace_path: vec![], + location: Range::default() + }, + UnresolvedReference { + name: String::from("Bar"), + namespace_path: vec![], + location: Range::default() + }, + UnresolvedReference { + name: String::from("Baz"), + namespace_path: vec![], + location: Range::default() + } + ], + process_from_contents( + contents, + &PathBuf::from("path/to/file.rb"), + &configuration + ) + .unresolved_references + ); + } + #[test] fn literal_tag_is_text_not_ruby() { let contents: String = String::from( diff --git a/src/packs/parsing/erb/packwerk/parser.rs b/src/packs/parsing/erb/packwerk/parser.rs index 04d32c0..7236382 100644 --- a/src/packs/parsing/erb/packwerk/parser.rs +++ b/src/packs/parsing/erb/packwerk/parser.rs @@ -1,6 +1,7 @@ use crate::packs::file_utils::file_read_contents; use crate::packs::{ - file_utils::convert_erb_to_ruby_without_sourcemaps, parsing::Range, + file_utils::convert_erb_to_ruby_without_sourcemaps, + parsing::{Range, SyntaxError}, Configuration, ProcessedFile, UnresolvedReference, }; use std::path::Path; @@ -40,5 +41,9 @@ pub(crate) fn process_from_contents( absolute_path: path.to_path_buf(), unresolved_references: references_without_range, definitions: vec![], + syntax_error: processed_file.syntax_error.map(|error| SyntaxError { + line: None, + ..error + }), } } diff --git a/src/packs/parsing/mod.rs b/src/packs/parsing/mod.rs index d711a92..b682392 100644 --- a/src/packs/parsing/mod.rs +++ b/src/packs/parsing/mod.rs @@ -25,7 +25,7 @@ pub fn process_file( configuration: &Configuration, ) -> anyhow::Result { if configuration.print_files { - println!("Started processing {}", path.display()); + eprintln!("Started processing {}", path.display()); } let file_type_option = get_file_type(path); @@ -53,16 +53,61 @@ pub fn process_file( absolute_path: path.to_path_buf(), unresolved_references: vec![], definitions: vec![], // TODO + syntax_error: None, }) }; if configuration.print_files { - println!("Finished processing {}", path.display()); + eprintln!("Finished processing {}", path.display()); } result } +/// The file's syntax error, if it is one of the app's own. The Ruby and gem +/// sources that `expose-monkey-patches` also reads would warn about syntax that +/// pks can't parse, which isn't the app's to fix. +fn syntax_error_to_warn_about<'a>( + processed_file: &'a ProcessedFile, + configuration: &Configuration, +) -> Option<&'a SyntaxError> { + processed_file.syntax_error.as_ref().filter(|_| { + configuration + .included_files + .contains(&processed_file.absolute_path) + }) +} + +fn warn_about_syntax_error( + path: &Path, + error: &SyntaxError, + configuration: &Configuration, +) { + let file = path + .strip_prefix(&configuration.absolute_root) + .unwrap_or(path); + let line = error + .line + .map(|line| format!(" at line {}", line)) + .unwrap_or_default(); + eprintln!( + "Warning: `{}` has a syntax error{} ({}), so pks may have missed \ + references in it.", + file.display(), + line, + error.message + ); +} + +/// The first error lib-ruby-parser reports in a file. What it can't parse, pks +/// can't find references in. +#[derive(Debug, PartialEq, Eq, Clone)] +pub struct SyntaxError { + /// None in ERB, whose lines are those of the Ruby that pks makes from it + pub line: Option, + pub message: String, +} + #[derive(Debug, PartialEq, Eq, Serialize, Deserialize, Clone)] pub struct UnresolvedReference { pub name: String, @@ -92,21 +137,34 @@ pub fn process_files_with_cache( paths .par_iter() .map(|absolute_path| -> anyhow::Result { - if is_stdin_file(absolute_path, configuration) { - process_file(absolute_path, configuration) + let processed_file = if is_stdin_file(absolute_path, configuration) + { + process_file(absolute_path, configuration)? } else { match cache.get(absolute_path)? { - CacheResult::Processed(processed_file) => { - Ok(processed_file) - } + CacheResult::Processed(processed_file) => processed_file, CacheResult::Miss(empty_cache_entry) => { let processed_file = process_file(absolute_path, configuration)?; - cache.write(&empty_cache_entry, &processed_file)?; - Ok(processed_file) + // So the warning comes back until the file is fixed + if syntax_error_to_warn_about( + &processed_file, + configuration, + ) + .is_none() + { + cache.write(&empty_cache_entry, &processed_file)?; + } + processed_file } } + }; + if let Some(error) = + syntax_error_to_warn_about(&processed_file, configuration) + { + warn_about_syntax_error(absolute_path, error, configuration); } + Ok(processed_file) }) .collect() } diff --git a/src/packs/parsing/ruby/experimental.rs b/src/packs/parsing/ruby/experimental.rs index a7bdf61..d7297ce 100644 --- a/src/packs/parsing/ruby/experimental.rs +++ b/src/packs/parsing/ruby/experimental.rs @@ -79,6 +79,7 @@ mod tests { absolute_path, unresolved_references, definitions, + syntax_error: None, }; assert_eq!(expected, actual); } @@ -108,6 +109,7 @@ mod tests { absolute_path, unresolved_references, definitions, + syntax_error: None, }; assert_eq!(expected, actual); } @@ -138,6 +140,7 @@ mod tests { absolute_path, unresolved_references, definitions, + syntax_error: None, }; assert_eq!(expected, actual); } @@ -168,6 +171,7 @@ mod tests { absolute_path, unresolved_references, definitions, + syntax_error: None, }; assert_eq!(expected, actual); } @@ -194,6 +198,7 @@ end absolute_path, unresolved_references, definitions, + syntax_error: None, }; assert_eq!(expected, actual); } @@ -230,6 +235,7 @@ end absolute_path, unresolved_references, definitions, + syntax_error: None, }; assert_eq!(expected, actual); } @@ -266,6 +272,7 @@ end absolute_path, unresolved_references, definitions, + syntax_error: None, }; assert_eq!(expected, actual); } @@ -301,6 +308,7 @@ end absolute_path, unresolved_references, definitions, + syntax_error: None, }; assert_eq!(expected, actual); } @@ -336,6 +344,7 @@ end absolute_path, unresolved_references, definitions, + syntax_error: None, }; assert_eq!(expected, actual); } @@ -375,6 +384,7 @@ end absolute_path, unresolved_references, definitions, + syntax_error: None, }; assert_eq!(expected, actual); } @@ -415,6 +425,7 @@ end absolute_path, unresolved_references, definitions, + syntax_error: None, }; assert_eq!(expected, actual); @@ -456,6 +467,7 @@ end absolute_path, unresolved_references, definitions, + syntax_error: None, }; assert_eq!(expected, actual); diff --git a/src/packs/parsing/ruby/experimental/parser.rs b/src/packs/parsing/ruby/experimental/parser.rs index a72d4e6..997cce0 100644 --- a/src/packs/parsing/ruby/experimental/parser.rs +++ b/src/packs/parsing/ruby/experimental/parser.rs @@ -3,8 +3,9 @@ use crate::packs::{ parsing::{ ruby::parse_utils::{ fetch_const_const_name, fetch_const_name, fetch_node_location, - get_constant_assignment_definition, get_definition_from, - get_reference_from_active_record_association, loc_to_range, + first_syntax_error, get_constant_assignment_definition, + get_definition_from, get_reference_from_active_record_association, + loc_to_range, }, ParsedDefinition, UnresolvedReference, }, @@ -200,6 +201,7 @@ pub(crate) fn process_from_contents( 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; @@ -210,6 +212,7 @@ pub(crate) fn process_from_contents( absolute_path: path.to_owned(), unresolved_references: vec![], definitions: vec![], + syntax_error, } } }; @@ -238,5 +241,6 @@ pub(crate) fn process_from_contents( absolute_path, unresolved_references, definitions, + syntax_error, } } diff --git a/src/packs/parsing/ruby/packwerk/parser.rs b/src/packs/parsing/ruby/packwerk/parser.rs index 8f50294..5184f42 100644 --- a/src/packs/parsing/ruby/packwerk/parser.rs +++ b/src/packs/parsing/ruby/packwerk/parser.rs @@ -5,7 +5,8 @@ use crate::packs::{ namespace_calculator::possible_fully_qualified_constants, parse_utils::{ fetch_const_const_name, fetch_const_name, fetch_node_location, - get_constant_assignment_definition, get_definition_from, + first_syntax_error, get_constant_assignment_definition, + get_definition_from, get_reference_from_active_record_association, loc_to_range, }, }, @@ -221,6 +222,7 @@ pub(crate) fn process_from_contents( 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; @@ -231,6 +233,7 @@ pub(crate) fn process_from_contents( absolute_path: path.to_owned(), unresolved_references: vec![], definitions: vec![], + syntax_error, } } }; @@ -311,5 +314,6 @@ pub(crate) fn process_from_contents( absolute_path, unresolved_references, definitions, + syntax_error, } } diff --git a/src/packs/parsing/ruby/parse_utils.rs b/src/packs/parsing/ruby/parse_utils.rs index 9a1b130..2a0b2c1 100644 --- a/src/packs/parsing/ruby/parse_utils.rs +++ b/src/packs/parsing/ruby/parse_utils.rs @@ -1,9 +1,11 @@ use std::collections::HashSet; -use lib_ruby_parser::{nodes, Loc, Node}; +use lib_ruby_parser::{nodes, Diagnostic, Loc, Node}; use line_col::LineColLookup; -use crate::packs::parsing::{ParsedDefinition, Range, UnresolvedReference}; +use crate::packs::parsing::{ + ParsedDefinition, Range, SyntaxError, UnresolvedReference, +}; use super::inflector_shim::to_class_case; @@ -48,6 +50,19 @@ pub fn get_definition_from( } } +pub fn first_syntax_error( + diagnostics: &[Diagnostic], + lookup: &LineColLookup, +) -> Option { + diagnostics + .iter() + .find(|diagnostic| diagnostic.is_error()) + .map(|diagnostic| SyntaxError { + line: Some(lookup.get(diagnostic.loc.begin).0), + message: diagnostic.render_message(), + }) +} + 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); diff --git a/src/packs/reference_extractor.rs b/src/packs/reference_extractor.rs index de969d3..706178e 100644 --- a/src/packs/reference_extractor.rs +++ b/src/packs/reference_extractor.rs @@ -1,6 +1,8 @@ use std::{collections::HashSet, path::PathBuf}; -use rayon::prelude::{IntoParallelRefIterator, ParallelIterator}; +use rayon::prelude::{ + IntoParallelRefIterator, ParallelIterator, ParallelSliceMut, +}; use tracing::debug; use crate::packs::{ @@ -87,3 +89,49 @@ pub(crate) fn get_all_references( references } + +/// The references from one pack to a constant that another pack defines, +/// limited to the given packs and sorted by `identity`, which puts the copies +/// of a reference, one for each file that defines its constant, next to each +/// other. +pub(crate) fn between_packs<'a>( + references: &'a [Reference], + defining_packs: &HashSet<&str>, + referencing_packs: &HashSet<&str>, +) -> Vec<&'a Reference> { + let mut references: Vec<&Reference> = references + .iter() + .filter(|reference| { + let Some(defining) = reference.defining_pack_name.as_deref() else { + return false; // A constant no pack defines, such as a gem's + }; + let referencing = reference.referencing_pack_name.as_str(); + defining != referencing + && selected(defining_packs, defining) + && selected(referencing_packs, referencing) + }) + .collect(); + // The copies give the same output in any order, so an unstable sort will do + references.par_sort_unstable_by(|a, b| identity(a).cmp(&identity(b))); + references +} + +/// No packs given means every pack. +fn selected(packs: &HashSet<&str>, pack: &str) -> bool { + packs.is_empty() || packs.contains(pack) +} + +/// What sets a reference apart from the others. The copies of a reference made +/// for each file that defines its constant share it. So do all of an ERB file's +/// references to one constant, since they are all at line 0. +pub(crate) fn identity( + reference: &Reference, +) -> (&str, usize, usize, &str, Option<&str>) { + ( + &reference.relative_referencing_file, + reference.source_location.line, + reference.source_location.column, + &reference.constant_name, + reference.defining_pack_name.as_deref(), + ) +} diff --git a/src/packs/reference_lister.rs b/src/packs/reference_lister.rs index 8027e38..1c9726c 100644 --- a/src/packs/reference_lister.rs +++ b/src/packs/reference_lister.rs @@ -6,15 +6,16 @@ use std::{ collections::HashSet, io::{BufWriter, Write}, + path::PathBuf, }; -use rayon::prelude::{ParallelIterator, ParallelSlice, ParallelSliceMut}; +use rayon::prelude::{ParallelIterator, ParallelSlice}; use serde::Serialize; use super::{ - checker::{get_checkers, reference::Reference}, + checker::get_checkers, checker_configuration::CheckerType, - reference_extractor::get_all_references, + reference_extractor::{between_packs, get_all_references, identity}, Configuration, }; @@ -49,24 +50,17 @@ pub(crate) fn list( let defining_packs = pack_names(configuration, defining_packs)?; let referencing_packs = pack_names(configuration, referencing_packs)?; - let all_references = - get_all_references(configuration, &configuration.included_files)?; - let mut references: Vec<&Reference> = all_references - .iter() - .filter(|reference| { - let Some(defining) = reference.defining_pack_name.as_deref() else { - return false; // A constant no pack defines, such as a gem's - }; - let referencing = reference.referencing_pack_name.as_str(); - defining != referencing - && selected(&defining_packs, defining) - && selected(&referencing_packs, referencing) - }) - .collect(); - // The order the schema promises. It also puts the copies of a reference - // next to each other, and as they give the same output in any order, an - // unstable sort will do. - references.par_sort_unstable_by(|a, b| identity(a).cmp(&identity(b))); + // Only the referencing packs' own files can reference from them + let owned_files; + let files = if referencing_packs.is_empty() { + &configuration.included_files + } else { + owned_files = files_of(configuration, &referencing_packs)?; + &owned_files + }; + let all_references = get_all_references(configuration, files)?; + let references = + between_packs(&all_references, &defining_packs, &referencing_packs); let checkers = get_checkers(configuration); let recorded_violations = &configuration.pack_set.all_violations; @@ -130,20 +124,17 @@ fn pack_names<'a>( .collect() } -/// No packs given means every pack. -fn selected(packs: &HashSet<&str>, pack: &str) -> bool { - packs.is_empty() || packs.contains(pack) -} - -/// What sets a reference apart from the others. The copies of a reference made -/// for each file that defines its constant share it. So do all of an ERB file's -/// references to one constant, since they are all at line 0. -fn identity(reference: &Reference) -> (&str, usize, usize, &str, Option<&str>) { - ( - &reference.relative_referencing_file, - reference.source_location.line, - reference.source_location.column, - &reference.constant_name, - reference.defining_pack_name.as_deref(), - ) +/// The included files that the packs own +fn files_of( + configuration: &Configuration, + packs: &HashSet<&str>, +) -> anyhow::Result> { + let mut files = HashSet::new(); + for file in &configuration.included_files { + let owner = configuration.pack_set.for_file(file)?; + if owner.is_some_and(|pack| packs.contains(pack.name.as_str())) { + files.insert(file.clone()); + } + } + Ok(files) } diff --git a/src/packs/remover.rs b/src/packs/remover.rs index 2a68697..8dd0c35 100644 --- a/src/packs/remover.rs +++ b/src/packs/remover.rs @@ -7,7 +7,7 @@ use super::{ get_zeitwerk_constant_resolver, pack::{write_pack_to_disk, Pack}, package_todo, - reference_extractor::get_all_references, + reference_extractor::{between_packs, get_all_references, identity}, Configuration, }; @@ -65,7 +65,9 @@ fn refuse_if_in_use( configuration: &Configuration, pack: &Pack, ) -> anyhow::Result<()> { - let references = references_from_other_packs(configuration, pack)?; + let all_references = + get_all_references(configuration, &configuration.included_files)?; + let references = references_from_other_packs(&all_references, pack); for reference in &references { println!( "{}:{}:{} references {}", @@ -171,34 +173,16 @@ fn nested_packs(configuration: &Configuration, pack: &Pack) -> Vec { nested } -fn references_from_other_packs( - configuration: &Configuration, +fn references_from_other_packs<'a>( + references: &'a [Reference], pack: &Pack, -) -> anyhow::Result> { - let mut references: Vec = - get_all_references(configuration, &configuration.included_files)? - .into_iter() - .filter(|reference| { - reference.referencing_pack_name != pack.name - && reference.defining_pack_name.as_deref() - == Some(pack.name.as_str()) - }) - .collect(); - - references.sort_by(|a, b| location(a).cmp(&location(b))); - // A constant defined in several of the pack's files yields one per file. - references.dedup_by(|a, b| location(a) == location(b)); - - Ok(references) -} - -fn location(reference: &Reference) -> (&str, usize, usize, &str) { - ( - &reference.relative_referencing_file, - reference.source_location.line, - reference.source_location.column, - &reference.constant_name, - ) +) -> Vec<&'a Reference> { + let defining_packs = HashSet::from([pack.name.as_str()]); + between_packs(references, &defining_packs, &HashSet::new()) + // A constant defined in several of the pack's files yields one per file + .chunk_by(|a, b| identity(a) == identity(b)) + .map(|copies| copies[0]) + .collect() } /// Serializes the whole pack, as `add-dependency` does, so the file loses its diff --git a/tests/check_test.rs b/tests/check_test.rs index 5416d5e..33fa4c9 100644 --- a/tests/check_test.rs +++ b/tests/check_test.rs @@ -1,24 +1,11 @@ use assert_cmd::cargo::cargo_bin_cmd; -use jsonschema::Validator; use predicates::prelude::*; use std::{error::Error, fs}; mod common; fn validate_check_output_schema(json_value: &serde_json::Value) { - let schema_str = fs::read_to_string("schema/check-output.json") - .expect("Failed to read schema file"); - let schema: serde_json::Value = - serde_json::from_str(&schema_str).expect("Schema should be valid JSON"); - let validator = - Validator::new(&schema).expect("Schema should be valid JSON Schema"); - if !validator.is_valid(json_value) { - let errors: Vec = validator - .iter_errors(json_value) - .map(|e| format!(" - {}", e)) - .collect(); - panic!("JSON output does not match schema:\n{}", errors.join("\n")); - } + common::validate_schema("schema/check-output.json", json_value); } pub fn stripped_output(output: Vec) -> String { @@ -70,6 +57,8 @@ fn test_check_with_privacy_dependency_error_template_overrides( #[test] fn test_check_with_private_constants() -> Result<(), Box> { let output = cargo_bin_cmd!("pks") + // `RUST_LOG` makes pks log to stderr, which this test expects empty + .env_remove("RUST_LOG") .arg("--project-root") .arg("tests/fixtures/privacy_violations_with_private_constants") .arg("check") @@ -735,6 +724,8 @@ fn test_check_contents_does_not_warn_about_existing_file_that_is_not_included( ) -> Result<(), Box> { // package.yml exists but is not in the include globs. cargo_bin_cmd!("pks") + // `RUST_LOG` makes pks log to stderr, which this test expects empty + .env_remove("RUST_LOG") .arg("--project-root") .arg("tests/fixtures/uses_strict_mode") .arg("check-contents") @@ -753,6 +744,8 @@ fn test_check_does_not_warn_about_existing_files_that_are_not_included( ) -> Result<(), Box> { // What a hook passing every staged file sends alongside the Ruby files. cargo_bin_cmd!("pks") + // `RUST_LOG` makes pks log to stderr, which this test expects empty + .env_remove("RUST_LOG") .arg("--project-root") .arg("tests/fixtures/uses_strict_mode") .arg("check") @@ -789,6 +782,8 @@ fn test_check_warns_about_other_spelling_of_included_file( fn test_check_does_not_warn_when_every_path_matches_an_included_file( ) -> Result<(), Box> { cargo_bin_cmd!("pks") + // `RUST_LOG` makes pks log to stderr, which this test expects empty + .env_remove("RUST_LOG") .arg("--project-root") .arg("tests/fixtures/uses_strict_mode") .arg("check") @@ -983,6 +978,8 @@ fn test_check_ignores_polymorphic_association() -> Result<(), Box> { // `:event` matches the private `::Event` in packs/baz, but a polymorphic // association names no class. An empty stderr means the path matched. cargo_bin_cmd!("pks") + // `RUST_LOG` makes pks log to stderr, which this test expects empty + .env_remove("RUST_LOG") .arg("--project-root") .arg("tests/fixtures/app_with_rails_relationships") .arg("check") diff --git a/tests/common/mod.rs b/tests/common/mod.rs index 1a8d88f..8fcf448 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -1,3 +1,5 @@ +use assert_cmd::{assert::Assert, cargo::cargo_bin_cmd}; +use jsonschema::Validator; use std::{ fs, path::{Path, PathBuf}, @@ -62,6 +64,39 @@ impl Fixture { } } +/// Runs `pks` against the fixture. `RUST_LOG` would make pks log to stderr, +/// which tests often expect to be empty, so it is removed. +#[allow(dead_code)] +pub fn pks(fixture: &Fixture, args: &[&str]) -> Assert { + cargo_bin_cmd!("pks") + .env_remove("RUST_LOG") + .arg("--project-root") + .arg(fixture.root()) + .args(args) + .assert() +} + +/// Panics, listing every error, unless `output` matches the schema +#[allow(dead_code)] +pub fn validate_schema(schema_path: &str, output: &serde_json::Value) { + let schema = fs::read_to_string(schema_path) + .unwrap_or_else(|e| panic!("Could not read {}: {}", schema_path, e)); + let schema: serde_json::Value = + serde_json::from_str(&schema).expect("Schema should be valid JSON"); + let validator = + Validator::new(&schema).expect("Schema should be valid JSON Schema"); + let errors: Vec = validator + .iter_errors(output) + .map(|e| format!(" - {}", e)) + .collect(); + assert!( + errors.is_empty(), + "JSON output does not match {}:\n{}", + schema_path, + errors.join("\n") + ); +} + fn copy_dir_recursive(from: &Path, to: &Path) -> std::io::Result<()> { fs::create_dir_all(to)?; for entry in fs::read_dir(from)? { diff --git a/tests/expose_monkey_patches_test.rs b/tests/expose_monkey_patches_test.rs index 758b738..dbe9dce 100644 --- a/tests/expose_monkey_patches_test.rs +++ b/tests/expose_monkey_patches_test.rs @@ -23,3 +23,33 @@ fn test_expose_monkey_patches() -> Result<(), Box> { Ok(()) } + +#[test] +fn test_expose_monkey_patches_warns_only_about_the_apps_syntax_errors( +) -> Result<(), Box> { + let fixture = common::Fixture::new("app_with_monkey_patches"); + // Ruby's and gems' sources aren't the app's to fix, so they don't warn + let broken = "class Broken\n def a(\nend\n"; + std::fs::write(fixture.path("rubydir_stub/broken.rb"), broken)?; + std::fs::write(fixture.path("gemdir_stub/rails/broken.rb"), broken)?; + std::fs::write(fixture.path("app/services/broken.rb"), broken)?; + + let rubydir = fixture.path("rubydir_stub"); + let gemdir = fixture.path("gemdir_stub"); + common::pks( + &fixture, + &[ + "--experimental-parser", + "expose-monkey-patches", + &format!("--rubydir={}", rubydir.display()), + &format!("--gemdir={}", gemdir.display()), + ], + ) + .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", + ); + + Ok(()) +} diff --git a/tests/list_references_test.rs b/tests/list_references_test.rs index cba80cf..1d3c61d 100644 --- a/tests/list_references_test.rs +++ b/tests/list_references_test.rs @@ -1,21 +1,10 @@ -use assert_cmd::{assert::Assert, cargo::cargo_bin_cmd}; -use jsonschema::Validator; use pretty_assertions::assert_eq; use serde_json::{json, Value}; use std::{error::Error, fs}; mod common; -fn pks(fixture: &common::Fixture, args: &[&str]) -> Assert { - cargo_bin_cmd!("pks") - // `RUST_LOG` makes pks log to stderr, which `list_references` expects - // to be empty - .env_remove("RUST_LOG") - .arg("--project-root") - .arg(fixture.root()) - .args(args) - .assert() -} +use common::pks; /// Runs pks with `args`, which include `list-references`, and returns its /// output after checking it against the schema. @@ -27,28 +16,10 @@ fn list_references(fixture: &common::Fixture, args: &[&str]) -> Value { .stdout .clone(); let output = serde_json::from_slice(&stdout).expect("output is not JSON"); - validate_schema(&output); + common::validate_schema("schema/list-references-output.json", &output); output } -fn validate_schema(output: &Value) { - let schema = fs::read_to_string("schema/list-references-output.json") - .expect("Failed to read schema file"); - let schema: Value = - serde_json::from_str(&schema).expect("Schema should be valid JSON"); - let validator = - Validator::new(&schema).expect("Schema should be valid JSON Schema"); - let errors: Vec = validator - .iter_errors(output) - .map(|e| format!(" - {}", e)) - .collect(); - assert!( - errors.is_empty(), - "JSON output does not match schema:\n{}", - errors.join("\n") - ); -} - // In simple_app, `packs/foo` references `::Bar`, which is private to // `packs/bar` and which it doesn't depend on, and `::Baz`, which it depends on. @@ -230,6 +201,46 @@ fn test_list_references_from_given_packs() -> Result<(), Box> { Ok(()) } +#[test] +fn test_list_references_prints_files_to_stderr() -> Result<(), Box> { + let fixture = common::Fixture::new("simple_app"); + + let output = pks(&fixture, &["--print-files", "list-references"]) + .success() + .get_output() + .clone(); + let references: Value = + serde_json::from_slice(&output.stdout).expect("output is not JSON"); + assert_eq!( + references, + json!({ "references": [bar_from_foo(), baz_from_foo()] }) + ); + assert!(String::from_utf8(output.stderr)?.contains("Started processing")); + + Ok(()) +} + +#[test] +fn test_list_references_from_given_packs_reads_only_their_files( +) -> Result<(), Box> { + let fixture = common::Fixture::new("simple_app"); + // pks warns about a file that doesn't parse, but only if it reads it + fs::write(fixture.path("packs/bar/app/services/broken.rb"), "def a(\n")?; + + assert_eq!( + list_references( + &fixture, + &["list-references", "--referencing-pack", "packs/foo"] + ), + json!({ "references": [bar_from_foo(), baz_from_foo()] }) + ); + pks(&fixture, &["list-references"]) + .success() + .stderr(predicates::str::contains("broken.rb")); + + Ok(()) +} + #[test] fn test_list_references_rejects_an_unknown_pack() -> Result<(), Box> { diff --git a/tests/rm_test.rs b/tests/rm_test.rs index 604e85f..61d6768 100644 --- a/tests/rm_test.rs +++ b/tests/rm_test.rs @@ -1,10 +1,11 @@ -use assert_cmd::{assert::Assert, cargo::cargo_bin_cmd}; use predicates::prelude::*; use pretty_assertions::assert_eq; use std::{collections::BTreeMap, error::Error, fs, path::Path}; mod common; +use common::pks; + // In the fixture, `packs/old` is about to be removed and every other pack // still names it: the root and `packs/foo` depend on it, `packs/bar` and // `packs/baz` list it in `visible_to`, `packs/baz` ignores it as a dependency, @@ -12,14 +13,6 @@ mod common; // No code references `Old` any more, so those todo entries are stale, as they // are once the last use of a pack is deleted. -fn pks(fixture: &common::Fixture, args: &[&str]) -> Assert { - cargo_bin_cmd!("pks") - .arg("--project-root") - .arg(fixture.root()) - .args(args) - .assert() -} - fn read(fixture: &common::Fixture, path: &str) -> String { fs::read_to_string(fixture.path(path)) .unwrap_or_else(|e| panic!("Could not read {}: {}", path, e)) @@ -146,6 +139,26 @@ fn test_rm_refuses_a_pack_other_packs_reference() -> Result<(), Box> Ok(()) } +#[test] +fn test_rm_lists_a_reference_once_when_several_files_define_its_constant( +) -> Result<(), Box> { + let fixture = common::Fixture::new("app_with_obsolete_pack"); + reference_old_from_baz(&fixture); + // The experimental parser resolves `::Old` to each file that defines it + fs::create_dir_all(fixture.path("packs/old/app/models"))?; + fs::write( + fixture.path("packs/old/app/models/old.rb"), + "module Old\n def self.other; end\nend\n", + )?; + + pks(&fixture, &["--experimental-parser", "rm", "packs/old"]) + .code(2) + .stdout("packs/baz/app/services/baz.rb:3:4 references ::Old\n") + .stderr(predicate::str::contains("Found 1 reference")); + + Ok(()) +} + #[test] fn test_rm_force_removes_a_pack_other_packs_reference( ) -> Result<(), Box> { diff --git a/tests/syntax_error_test.rs b/tests/syntax_error_test.rs new file mode 100644 index 0000000..a7be7e1 --- /dev/null +++ b/tests/syntax_error_test.rs @@ -0,0 +1,66 @@ +use std::{error::Error, fs}; + +mod common; + +/// simple_app, with the cache on, since a file with a syntax error must warn on +/// every run rather than only when it is first parsed +fn fixture_app() -> Result> { + let fixture = common::Fixture::new("simple_app"); + let packwerk_yml = fixture.path("packwerk.yml"); + let contents = fs::read_to_string(&packwerk_yml)?; + fs::write( + &packwerk_yml, + contents.replace("cache: false", "cache: true"), + )?; + Ok(fixture) +} + +fn check_stderr( + fixture: &common::Fixture, + args: &[&str], +) -> Result> { + let args = [args, &["check"]].concat(); + let output = common::pks(fixture, &args).get_output().clone(); + Ok(String::from_utf8(output.stderr)?) +} + +#[test] +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"; + for args in [&[][..], &["--experimental-parser"]] { + let fixture = fixture_app()?; + fs::write( + fixture.path("packs/foo/app/services/broken.rb"), + "class Broken\n def a\n Bar\n end\n\n def b(\nend\n", + )?; + + assert_eq!(check_stderr(&fixture, args)?, warning); + assert_eq!(check_stderr(&fixture, args)?, warning); + } + + 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"; + for args in [&[][..], &["--experimental-parser"]] { + let fixture = fixture_app()?; + fs::write( + fixture.path("packs/foo/app/views/broken.erb"), + "<%= Bar.bar( %>\n", + )?; + + assert_eq!(check_stderr(&fixture, args)?, warning); + } + + Ok(()) +}