Warn about files that don't parse, read <%== in ERB, and release 0.7.0

.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

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

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"

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<UnresolvedReference>,
     pub definitions: Vec<ParsedDefinition>,
+    /// Never cached, since a file with a syntax error isn't
+    #[serde(skip)]
+    pub syntax_error: Option<SyntaxError>,
 }
 
 #[derive(

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 {

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

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

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(

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

src/packs/parsing/mod.rs

@@ -25,7 +25,7 @@ pub fn process_file(
     configuration: &Configuration,
 ) -> anyhow::Result<ProcessedFile> {
     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<usize>,
+    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<ProcessedFile> {
-            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()
 }