diff --git a/CHANGELOG.md b/CHANGELOG.md index 231523cc..73ff3a45 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,36 @@ ### Fixes +#### ERB comments no longer hide or invent references + +pks read every line of a multi-line `<%# %>` comment after the first as Ruby +code. Depending on what the comment said, references elsewhere in the file were +missed, sometimes all of them, and constants named inside the comment were +reported as references. ERB comments are now skipped, as packwerk does. This +changes results in both directions: references that were missed are now +reported, and a `package_todo.yml` entry recorded for a constant named inside a +comment is no longer found, so `pks check` reports it as stale until you run +`pks update`. + +pks also found nothing at all in an ERB file whose first tag was an encoding +comment, such as `<%# encoding: iso-8859-1 %>`, unless it named UTF-8 or +ASCII-8BIT. Those files are now checked like any other, so they may show new +violations. + +#### `<%%` in ERB no longer hides references + +`<%%` writes a literal `<%`, but pks read the rest of the tag as Ruby, so a file +using it lost some or all of its references. It's now treated as text, as Rails +does. packwerk parses the literal as Ruby instead, so it fails on `<%%=` and can +report constants from inside other literals. + +#### Upgrading pks invalidates cached results + +Cached results didn't record which version of pks produced them, so after an +upgrade pks kept serving old results for files that hadn't changed, including +results the ERB fixes above correct. Each entry now includes the pks version, so +the first run after an upgrade re-reads every file. + #### `check -o json` and `-o csv` list each strict violation once A strict violation that fails the run is both a new violation and a strict-mode diff --git a/src/packs/caching/mod.rs b/src/packs/caching/mod.rs index c3485f06..c772eb42 100644 --- a/src/packs/caching/mod.rs +++ b/src/packs/caching/mod.rs @@ -29,7 +29,11 @@ impl EmptyCacheEntry { let file_name_digest = format!("{:x}", file_digest); let cache_file_path = cache_directory.join(&file_name_digest); - let file_contents_digest = file_content_digest(filepath)?; + let file_contents_digest = format!( + "{}-{}", + file_content_digest(filepath)?, + env!("CARGO_PKG_VERSION") + ); Ok(EmptyCacheEntry { filepath: filepath.to_owned(), diff --git a/src/packs/caching/per_file_cache.rs b/src/packs/caching/per_file_cache.rs index 8d4118b1..193cf0a9 100644 --- a/src/packs/caching/per_file_cache.rs +++ b/src/packs/caching/per_file_cache.rs @@ -222,4 +222,49 @@ mod tests { Ok(()) } + + #[test] + fn test_entry_from_another_pks_version_is_a_miss() -> anyhow::Result<()> { + let cache_dir = tempfile::tempdir()?; + let cache = PerFileCache { + cache_dir: cache_dir.path().to_path_buf(), + }; + let path = PathBuf::from( + "tests/fixtures/simple_app/packs/bar/app/services/bar.rb", + ); + let processed_file = ProcessedFile { + absolute_path: path.clone(), + unresolved_references: vec![], + definitions: vec![], + }; + + let CacheResult::Miss(empty_cache_entry) = cache.get(&path)? else { + panic!("expected a miss on an empty cache"); + }; + cache.write(&empty_cache_entry, &processed_file)?; + assert!(matches!(cache.get(&path)?, CacheResult::Processed(_))); + + // The bare content digest is what entries held before they recorded + // the pks version. + let content_digest = file_content_digest(&path)?; + for file_contents_digest in + [content_digest.clone(), format!("{content_digest}-0.0.0")] + { + let entry = CacheEntry { + file_contents_digest, + processed_file: processed_file.clone(), + }; + fs::write( + &empty_cache_entry.cache_file_path, + serde_json::to_string(&entry)?, + )?; + assert!( + matches!(cache.get(&path)?, CacheResult::Miss(_)), + "served an entry with digest {}", + entry.file_contents_digest + ); + } + + Ok(()) + } } diff --git a/src/packs/file_utils.rs b/src/packs/file_utils.rs index 8b1a5003..929ddb6c 100644 --- a/src/packs/file_utils.rs +++ b/src/packs/file_utils.rs @@ -109,6 +109,9 @@ pub(crate) fn convert_erb_to_ruby_without_sourcemaps( let extracted_contents: Vec<&str> = regex .captures_iter(&contents) + .filter(|capture| { + !capture[0].starts_with("<%#") && !capture[0].starts_with("<%%") + }) .map(|capture| capture.get(1).unwrap().as_str()) .collect(); diff --git a/src/packs/parsing/erb/packwerk.rs b/src/packs/parsing/erb/packwerk.rs index 2c790837..ee5c8bbc 100644 --- a/src/packs/parsing/erb/packwerk.rs +++ b/src/packs/parsing/erb/packwerk.rs @@ -234,4 +234,162 @@ mod tests { .unresolved_references ); } + + #[test] + fn multiline_comment_does_not_hide_surrounding_references() { + let configuration = Configuration::default(); + let expected: Vec = ["Foo", "Bar", "Baz"] + .iter() + .map(|name| UnresolvedReference { + name: String::from(*name), + namespace_path: vec![], + location: Range::default(), + }) + .collect(); + + for text in [ + "second line.", + "it's here", + "the end", + "see } here", + "second (line", + "class foo", + "def x", + ] { + let contents = format!( + "/ +<%= Foo %> +<%# first line + {text} %> +<%= Bar %> +<%= Baz %> + " + ); + assert_eq!( + expected, + process_from_contents( + contents, + &PathBuf::from("path/to/file.rb"), + &configuration + ) + .unresolved_references, + "comment ending with {text:?}" + ); + } + } + + #[test] + fn comment_contents_are_not_references() { + let contents: String = String::from( + "/ +<%# Qux %> +<%# + Foo +%> +<%# uses + Baz -%> +<%= Bar %> + ", + ); + let configuration = Configuration::default(); + assert_eq!( + vec![UnresolvedReference { + name: String::from("Bar"), + namespace_path: vec![], + location: Range::default() + }], + process_from_contents( + contents, + &PathBuf::from("path/to/file.rb"), + &configuration + ) + .unresolved_references + ); + } + + #[test] + fn leading_encoding_comment_does_not_hide_references() { + let contents: String = + String::from("<%# encoding: iso-8859-1 %>\n<%= Foo %>"); + let configuration = Configuration::default(); + assert_eq!( + vec![UnresolvedReference { + name: String::from("Foo"), + 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( + "/ +<%= Foo %> +<%%= Qux %> +<%% if Quux %%> +<%= Bar %> + ", + ); + 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() + } + ], + process_from_contents( + contents, + &PathBuf::from("path/to/file.rb"), + &configuration + ) + .unresolved_references + ); + } + + #[test] + fn hash_after_hyphen_or_space_is_code_not_comment() { + let contents: String = String::from( + "/ +<%-# first line + Foo %> +<% # first line + Bar %> + ", + ); + 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() + } + ], + process_from_contents( + contents, + &PathBuf::from("path/to/file.rb"), + &configuration + ) + .unresolved_references + ); + } }