Permit FFI files in subdirectories

.github/workflows/ci.yaml

@@ -419,3 +419,7 @@ jobs:
       - name: Test running modules
         run: make test
         working-directory: ./test/running_modules
+
+      - name: Test FFI in subdirectories
+        run: make
+        working-directory: ./test/subdir_ffi

CHANGELOG.md

@@ -44,6 +44,10 @@
 
   ([Richard Viney](https://github.com/richard-viney))
 
+- FFI files (such as `.mjs` and `.erl`) are now permitted in
+subdirectories of `src/` and `test/`.
+([PgBiel](https://github.com/PgBiel))
+
 ### Compiler
 
 - The compiler now prints correctly qualified or aliased type names when
@@ -416,6 +420,10 @@
   to a negative value wider than 48 bits in a bit array.
   ([Richard Viney](https://github.com/richard-viney))
 
+- Fixed a bug where a `module.mjs` file would be overwritten by
+a `module.gleam` file of same name without warning. It now produces an error.
+([PgBiel](https://github.com/PgBiel))
+
 ## v1.5.1 - 2024-09-26
 
 ### Bug Fixes

Makefile

@@ -25,6 +25,7 @@ test: ## Run the compiler unit tests
 	cd test/project_deno && cargo run clean && cargo run check && cargo run test
 	cd test/hextarball && make test
 	cd test/running_modules && make test
+	cd test/subdir_ffi && make
 
 .PHONY: language-test
 language-test: ## Run the language integration tests for all targets

compiler-cli/src/fs.rs

@@ -13,7 +13,6 @@ use gleam_core::{
 };
 use std::{
     collections::HashSet,
-    ffi::OsStr,
     fmt::Debug,
     fs::File,
     io::{self, BufRead, BufReader, Write},
@@ -74,38 +73,6 @@ impl ProjectIO {
 }
 
 impl FileSystemReader for ProjectIO {
-    fn gleam_source_files(&self, dir: &Utf8Path) -> Vec<Utf8PathBuf> {
-        if !dir.is_dir() {
-            return vec![];
-        }
-        let dir = dir.to_path_buf();
-        walkdir::WalkDir::new(dir)
-            .follow_links(true)
-            .into_iter()
-            .filter_map(Result::ok)
-            .filter(|e| e.file_type().is_file())
-            .map(|d| d.into_path())
-            .filter(move |d| d.extension() == Some(OsStr::new("gleam")))
-            .map(|pb| Utf8PathBuf::from_path_buf(pb).expect("Non Utf-8 Path"))
-            .collect()
-    }
-
-    fn gleam_cache_files(&self, dir: &Utf8Path) -> Vec<Utf8PathBuf> {
-        if !dir.is_dir() {
-            return vec![];
-        }
-        let dir = dir.to_path_buf();
-        walkdir::WalkDir::new(dir)
-            .follow_links(true)
-            .into_iter()
-            .filter_map(Result::ok)
-            .filter(|e| e.file_type().is_file())
-            .map(|d| d.into_path())
-            .filter(|p| p.extension().and_then(OsStr::to_str) == Some("cache"))
-            .map(|pb| Utf8PathBuf::from_path_buf(pb).expect("Non Utf-8 Path"))
-            .collect()
-    }
-
     fn read(&self, path: &Utf8Path) -> Result<String, Error> {
         read(path)
     }

compiler-core/src/build/native_file_copier.rs

@@ -1,12 +1,13 @@
 #[cfg(test)]
 mod tests;
 
-use std::collections::HashSet;
+use std::collections::{HashMap, HashSet};
 
 use camino::{Utf8Path, Utf8PathBuf};
+use ecow::{eco_format, EcoString};
 
 use crate::{
-    io::{FileSystemReader, FileSystemWriter},
+    io::{DirWalker, FileSystemReader, FileSystemWriter},
     Error, Result,
 };
 
@@ -21,6 +22,7 @@ pub(crate) struct NativeFileCopier<'a, IO> {
     root: &'a Utf8Path,
     destination_dir: &'a Utf8Path,
     seen_native_files: HashSet<Utf8PathBuf>,
+    seen_modules: HashMap<EcoString, Utf8PathBuf>,
     to_compile: Vec<Utf8PathBuf>,
     elixir_files_copied: bool,
 }
@@ -36,6 +38,7 @@ where
             destination_dir: out,
             to_compile: Vec::new(),
             seen_native_files: HashSet::new(),
+            seen_modules: HashMap::new(),
             elixir_files_copied: false,
         }
     }
@@ -57,39 +60,60 @@ where
             self.copy_files(&test)?;
         }
 
+        // Sort for deterministic output
+        self.to_compile.sort_unstable();
+
         Ok(CopiedNativeFiles {
             to_compile: self.to_compile,
             any_elixir: self.elixir_files_copied,
         })
     }
 
     fn copy_files(&mut self, src_root: &Utf8Path) -> Result<()> {
-        let mut check_elixir_libs = true;
-
-        for entry in self.io.read_dir(src_root)? {
-            let path = entry.expect("copy_native_files dir_entry").pathbuf;
-            self.copy(path, src_root)?;
+        let mut dir_walker = DirWalker::new(src_root.to_path_buf());
+        while let Some(path) = dir_walker.next_file(&self.io)? {
+            self.copy(path, &src_root)?;
         }
         Ok(())
     }
 
     fn copy(&mut self, file: Utf8PathBuf, src_root: &Utf8Path) -> Result<()> {
         let extension = file.extension().unwrap_or_default();
 
+        let relative_path = file
+            .strip_prefix(src_root)
+            .expect("copy_native_files strip prefix")
+            .to_path_buf();
+
+        // No need to run duplicate native file checks for .gleam files, but we
+        // still need to check for conflicting `.gleam` and `.mjs` files, so we
+        // add a special case for `.gleam`.
+        if extension == "gleam" {
+            self.check_for_conflicting_javascript_modules(&relative_path)?;
+
+            return Ok(());
+        }
+
         // Skip unknown file formats that are not supported native files
         if !matches!(extension, "mjs" | "js" | "ts" | "hrl" | "erl" | "ex") {
             return Ok(());
         }
 
-        let relative_path = file
-            .strip_prefix(src_root)
-            .expect("copy_native_files strip prefix")
-            .to_path_buf();
         let destination = self.destination_dir.join(&relative_path);
 
         // Check that this native file was not already copied
         self.check_for_duplicate(&relative_path)?;
 
+        // Check for JavaScript modules conflicting between each other within
+        // the same relative path. We need to do this as '.gleam' files can
+        // also cause a conflict, despite not being native files, as they are
+        // compiled to `.mjs`.
+        self.check_for_conflicting_javascript_modules(&relative_path)?;
+
+        // Check for Erlang modules conflicting between each other anywhere in
+        // the tree.
+        self.check_for_conflicting_erlang_modules(&relative_path)?;
+
         // If the source file's mtime is older than the destination file's mtime
         // then it has not changed and as such does not need to be copied.
         //
@@ -103,6 +127,12 @@ where
         }
 
         tracing::debug!(?file, "copying_native_file");
+
+        // Ensure destination exists (subdir might not exist yet in the output)
+        if let Some(parent) = destination.parent() {
+            self.io.mkdir(parent)?;
+        }
+
         self.io.copy(&file, &destination)?;
         self.elixir_files_copied = self.elixir_files_copied || extension == "ex";
 
@@ -122,4 +152,82 @@ where
         }
         Ok(())
     }
+
+    /// Gleam files are compiled to `.mjs` files, which must not conflict with
+    /// an FFI `.mjs` file with the same name, so we check for this case here.
+    fn check_for_conflicting_javascript_modules(
+        &mut self,
+        relative_path: &Utf8PathBuf,
+    ) -> Result<(), Error> {
+        let mjs_path = match relative_path.extension() {
+            Some("gleam") => eco_format!("{}", relative_path.with_extension("mjs")),
+            Some("mjs") => eco_format!("{}", relative_path),
+            _ => return Ok(()),
+        };
+
+        // Insert the full relative `.mjs` path in `seen_modules` as there is
+        // no conflict if two `.mjs` files have the same name but are in
+        // different subpaths, unlike Erlang files.
+        if let Some(existing) = self
+            .seen_modules
+            .insert(mjs_path.clone(), relative_path.clone())
+        {
+            let existing_is_gleam = existing.extension() == Some("gleam");
+            return Err(
+                if existing_is_gleam || relative_path.extension() == Some("gleam") {
+                    let (gleam_file, native_file) = if existing_is_gleam {
+                        (&existing, relative_path)
+                    } else {
+                        (relative_path, &existing)
+                    };
+                    Error::ClashingGleamModuleAndNativeFileName {
+                        module: eco_format!("{}", gleam_file.with_extension("")),
+                        gleam_file: gleam_file.clone(),
+                        native_file: native_file.clone(),
+                    }
+                } else {
+                    // The only way for two `.mjs` files to clash is by having
+                    // the exact same path.
+                    assert_eq!(&existing, relative_path);
+                    Error::DuplicateSourceFile {
+                        file: existing.to_string(),
+                    }
+                },
+            );
+        }
+
+        Ok(())
+    }
+
+    /// Erlang module files cannot have the same name regardless of their
+    /// relative positions within the project. Ensure we raise an error if the
+    /// user attempts to create `.erl` files with the same name.
+    fn check_for_conflicting_erlang_modules(
+        &mut self,
+        relative_path: &Utf8PathBuf,
+    ) -> Result<(), Error> {
+        // Ideally we'd check for `.gleam` files here as well. However, it is
+        // actually entirely legitimate to receive precompiled `.erl` files for
+        // each `.gleam` file from Hex, so this would prompt an error for every
+        // package downloaded from Hex, which we do not want.
+        if !matches!(relative_path.extension(), Some("erl")) {
+            return Ok(());
+        }
+
+        // Insert just the `.erl` module filename in `seen_modules` instead of
+        // its full relative path, because `.erl` files with the same name
+        // cause a conflict when targetting Erlang regardless of subpath.
+        let erl_file = relative_path.file_name().expect("path has file name");
+        let erl_string = eco_format!("{}", erl_file);
+
+        if let Some(first) = self.seen_modules.insert(erl_string, relative_path.clone()) {
+            return Err(Error::DuplicateNativeErlangModule {
+                module: eco_format!("{}", relative_path.file_stem().expect("path has file stem")),
+                first,
+                second: relative_path.clone(),
+            });
+        }
+
+        Ok(())
+    }
 }