Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix and clarify cache file handling #4325

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Fix and clarify cache file handling #4325

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
1ede1ac
Add module_erlang_name and use it where relevant
sbergen Mar 9, 2025
3590912
Strongly typed Gleam source files
sbergen Mar 9, 2025
336184b
Strongly typed cache file loading
sbergen Mar 9, 2025
79b0da5
Reduce duplicated logic in ModuleLoader
sbergen Mar 9, 2025
aefbff8
Move module_name to CacheFile (to be fixed later)
sbergen Mar 9, 2025
8c8e22d
Get rid of duplicate file name mangling
sbergen Mar 9, 2025
60dc5c8
Polish & optimize
sbergen Mar 9, 2025
0552652
Fix nested module deletion detection
sbergen Mar 9, 2025
66e6783
Delete cache files if module is removed
sbergen Mar 9, 2025
706b475
Self-review
sbergen Mar 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion compiler-core/src/ast.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ pub use self::untyped::{FunctionLiteralKind, UntypedExpr};
pub use self::constant::{Constant, TypedConstant, UntypedConstant};

use crate::analyse::Inferred;
use crate::build::{Located, Target};
use crate::build::{Located, Target, module_erlang_name};
use crate::parse::SpannedString;
use crate::type_::error::VariableOrigin;
use crate::type_::expression::Implementations;
Expand Down Expand Up @@ -52,6 +52,12 @@ pub struct Module<Info, Statements> {
pub names: Names,
}

impl<Info, Statements> Module<Info, Statements> {
pub fn erlang_name(&self) -> EcoString {
module_erlang_name(&self.name)
}
}

