Skip to content

Commit

Permalink
Get rid of the REPL context - use only one typing context
Browse files Browse the repository at this point in the history
  • Loading branch information
@yannham
yannham committed Jan 24, 2025
1 parent 371d62f commit a75d6cb
Show file tree
Hide file tree
Showing 2 changed files with 73 additions and 152 deletions.
213 changes: 69 additions & 144 deletions core/src/cache.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,11 @@ pub use ast_cache::AstCache;
// - [ ] Handle cyclic imports in the new resolver

use crate::{
bytecode::ast::{self, compat::{ToMainline, ToAst}, Ast, AstAlloc, TryConvert},
bytecode::ast::{
self,
compat::{ToAst, ToMainline},
Ast, AstAlloc, TryConvert,
},
closurize::Closurize as _,
error::{Error, ImportError, ParseError, ParseErrors, TypecheckError},
eval::cache::Cache as EvalCache,
Expand Down Expand Up @@ -975,7 +979,7 @@ impl Caches {

let typecheck_res = self
.asts
.typecheck_repl(
.typecheck(
&mut self.sources,
&mut self.wildcards,
&mut self.terms,
Expand All @@ -991,7 +995,7 @@ impl Caches {

if let Some(id) = id {
self.asts
.add_repl_binding(&mut self.sources, &mut self.import_data, id, file_id);
.add_type_binding(&mut self.sources, &mut self.import_data, id, file_id);
}

done = done || matches!(typecheck_res, CacheOp::Done(_));
Expand Down Expand Up @@ -1312,7 +1316,7 @@ impl Caches {
/// defined through interpolation.
pub fn add_repl_bindings(&mut self, term: &RichTerm) {
self.asts
.add_repl_bindings(&mut self.sources, &mut self.import_data, term);
.add_type_bindings(&mut self.sources, &mut self.import_data, term);
}
}

Expand Down Expand Up @@ -1831,9 +1835,9 @@ pub mod resolvers {
impl<'ast, 'cache, 'input> AstImportResolver<'ast> for AstResolver<'ast, 'cache, 'input> {
fn resolve(
&mut self,
import: &Import,
parent: Option<FileId>,
pos: &TermPos,
_import: &Import,
_parent: Option<FileId>,
_pos: &TermPos,
) -> Result<(ResolvedTerm, Ast<'ast>), ImportError> {
todo!()
}
Expand Down Expand Up @@ -1998,16 +2002,13 @@ mod ast_cache {
/// The initial typing context. It's morally an option (unitialized at first), but we just
/// juse an empty context as a default value.
///
/// This context can be augmented through [AstCache::add_repl_binding] and
/// [AstCache::add_repl_bindings], which is typically used in the REPL to add top-level
/// bindings.
///
/// **Caution**: as for [Self::asts], we have to use a `'static` lifetime here as a place
/// holder, but it isn't static.
type_ctxt: typecheck::Context<'static>,
/// The REPL typing context.
///
/// This is an unsatisfactory hack, but the REPL needs to maintain its own type environment
/// augmented by top-level bindings, and having it separated from the allocator - which is
/// located here in [AstCache] - just doesn't work with borrowing. Instead, we store it
/// there and provides methods to mutate it.
repl_type_ctxt: typecheck::Context<'static>,
}

impl AstCache {
Expand All @@ -2016,19 +2017,19 @@ mod ast_cache {
alloc: AstAlloc::new(),
asts: HashMap::new(),
type_ctxt: typecheck::Context::new(),
repl_type_ctxt: typecheck::Context::new(),
}
}

/// Clears the allocator and the cached ASTs.
pub fn clear(&mut self) {
// We release the memory previously used by the allocator. Note that creating a new
// allocator doesn't require heap allocation, or at worst very few (we just allocate
// empty vectors and arenas, which usually have a capacity of 0 by default), so we
// don't bother with an optional.
//
// **Caution**: TO AVOID ANY UNDEFINED BEHAVIOR (values cached in one form or another
// **Important**: TO AVOID ANY UNDEFINED BEHAVIOR (values cached in one form or another
// that would borrow from the allocator and survive it), WE MAKE SURE TO CLEAR ALL AND
// EVERY FIELDS OF THE CACHE AT ONCE BY REINITIALIZING IT ENTIRELY. Change with care.
// EVERY FIELDS OF THE CACHE AT ONCE BY REINITIALIZING IT ENTIRELY. CHANGE WITH CARE.
std::mem::replace(self, Self::new());

Check failure on line 2033 in core/src/cache.rs

View workflow job for this annotation

GitHub Actions / build-and-test (windows-latest) 

unused return value of `std::mem::replace` that must be used
}

Expand All @@ -2037,13 +2038,22 @@ mod ast_cache {
&self.alloc
}

/// Retrieve the AST associated with a file id.
/// Retrieves a copy of the AST associated with a file id.
pub fn get<'ast>(&'ast self, file_id: &FileId) -> Option<Ast<'ast>> {
self.asts.get(file_id).map(|(ast, _errs)| ast).cloned()
}

/// Retrieve the AST associated with a file id.
fn get2<'ast>(
/// ASTs are stored with the `'static` lifetime. We try as much as possible to *not*
/// manipulate them in this state, but convert them back to a safe and local lifetime
/// associated with `self`.
///
/// This internal function does precisely that: it borrows the allocator and returns a
/// cached AST with an associated lifetime. This keeps the allocator borrowed as long as
/// the resulting AST is alive.
///
/// Note that we need to split `alloc` and `asts` to make this helper flexible enough; we
/// can't just take `self` as a whole.
fn borrows_ast<'ast>(
_alloc: &'ast AstAlloc,
asts: &HashMap<FileId, (Ast<'static>, ParseErrors)>,
file_id: &FileId,
Expand Down Expand Up @@ -2106,45 +2116,6 @@ mod ast_cache {
}
}

// /// Takes a closure that builds an AST node from an allocator, a file ID, and populate the
// /// corresponding entry in the cache with the AST. Returns the previously cached AST, if
// /// any.
// fn insert_with_alloc<'ast, F>(&'ast mut self, file_id: FileId, f: F) -> Option<Ast<'ast>>
// where
// F: for<'a> FnOnce(&'ast AstAlloc) -> Ast<'ast>,
// {
// let ast = f(&self.alloc);
// // Safety: we are transmuting the lifetime of the AST from `'ast` to `'static`. This is
// // unsafe in general, but we never use or leak any `'static` reference. It's just a
// // placeholder. We only store such `Ast<'static>` in `asts`, and return them as `'a`
// // references where `self: 'a` in `get()`.
// //
// // Thus, the `'static` lifetime isn't observable from outsideof `AstCache`.
// let promoted_ast = unsafe { std::mem::transmute::<Ast<'_>, Ast<'static>>(ast) };
// self.asts.insert(file_id, promoted_ast)
// }

// pub(super) fn insert_with_result<'ast, F, T, E>(
// &'ast mut self,
// file_id: FileId,
// f: F,
// ) -> Result<T, E>
// where
// F: for<'a> FnOnce(&'ast AstAlloc) -> Result<(Ast<'ast>, T), E>,
// {
// let (ast, result) = f(&self.alloc)?;
// // Safety: we are transmuting the lifetime of the AST from `'ast` to `'static`. This is
// // unsafe in general, but we never use or leak any `'static` reference. It's just a
// // placeholder. We only store such `Ast<'static>` in `asts`, and return them as `'a`
// // references where `self: 'a` in `get()`.
// //
// // Thus, the `'static` lifetime isn't observable from outsideof `AstCache`.
// let promoted_ast = unsafe { std::mem::transmute::<Ast<'_>, Ast<'static>>(ast) };
// let _ = self.asts.insert(file_id, promoted_ast);
//
// Ok(result)
// }

pub fn parse_nickel<'ast>(
&'ast mut self,
file_id: FileId,
Expand Down Expand Up @@ -2200,27 +2171,6 @@ mod ast_cache {
self.asts.remove(&file_id)
}

/// Same as [Self::typecheck], but use the special REPL typing context.
pub fn typecheck_repl<'ast, 'input>(
&'ast mut self,
sources: &'input mut SourceCache,
wildcards: &mut WildcardsCache,
terms: &mut TermCache,
import_data: &mut ImportData,
file_id: FileId,
initial_mode: TypecheckMode,
) -> Result<CacheOp<()>, CacheError<TypecheckError>> {
self.typecheck_(
sources,
wildcards,
terms,
import_data,
file_id,
initial_mode,
true,
)
}

/// Typecheck an entry of the cache and update its state accordingly, or do nothing if the
/// entry has already been typechecked. Require that the corresponding source has been parsed.
/// If the source contains imports, recursively typecheck on the imports too.
Expand All @@ -2239,36 +2189,15 @@ mod ast_cache {
import_data: &mut ImportData,
file_id: FileId,
initial_mode: TypecheckMode,
) -> Result<CacheOp<()>, CacheError<TypecheckError>> {
self.typecheck_(
sources,
wildcards,
terms,
import_data,
file_id,
initial_mode,
false,
)
}

fn typecheck_<'ast, 'input>(
&'ast mut self,
sources: &'input mut SourceCache,
wildcards: &mut WildcardsCache,
terms: &mut TermCache,
import_data: &mut ImportData,
file_id: FileId,
initial_mode: TypecheckMode,
use_repl_context: bool,
) -> Result<CacheOp<()>, CacheError<TypecheckError>> {
if let Some(EntryState::Typechecked) = terms.entry_state(file_id) {
return Ok(CacheOp::Cached(()));
}

// Ensure the initial typing context is properly initialized.
self.populate_type_ctxts(sources);
self.populate_type_ctxt(sources);

let Some(ast) = Self::get2(&self.alloc, &self.asts, &file_id) else {
let Some(ast) = Self::borrows_ast(&self.alloc, &self.asts, &file_id) else {
return Err(CacheError::NotParsed);
};

Expand All @@ -2280,11 +2209,7 @@ mod ast_cache {
sources,
};

let type_ctxt = if use_repl_context {
Self::type_ctxt2(&self.alloc, &self.repl_type_ctxt)
} else {
Self::type_ctxt2(&self.alloc, &self.type_ctxt)
};
let type_ctxt = Self::type_ctxt2(&self.alloc, &self.type_ctxt);

let wildcards_map = measure_runtime!(
"runtime:type_check",
Expand Down Expand Up @@ -2327,7 +2252,7 @@ mod ast_cache {
import_data: &mut ImportData,
) -> Result<CacheOp<()>, CacheError<TypecheckError>> {
let mut ret = CacheOp::Cached(());
self.populate_type_ctxts(sources);
self.populate_type_ctxt(sources);

for (_, stdlib_module_id) in sources.stdlib_modules() {
let result = self.typecheck(
Expand All @@ -2347,6 +2272,8 @@ mod ast_cache {
Ok(ret)
}

/// Typechecks a file (if it wasn't already) and returns the inferred type, with type
/// wildcards properly substituted.
pub fn type_of<'ast, 'input>(
&'ast mut self,
sources: &'input mut SourceCache,
Expand All @@ -2360,7 +2287,7 @@ mod ast_cache {
};

if state < EntryState::Typechecked {
self.typecheck_repl(
self.typecheck(
sources,
wildcards,
terms,
Expand All @@ -2383,7 +2310,7 @@ mod ast_cache {
sources,
};

let type_ctxt = Self::type_ctxt2(&self.alloc, &self.repl_type_ctxt);
let type_ctxt = Self::type_ctxt2(&self.alloc, &self.type_ctxt);

let typ: ast::typ::Type<'_> = TryConvert::try_convert(
&self.alloc,
Expand Down Expand Up @@ -2413,17 +2340,20 @@ mod ast_cache {
))
}

/// If the type contexts (both normal and REPL) haven't been created yet, generate and
/// cache the initial typing context from the list of `file_ids` corresponding to the
/// standard library parts. Otherwise, do nothing.
fn populate_type_ctxts(&mut self, sources: &SourceCache) {
if self.type_ctxt.is_empty() {
let stdlib_terms_vec: Vec<(StdlibModule, Ast<'_>)> = sources
/// If the type context hasn't been created yet, generate and cache the initial typing
/// context from the list of `file_ids` corresponding to the standard library parts.
/// Otherwise, do nothing.
fn populate_type_ctxt(&mut self, sources: &SourceCache) {
if !self.type_ctxt.is_empty() {
return;
}

let stdlib_terms_vec: Vec<(StdlibModule, Ast<'_>)> = sources
.stdlib_modules()
.map(|(module, file_id)| {
(
module,
Self::get2(&self.alloc, &self.asts, &file_id)
Self::borrows_ast(&self.alloc, &self.asts, &file_id)
.expect(
"cache::ast_cache::AstCache::populate_type_ctxt(): can't build environment, stdlib not parsed",
)
Expand All @@ -2432,24 +2362,19 @@ mod ast_cache {
})
.collect();

let ctxt = typecheck::mk_initial_ctxt(&self.alloc, &stdlib_terms_vec).unwrap();
let ctxt = typecheck::mk_initial_ctxt(&self.alloc, &stdlib_terms_vec).unwrap();

// Safety: as for asts, we "forget" the lifetime of the context but it's tied to the
// allocator stored in `self`. It is also correctly reset when the allocator is reset
// (upon [Self::clear]).
self.type_ctxt = unsafe {
std::mem::transmute::<typecheck::Context<'_>, typecheck::Context<'static>>(ctxt)
};
}

if self.repl_type_ctxt.is_empty() {
self.repl_type_ctxt = self.type_ctxt.clone();
}
// Safety: as for asts, we "forget" the lifetime of the context but it's tied to the
// allocator stored in `self`. It is also correctly reset when the allocator is reset
// (upon [Self::clear]).
self.type_ctxt = unsafe {
std::mem::transmute::<typecheck::Context<'_>, typecheck::Context<'static>>(ctxt)
};
}

/// Adds a binding to the REPL type environment. The bound term is identified by its file id
/// Adds a binding to the type environment. The bound term is identified by its file id
/// `file_id`.
pub fn add_repl_binding(
pub fn add_type_binding(
&mut self,
sources: &mut SourceCache,
import_data: &mut ImportData,
Expand All @@ -2468,26 +2393,26 @@ mod ast_cache {
sources,
};

let repl_type_ctxt = Self::type_ctxt2_mut(&self.alloc, &mut self.repl_type_ctxt);
let type_ctxt = Self::type_ctxt2_mut(&self.alloc, &mut self.type_ctxt);

typecheck::env_add(
&self.alloc,
&mut repl_type_ctxt.type_env,
&mut type_ctxt.type_env,
id,
ast,
&repl_type_ctxt.term_env,
&type_ctxt.term_env,
&resolver,
);

self.repl_type_ctxt.term_env.0.insert(
id.ident(),
(ast.clone(), self.repl_type_ctxt.term_env.clone()),
);
self.type_ctxt
.term_env
.0
.insert(id.ident(), (ast.clone(), self.type_ctxt.term_env.clone()));
}

/// Add the bindings of a record to the REPL type environment. Ignore fields whose name are
/// Add the bindings of a record to the type environment. Ignore fields whose name are
/// defined through interpolation.
pub fn add_repl_bindings(
pub fn add_type_bindings(
&mut self,
sources: &mut SourceCache,
import_data: &mut ImportData,
Expand All @@ -2505,13 +2430,13 @@ mod ast_cache {
sources,
};

let repl_type_ctxt = Self::type_ctxt2_mut(&self.alloc, &mut self.repl_type_ctxt);
let type_ctxt = Self::type_ctxt2_mut(&self.alloc, &mut self.type_ctxt);

typecheck::env_add_term(

Check failure on line 2435 in core/src/cache.rs

View workflow job for this annotation

GitHub Actions / build-and-test (windows-latest) 

unused `std::result::Result` that must be used
&self.alloc,
&mut repl_type_ctxt.type_env,
&mut type_ctxt.type_env,
ast,
&repl_type_ctxt.term_env,
&type_ctxt.term_env,
&resolver,
);
}
Expand Down
Loading

0 comments on commit a75d6cb

Please sign in to comment.