Skip to content

Track RPL_PATS environment variable and pattern files#110

Draft
TheVeryDarkness wants to merge 1 commit intoRPL-Toolchain:masterfrom
TheVeryDarkness:track-rpl-pats
Draft

Track RPL_PATS environment variable and pattern files#110
TheVeryDarkness wants to merge 1 commit intoRPL-Toolchain:masterfrom
TheVeryDarkness:track-rpl-pats

Conversation

@TheVeryDarkness
Copy link
Collaborator

This should be able to fix the interaction issue between RPL and rustc's incremental mechanism.

@TheVeryDarkness
Copy link
Collaborator Author

TODO: add some basic CI for this

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Tracks RPL_PATS (env var value + referenced pattern paths) in rustc dep-info so Cargo/rustc incremental correctly invalidates when RPL’s configuration changes.

Changes:

  • Capture RPL_PATS as a Result in the driver and pass both the raw env var value and parsed paths into RplCallbacks.
  • Add RPL_PATS env dep tracking and pattern-path file dep tracking in RplCallbacks.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
src/driver.rs Plumbs RPL_PATS env var value and parsed pattern paths into RplCallbacks.
crates/rpl_interface/src/callbacks.rs Tracks RPL_PATS in env_depinfo and adds pattern-path entries to file_depinfo.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 18 to +19
pub static RPL_ARGS_ENV: &str = "RPL_ARGS";
pub static RPL_PATS_ENV: &str = "RPL_PATS";
Copy link

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RPL_PATS_ENV is introduced here, but the driver still hard-codes "RPL_PATS". To avoid drift/typos, consider re-exporting RPL_PATS_ENV from crates/rpl_interface/src/lib.rs (like RPL_ARGS_ENV) and using the constant everywhere.

Copilot uses AI. Check for mistakes.
Comment on lines +224 to +225
let rpl_pats_var = env::var("RPL_PATS");
let pattern_paths = match &rpl_pats_var {
Copy link

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The driver hard-codes the env var name ("RPL_PATS") instead of using a shared constant (similar to rpl_interface::RPL_ARGS_ENV). Consider re-exporting/using RPL_PATS_ENV so changes to the env var name can’t silently diverge across crates.

Copilot uses AI. Check for mistakes.
Comment on lines +51 to +55
let pat_depinfo = psess.file_depinfo.get_mut();

for path in pats {
pat_depinfo.insert(Symbol::intern(path));
}
Copy link

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

track_rpl_pats records the raw RPL_PATS arguments into file_depinfo, but RPL_PATS is commonly set to a directory (e.g. docs/patterns-pest). Tracking only the directory path won’t cause Cargo/rustc to rerun when an existing .rpl file inside that directory changes (directory mtime doesn’t update on file content changes). Consider tracking the actual .rpl files read (e.g. the canonicalized paths produced by collect_file_from_string_args/traverse_rpl) rather than the input roots.

Copilot uses AI. Check for mistakes.
Comment on lines 121 to +123
let rpl_args_var = self.rpl_args_var.take();
let rpl_pats_var = self.rpl_pats_var.take();
let rpl_pats = self.pattern_paths.clone();
Copy link

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let rpl_pats = self.pattern_paths.clone(); needlessly clones the full pattern path list just to feed dep-info tracking. Since patterns are already being collected into PATTERNS in this method, you can derive the tracked path list once from that result (or take() the option) and avoid the extra allocation/copy.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant