Skip ERB comments and <%% literals when converting ERB to Ruby

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

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(),

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(())
+    }
 }

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();
 

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<UnresolvedReference> = ["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
+        );
+    }
 }