Read bs-dev-dependencies if --dev was passed. (#7650)

* Read bs-dev-dependencies if --dev was passed.

* Try and check the file type when allowing dev dependency access.

* Store is_type_dev in SourceFileMeta.

* Only allow usage of dev_dependencies when type:dev is true for source file

* Traverse rescript config for compiler-args command to find out if type = dev

* Collect all qualified package sources

* Expand search for type:dev in rescript config.

* Add some unit test to cover find_is_type_dev_for_path

* Add is_type_dev to Module as well.

* Remove pub

* Add changelog entry

* Add negative test

* Add snapshot

* Normalize paths

* Try --ignore-cr-at-eol

* Revert "Try --ignore-cr-at-eol"

This reverts commit 3a67a8f.

* Trigger CI

* Remove extra lines in changelog

* Add tmate on Windows

* Add tmate on Windows but earlier 🙃

* Remove more stuff

* More general path normalize check on Windows

* This has to be newlines

* Update to beta 1

* Revert line changes

Author: nojaf

CHANGELOG.md

@@ -44,6 +44,7 @@
 - Fix inside comment printing for empty dict. https://github.com/rescript-lang/rescript/pull/7654
 - Fix I/O error message when trying to extract extra info from non-existing file. https://github.com/rescript-lang/rescript/pull/7656
 - Fix fatal error when JSX expression used without configuring JSX in rescript.json. https://github.com/rescript-lang/rescript/pull/7656
+- Rewatch: Only allow access to `"bs-dev-dependencies"` from `"type": "dev"` source files. https://github.com/rescript-lang/rescript/pull/7650
 
 # 12.0.0-beta.1
 

rewatch/src/build.rs

@@ -8,6 +8,8 @@ pub mod packages;
 pub mod parse;
 pub mod read_compile_state;
 
+use self::compile::compiler_args;
+use self::parse::parser_args;
 use crate::build::compile::{mark_modules_with_deleted_deps_dirty, mark_modules_with_expired_deps_dirty};
 use crate::helpers::emojis::*;
 use crate::helpers::{self, get_workspace_root};
@@ -26,9 +28,6 @@ use std::path::{Path, PathBuf};
 use std::process::Stdio;
 use std::time::{Duration, Instant};
 
-use self::compile::compiler_args;
-use self::parse::parser_args;
-
 fn is_dirty(module: &Module) -> bool {
     match module.source_type {
         SourceType::SourceFile(SourceFile {
@@ -56,14 +55,18 @@ pub struct CompilerArgs {
     pub parser_args: Vec<String>,
 }
 
-pub fn get_compiler_args(path: &Path, build_dev_deps: bool) -> Result<String> {
+pub fn get_compiler_args(path: &Path) -> Result<String> {
     let filename = &helpers::get_abs_path(path);
     let package_root =
         helpers::get_abs_path(&helpers::get_nearest_config(&path).expect("Couldn't find package root"));
     let workspace_root = get_workspace_root(&package_root).map(|p| helpers::get_abs_path(&p));
     let root_rescript_config =
         packages::read_config(&workspace_root.to_owned().unwrap_or(package_root.to_owned()))?;
     let rescript_config = packages::read_config(&package_root)?;
+    let is_type_dev = match filename.strip_prefix(&package_root) {
+        Err(_) => false,
+        Ok(relative_path) => root_rescript_config.find_is_type_dev_for_path(relative_path),
+    };
 
     // make PathBuf from package root and get the relative path for filename
     let relative_filename = filename.strip_prefix(PathBuf::from(&package_root)).unwrap();
@@ -97,7 +100,7 @@ pub fn get_compiler_args(path: &Path, build_dev_deps: bool) -> Result<String> {
         &package_root,
         &workspace_root,
         &None,
-        build_dev_deps,
+        is_type_dev,
         true,
     );
 
@@ -281,7 +284,6 @@ pub fn incremental_build(
     show_progress: bool,
     only_incremental: bool,
     create_sourcedirs: bool,
-    build_dev_deps: bool,
     snapshot_output: bool,
 ) -> Result<(), IncrementalBuildError> {
     logs::initialize(&build_state.packages);
@@ -393,7 +395,6 @@ pub fn incremental_build(
         show_progress,
         || pb.inc(1),
         |size| pb.set_length(size),
-        build_dev_deps,
     )
     .map_err(|e| IncrementalBuildError {
         kind: IncrementalBuildErrorKind::CompileError(Some(e.to_string())),
@@ -500,7 +501,6 @@ pub fn build(
         show_progress,
         false,
         create_sourcedirs,
-        build_dev_deps,
         snapshot_output,
     ) {
         Ok(_) => {

rewatch/src/build/build_types.rs

@@ -71,6 +71,7 @@ pub struct Module {
     pub last_compiled_cmi: Option<SystemTime>,
     pub last_compiled_cmt: Option<SystemTime>,
     pub deps_dirty: bool,
+    pub is_type_dev: bool,
 }
 
 impl Module {

rewatch/src/build/compile.rs

@@ -22,7 +22,6 @@ pub fn compile(
     show_progress: bool,
     inc: impl Fn() + std::marker::Sync,
     set_length: impl Fn(u64),
-    build_dev_deps: bool,
 ) -> anyhow::Result<(String, String, usize)> {
     let mut compiled_modules = AHashSet::<String>::new();
     let dirty_modules = build_state
@@ -170,7 +169,6 @@ pub fn compile(
                                         &build_state.packages,
                                         &build_state.project_root,
                                         &build_state.workspace_root,
-                                        build_dev_deps,
                                     );
                                     Some(result)
                                 }
@@ -186,7 +184,6 @@ pub fn compile(
                                 &build_state.packages,
                                 &build_state.project_root,
                                 &build_state.workspace_root,
-                                build_dev_deps,
                             );
                             let cmi_digest_after = helpers::compute_file_hash(Path::new(&cmi_path));
 
@@ -360,13 +357,13 @@ pub fn compiler_args(
     // if packages are known, we pass a reference here
     // this saves us a scan to find their paths
     packages: &Option<&AHashMap<String, packages::Package>>,
-    build_dev_deps: bool,
+    // Is the file listed as "type":"dev"?
+    is_type_dev: bool,
     is_local_dep: bool,
 ) -> Vec<String> {
     let bsc_flags = config::flatten_flags(&config.bsc_flags);
 
-    let dependency_paths =
-        get_dependency_paths(config, project_root, workspace_root, packages, build_dev_deps);
+    let dependency_paths = get_dependency_paths(config, project_root, workspace_root, packages, is_type_dev);
 
     let module_name = helpers::file_path_to_module_name(file_path, &config.get_namespace());
 
@@ -504,7 +501,7 @@ fn get_dependency_paths(
     project_root: &Path,
     workspace_root: &Option<PathBuf>,
     packages: &Option<&AHashMap<String, packages::Package>>,
-    build_dev_deps: bool,
+    is_file_type_dev: bool,
 ) -> Vec<Vec<String>> {
     let normal_deps = config
         .bs_dependencies
@@ -513,7 +510,9 @@ fn get_dependency_paths(
         .into_iter()
         .map(DependentPackage::Normal)
         .collect();
-    let dev_deps = if build_dev_deps {
+
+    // We can only access dev dependencies for source_files that are marked as "type":"dev"
+    let dev_deps = if is_file_type_dev {
         config
             .bs_dev_dependencies
             .clone()
@@ -569,7 +568,6 @@ fn compile_file(
     packages: &AHashMap<String, packages::Package>,
     project_root: &Path,
     workspace_root: &Option<PathBuf>,
-    build_dev_deps: bool,
 ) -> Result<Option<String>, String> {
     let ocaml_build_path_abs = package.get_ocaml_build_path();
     let build_path_abs = package.get_build_path();
@@ -583,6 +581,7 @@ fn compile_file(
     }?;
     let module_name = helpers::file_path_to_module_name(implementation_file_path, &package.namespace);
     let has_interface = module.get_interface().is_some();
+    let is_type_dev = module.is_type_dev;
     let to_mjs_args = compiler_args(
         &package.config,
         &root_package.config,
@@ -593,7 +592,7 @@ fn compile_file(
         project_root,
         workspace_root,
         &Some(packages),
-        build_dev_deps,
+        is_type_dev,
         package.is_local_dep,
     );
 

rewatch/src/build/packages.rs

@@ -19,6 +19,7 @@ use std::time::SystemTime;
 #[derive(Debug, Clone)]
 pub struct SourceFileMeta {
     pub modified: SystemTime,
+    pub is_type_dev: bool,
 }
 
 #[derive(Debug, Clone)]
@@ -112,6 +113,13 @@ impl Package {
             .expect("namespace should be set for mlmap module");
         self.get_build_path().join(format!("{}.cmi", suffix))
     }
+
+    pub fn is_source_file_type_dev(&self, path: &Path) -> bool {
+        self.source_files
+            .as_ref()
+            .and_then(|sf| sf.get(path).map(|sfm| sfm.is_type_dev))
+            .unwrap_or(false)
+    }
 }
 
 impl PartialEq for Package {
@@ -138,6 +146,7 @@ pub fn read_folders(
     package_dir: &Path,
     path: &Path,
     recurse: bool,
+    is_type_dev: bool,
 ) -> Result<AHashMap<PathBuf, SourceFileMeta>, Box<dyn error::Error>> {
     let mut map: AHashMap<PathBuf, SourceFileMeta> = AHashMap::new();
     let path_buf = PathBuf::from(path);
@@ -147,6 +156,7 @@ pub fn read_folders(
             path.to_owned(),
             SourceFileMeta {
                 modified: meta.modified().unwrap(),
+                is_type_dev,
             },
         )
     });
@@ -159,7 +169,7 @@ pub fn read_folders(
         let path_ext = entry_path_buf.extension().and_then(|x| x.to_str());
         let new_path = path_buf.join(&name);
         if metadata.file_type().is_dir() && recurse {
-            match read_folders(filter, package_dir, &new_path, recurse) {
+            match read_folders(filter, package_dir, &new_path, recurse, is_type_dev) {
                 Ok(s) => map.extend(s),
                 Err(e) => log::error!("Could not read directory: {}", e),
             }
@@ -174,6 +184,7 @@ pub fn read_folders(
                         path,
                         SourceFileMeta {
                             modified: metadata.modified().unwrap(),
+                            is_type_dev,
                         },
                     );
                 }
@@ -297,11 +308,16 @@ fn read_dependencies(
     project_root: &Path,
     workspace_root: &Option<PathBuf>,
     show_progress: bool,
+    build_dev_deps: bool,
 ) -> Vec<Dependency> {
-    return parent_config
-        .bs_dependencies
-        .to_owned()
-        .unwrap_or_default()
+    let mut dependencies = parent_config.bs_dependencies.to_owned().unwrap_or_default();
+
+    // Concatenate dev dependencies if build_dev_deps is true
+    if build_dev_deps && let Some(dev_deps) = parent_config.bs_dev_dependencies.to_owned() {
+        dependencies.extend(dev_deps);
+    }
+
+    dependencies
         .iter()
         .filter_map(|package_name| {
             if registered_dependencies_set.contains(package_name) {
@@ -360,7 +376,8 @@ fn read_dependencies(
                 &canonical_path,
                 project_root,
                 workspace_root,
-                show_progress
+                show_progress,
+                build_dev_deps
             );
 
             Dependency {
@@ -371,7 +388,7 @@ fn read_dependencies(
                 dependencies,
             }
         })
-        .collect::<Vec<Dependency>>();
+        .collect()
 }
 
 fn flatten_dependencies(dependencies: Vec<Dependency>) -> Vec<Dependency> {
@@ -461,6 +478,7 @@ fn read_packages(
     project_root: &Path,
     workspace_root: &Option<PathBuf>,
     show_progress: bool,
+    build_dev_deps: bool,
 ) -> Result<AHashMap<String, Package>> {
     let root_config = read_config(project_root)?;
 
@@ -477,6 +495,7 @@ fn read_packages(
         project_root,
         workspace_root,
         show_progress,
+        build_dev_deps,
     ));
     dependencies.iter().for_each(|d| {
         if !map.contains_key(&d.name) {
@@ -515,9 +534,14 @@ pub fn get_source_files(
     };
 
     let path_dir = Path::new(&source.dir);
+    let is_type_dev = type_
+        .as_ref()
+        .map(|t| t.as_str() == "dev")
+        .unwrap_or(false)
+        .clone();
     match (build_dev_deps, type_) {
         (false, Some(type_)) if type_ == "dev" => (),
-        _ => match read_folders(filter, package_dir, path_dir, recurse) {
+        _ => match read_folders(filter, package_dir, path_dir, recurse, is_type_dev) {
             Ok(files) => map.extend(files),
 
             Err(_e) => log::error!(
@@ -596,7 +620,7 @@ pub fn make(
     show_progress: bool,
     build_dev_deps: bool,
 ) -> Result<AHashMap<String, Package>> {
-    let map = read_packages(root_folder, workspace_root, show_progress)?;
+    let map = read_packages(root_folder, workspace_root, show_progress, build_dev_deps)?;
 
     /* Once we have the deduplicated packages, we can add the source files for each - to minimize
      * the IO */
@@ -720,6 +744,8 @@ pub fn parse_packages(build_state: &mut BuildState) {
                         compile_dirty: false,
                         last_compiled_cmt: None,
                         last_compiled_cmi: None,
+                        // Not sure if this is correct
+                        is_type_dev: false,
                     },
                 );
             });
@@ -772,6 +798,7 @@ pub fn parse_packages(build_state: &mut BuildState) {
                                 compile_dirty: true,
                                 last_compiled_cmt: None,
                                 last_compiled_cmi: None,
+                                is_type_dev: metadata.is_type_dev,
                             });
                     } else {
                         // remove last character of string: resi -> res, rei -> re, mli -> ml
@@ -833,6 +860,7 @@ pub fn parse_packages(build_state: &mut BuildState) {
                                         compile_dirty: true,
                                         last_compiled_cmt: None,
                                         last_compiled_cmi: None,
+                                        is_type_dev: metadata.is_type_dev,
                                     });
                             }
                         }

rewatch/src/cli.rs

@@ -214,9 +214,6 @@ pub enum Command {
         /// Path to a rescript file (.res or .resi)
         #[command()]
         path: String,
-
-        #[command(flatten)]
-        dev: DevArg,
     },
     /// Use the legacy build system.
     ///