impl TypedModule {
pub fn find_node(&self, byte_index: u32) -> Option<Located<'_>> {
self.definitions
Expand Down
14 changes: 11 additions & 3 deletions compiler-core/src/build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,10 +246,14 @@ pub struct Module {
}

impl Module {
pub fn erlang_name(&self) -> EcoString {
module_erlang_name(&self.name)
}

pub fn compiled_erlang_path(&self) -> Utf8PathBuf {
let mut path = self.name.replace("/", "@");
path.push_str(".erl");
Utf8PathBuf::from(path.as_ref())
let mut path = Utf8PathBuf::from(&module_erlang_name(&self.name));
assert!(path.set_extension("erl"), "Couldn't set file extension");
path
}

pub fn is_test(&self) -> bool {
Expand Down Expand Up @@ -321,6 +325,10 @@ impl Module {
}
}

pub fn module_erlang_name(gleam_name: &EcoString) -> EcoString {
gleam_name.replace("/", "@")
}

#[derive(Debug, Clone, PartialEq)]
pub struct UnqualifiedImport<'a> {
pub name: &'a EcoString,
Expand Down
35 changes: 15 additions & 20 deletions compiler-core/src/build/module_loader.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ use serde::{Deserialize, Serialize};

use super::{
Mode, Origin, SourceFingerprint, Target,
package_compiler::{CacheMetadata, CachedModule, Input, UncompiledModule, module_name},
package_loader::CodegenRequired,
package_compiler::{CacheMetadata, CachedModule, Input, UncompiledModule},
package_loader::{CodegenRequired, GleamFile},
};
use crate::{
Error, Result,
Expand All @@ -28,7 +28,6 @@ pub(crate) struct ModuleLoader<'a, IO> {
pub target: Target,
pub codegen: CodegenRequired,
pub package_name: &'a EcoString,
pub source_directory: &'a Utf8Path,
pub artefact_directory: &'a Utf8Path,
pub origin: Origin,
/// The set of modules that have had partial compilation done since the last
Expand All @@ -48,14 +47,13 @@ where
/// Whether the module has changed or not is determined by comparing the
/// modification time of the source file with the value recorded in the
/// `.timestamp` file in the artefact directory.
pub fn load(&self, path: Utf8PathBuf) -> Result<Input> {
let name = module_name(self.source_directory, &path);
let artefact = name.replace("/", "@");
let source_mtime = self.io.modification_time(&path)?;
pub fn load(&self, file: GleamFile) -> Result<Input> {
let name = file.module_name.clone();
let source_mtime = self.io.modification_time(&file.path)?;

let read_source = |name| self.read_source(path, name, source_mtime);
let read_source = |name| self.read_source(file.path.clone(), name, source_mtime);
Comment on lines +50 to +54
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't spend too much time trying to get rid of these two clones, but there might be something we could still optimize. I thought I'd get overall feedback first before going too detailed.


let meta = match self.read_cache_metadata(&artefact)? {
let meta = match self.read_cache_metadata(&file)? {
Some(meta) => meta,
None => return read_source(name).map(Input::New),
};
Expand Down Expand Up @@ -84,16 +82,13 @@ where
}
}

Ok(Input::Cached(self.cached(name, meta)))
Ok(Input::Cached(self.cached(file, meta)))
}

/// Read the timestamp file from the artefact directory for the given
/// artefact slug. If the file does not exist, return `None`.
fn read_cache_metadata(&self, artefact: &str) -> Result<Option<CacheMetadata>> {
let meta_path = self
.artefact_directory
.join(artefact)
.with_extension("cache_meta");
/// Read the cache metadata file from the artefact directory for the given
/// source file. If the file does not exist, return `None`.
fn read_cache_metadata(&self, source_file: &GleamFile) -> Result<Option<CacheMetadata>> {
let meta_path = source_file.cache_files(&self.artefact_directory).meta_path;

if !self.io.is_file(&meta_path) {
return Ok(None);
Expand Down Expand Up @@ -129,12 +124,12 @@ where
)
}

fn cached(&self, name: EcoString, meta: CacheMetadata) -> CachedModule {
fn cached(&self, file: GleamFile, meta: CacheMetadata) -> CachedModule {
CachedModule {
dependencies: meta.dependencies,
source_path: self.source_directory.join(format!("{}.gleam", name)),
source_path: file.path,
origin: self.origin,
name,
name: file.module_name,
line_numbers: meta.line_numbers,
}
}
Expand Down
74 changes: 27 additions & 47 deletions compiler-core/src/build/module_loader/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,193 +9,175 @@ use std::time::Duration;
#[test]
fn no_cache_present() {
let name = "package".into();
let src = Utf8Path::new("/src");
let artefact = Utf8Path::new("/artefact");
let fs = InMemoryFileSystem::new();
let warnings = WarningEmitter::null();
let incomplete_modules = HashSet::new();
let loader = make_loader(&warnings, &name, &fs, src, artefact, &incomplete_modules);
let loader = make_loader(&warnings, &name, &fs, artefact, &incomplete_modules);

fs.write(&Utf8Path::new("/src/main.gleam"), "const x = 1")
.unwrap();

let result = loader
.load(Utf8Path::new("/src/main.gleam").to_path_buf())
.unwrap();
let file = GleamFile::new("/src".into(), "/src/main.gleam".into());
let result = loader.load(file).unwrap();

assert!(result.is_new());
}

#[test]
fn cache_present_and_fresh() {
let name = "package".into();
let src = Utf8Path::new("/src");
let artefact = Utf8Path::new("/artefact");
let fs = InMemoryFileSystem::new();
let warnings = WarningEmitter::null();
let incomplete_modules = HashSet::new();
let loader = make_loader(&warnings, &name, &fs, src, artefact, &incomplete_modules);
let loader = make_loader(&warnings, &name, &fs, artefact, &incomplete_modules);

// The mtime of the source is older than that of the cache
write_src(&fs, TEST_SOURCE_1, "/src/main.gleam", 0);
write_cache(&fs, TEST_SOURCE_1, "/artefact/main.cache_meta", 1, false);

let result = loader
.load(Utf8Path::new("/src/main.gleam").to_path_buf())
.unwrap();
let file = GleamFile::new("/src".into(), "/src/main.gleam".into());
let result = loader.load(file).unwrap();

assert!(result.is_cached());
}

#[test]
fn cache_present_and_stale() {
let name = "package".into();
let src = Utf8Path::new("/src");
let artefact = Utf8Path::new("/artefact");
let fs = InMemoryFileSystem::new();
let warnings = WarningEmitter::null();
let incomplete_modules = HashSet::new();
let loader = make_loader(&warnings, &name, &fs, src, artefact, &incomplete_modules);
let loader = make_loader(&warnings, &name, &fs, artefact, &incomplete_modules);

// The mtime of the source is newer than that of the cache
write_src(&fs, TEST_SOURCE_2, "/src/main.gleam", 2);
write_cache(&fs, TEST_SOURCE_1, "/artefact/main.cache_meta", 1, false);

let result = loader
.load(Utf8Path::new("/src/main.gleam").to_path_buf())
.unwrap();
let file = GleamFile::new("/src".into(), "/src/main.gleam".into());
let result = loader.load(file).unwrap();

assert!(result.is_new());
}

#[test]
fn cache_present_and_stale_but_source_is_the_same() {
let name = "package".into();
let src = Utf8Path::new("/src");
let artefact = Utf8Path::new("/artefact");
let fs = InMemoryFileSystem::new();
let warnings = WarningEmitter::null();
let incomplete_modules = HashSet::new();
let loader = make_loader(&warnings, &name, &fs, src, artefact, &incomplete_modules);
let loader = make_loader(&warnings, &name, &fs, artefact, &incomplete_modules);

// The mtime of the source is newer than that of the cache
write_src(&fs, TEST_SOURCE_1, "/src/main.gleam", 2);
write_cache(&fs, TEST_SOURCE_1, "/artefact/main.cache_meta", 1, false);

let result = loader
.load(Utf8Path::new("/src/main.gleam").to_path_buf())
.unwrap();
let file = GleamFile::new("/src".into(), "/src/main.gleam".into());
let result = loader.load(file).unwrap();

assert!(result.is_cached());
}

#[test]
fn cache_present_and_stale_source_is_the_same_lsp_mode() {
let name = "package".into();
let src = Utf8Path::new("/src");
let artefact = Utf8Path::new("/artefact");
let fs = InMemoryFileSystem::new();
let warnings = WarningEmitter::null();
let incomplete_modules = HashSet::new();
let mut loader = make_loader(&warnings, &name, &fs, src, artefact, &incomplete_modules);
let mut loader = make_loader(&warnings, &name, &fs, artefact, &incomplete_modules);
loader.mode = Mode::Lsp;

// The mtime of the source is newer than that of the cache
write_src(&fs, TEST_SOURCE_1, "/src/main.gleam", 2);
write_cache(&fs, TEST_SOURCE_1, "/artefact/main.cache_meta", 1, false);

let result = loader
.load(Utf8Path::new("/src/main.gleam").to_path_buf())
.unwrap();
let file = GleamFile::new("/src".into(), "/src/main.gleam".into());
let result = loader.load(file).unwrap();

assert!(result.is_cached());
}

#[test]
fn cache_present_and_stale_source_is_the_same_lsp_mode_and_invalidated() {
let name = "package".into();
let src = Utf8Path::new("/src");
let artefact = Utf8Path::new("/artefact");
let fs = InMemoryFileSystem::new();
let warnings = WarningEmitter::null();
let mut incomplete_modules = HashSet::new();
let _ = incomplete_modules.insert("main".into());
let mut loader = make_loader(&warnings, &name, &fs, src, artefact, &incomplete_modules);
let mut loader = make_loader(&warnings, &name, &fs, artefact, &incomplete_modules);
loader.mode = Mode::Lsp;

// The mtime of the source is newer than that of the cache
write_src(&fs, TEST_SOURCE_1, "/src/main.gleam", 2);
write_cache(&fs, TEST_SOURCE_1, "/artefact/main.cache_meta", 1, false);

let result = loader
.load(Utf8Path::new("/src/main.gleam").to_path_buf())
.unwrap();
let file = GleamFile::new("/src".into(), "/src/main.gleam".into());
let result = loader.load(file).unwrap();

assert!(result.is_new());
}

#[test]
fn cache_present_without_codegen_when_required() {
let name = "package".into();
let src = Utf8Path::new("/src");
let artefact = Utf8Path::new("/artefact");
let fs = InMemoryFileSystem::new();
let warnings = WarningEmitter::null();
let incomplete_modules = HashSet::new();
let mut loader = make_loader(&warnings, &name, &fs, src, artefact, &incomplete_modules);
let mut loader = make_loader(&warnings, &name, &fs, artefact, &incomplete_modules);
loader.codegen = CodegenRequired::Yes;

// The mtime of the cache is newer than that of the source
write_src(&fs, TEST_SOURCE_1, "/src/main.gleam", 0);
write_cache(&fs, TEST_SOURCE_1, "/artefact/main.cache_meta", 1, false);

let result = loader
.load(Utf8Path::new("/src/main.gleam").to_path_buf())
.unwrap();
let file = GleamFile::new("/src".into(), "/src/main.gleam".into());
let result = loader.load(file).unwrap();

assert!(result.is_new());
}

#[test]
fn cache_present_with_codegen_when_required() {
let name = "package".into();
let src = Utf8Path::new("/src");
let artefact = Utf8Path::new("/artefact");
let fs = InMemoryFileSystem::new();
let warnings = WarningEmitter::null();
let incomplete_modules = HashSet::new();
let mut loader = make_loader(&warnings, &name, &fs, src, artefact, &incomplete_modules);
let mut loader = make_loader(&warnings, &name, &fs, artefact, &incomplete_modules);
loader.codegen = CodegenRequired::Yes;

// The mtime of the cache is newer than that of the source
write_src(&fs, TEST_SOURCE_1, "/src/main.gleam", 0);
write_cache(&fs, TEST_SOURCE_1, "/artefact/main.cache_meta", 1, true);

let result = loader
.load(Utf8Path::new("/src/main.gleam").to_path_buf())
.unwrap();
let file = GleamFile::new("/src".into(), "/src/main.gleam".into());
let result = loader.load(file).unwrap();

assert!(result.is_cached());
}

#[test]
fn cache_present_without_codegen_when_not_required() {
let name = "package".into();
let src = Utf8Path::new("/src");
let artefact = Utf8Path::new("/artefact");
let fs = InMemoryFileSystem::new();
let warnings = WarningEmitter::null();
let incomplete_modules = HashSet::new();
let mut loader = make_loader(&warnings, &name, &fs, src, artefact, &incomplete_modules);
let mut loader = make_loader(&warnings, &name, &fs, artefact, &incomplete_modules);
loader.codegen = CodegenRequired::No;

// The mtime of the cache is newer than that of the source
write_src(&fs, TEST_SOURCE_1, "/src/main.gleam", 0);
write_cache(&fs, TEST_SOURCE_1, "/artefact/main.cache_meta", 1, false);

let result = loader
.load(Utf8Path::new("/src/main.gleam").to_path_buf())
.unwrap();
let file = GleamFile::new("/src".into(), "/src/main.gleam".into());
let result = loader.load(file).unwrap();

assert!(result.is_cached());
}
Expand Down Expand Up @@ -232,7 +214,6 @@ fn make_loader<'a>(
warnings: &'a WarningEmitter,
package_name: &'a EcoString,
fs: &InMemoryFileSystem,
src: &'a Utf8Path,
artefact: &'a Utf8Path,
incomplete_modules: &'a HashSet<EcoString>,
) -> ModuleLoader<'a, InMemoryFileSystem> {
Expand All @@ -243,7 +224,6 @@ fn make_loader<'a>(
target: Target::Erlang,
codegen: CodegenRequired::No,
package_name,
source_directory: &src,
artefact_directory: &artefact,
origin: Origin::Src,
incomplete_modules,
Expand Down
Loading
Loading