From e14d5ae2d5ee05226b49fe9e9741da2152795ef7 Mon Sep 17 00:00:00 2001 From: Matthew Paras <34500476+mattwparas@users.noreply.github.com> Date: Tue, 16 Jan 2024 18:19:39 -0800 Subject: [PATCH] Fix regression in module requiring (#137) --- build.sh | 11 ---- cogs/module-tests/export.scm | 4 ++ cogs/module-tests/require.scm | 1 + crates/steel-core/src/compiler/compiler.rs | 8 +++ crates/steel-core/src/compiler/modules.rs | 65 ++++++++++++++----- .../src/compiler/passes/analysis.rs | 50 +++++++------- crates/steel-core/src/env.rs | 4 ++ .../src/scheme/modules/parameters.scm | 2 +- crates/steel-core/src/steel_vm/engine.rs | 29 +++++++++ crates/steel-core/src/steel_vm/primitives.rs | 4 ++ crates/steel-core/src/steel_vm/vm.rs | 4 ++ 11 files changed, 131 insertions(+), 51 deletions(-) delete mode 100644 build.sh create mode 100644 cogs/module-tests/require.scm diff --git a/build.sh b/build.sh deleted file mode 100644 index 3a83e2956..000000000 --- a/build.sh +++ /dev/null @@ -1,11 +0,0 @@ -cd crates/ - -cd steel-sys-info && cargo-steel-lib && cd .. - -# cd steel-toml && cargo-steel-lib && cd .. -# cd steel-webserver && cargo-steel-lib && cd .. -# cd example-dylib && cargo-steel-lib && cd .. - -cargo build - -cd .. \ No newline at end of file diff --git a/cogs/module-tests/export.scm b/cogs/module-tests/export.scm index 225870eb9..83da8b289 100644 --- a/cogs/module-tests/export.scm +++ b/cogs/module-tests/export.scm @@ -14,9 +14,13 @@ (define (bananas) (error "Hello world")) +;;@doc +;; Does this show up? (define (foo-bar-baz) 10) +;;@doc +;; How does this get rendered? (define/contract (my-fun-contracted-function x y) (->/c int? int? int?) (+ x y)) diff --git a/cogs/module-tests/require.scm b/cogs/module-tests/require.scm new file mode 100644 index 000000000..f36c18751 --- /dev/null +++ b/cogs/module-tests/require.scm @@ -0,0 +1 @@ +(require "import.scm") diff --git a/crates/steel-core/src/compiler/compiler.rs b/crates/steel-core/src/compiler/compiler.rs index 889b62d10..1119aa0f2 100644 --- a/crates/steel-core/src/compiler/compiler.rs +++ b/crates/steel-core/src/compiler/compiler.rs @@ -866,6 +866,14 @@ impl Compiler { // let mut analysis = Analysis::from_exprs(&expanded_statements); analysis.populate_captures(&expanded_statements); + // let mut semantic = SemanticAnalysis::from_analysis(&mut expanded_statements, analysis); + + // println!("MARKER HERE--------------------------"); + + // expanded_statements.pretty_print(); + + // println!("END MARKER---------------------------"); + let mut semantic = SemanticAnalysis::from_analysis(&mut expanded_statements, analysis); // This is definitely broken still diff --git a/crates/steel-core/src/compiler/modules.rs b/crates/steel-core/src/compiler/modules.rs index 14dee66fc..3fd260005 100644 --- a/crates/steel-core/src/compiler/modules.rs +++ b/crates/steel-core/src/compiler/modules.rs @@ -1449,31 +1449,62 @@ impl CompiledModule { SyntaxObject::default(TokenType::Quote), ))); - let mut offset = None; + // let mut offset = None; // Find offset of first non builtin require definition: - for (idx, expr) in exprs.iter().enumerate() { - if let ExprKind::Define(d) = expr { - if !is_a_builtin_definition(d) { - // println!("Found offset at: {:?}", offset); + // for (idx, expr) in exprs.iter().enumerate() { + // if let ExprKind::Define(d) = expr { + // // if !is_a_builtin_definition(d) || !is_a_require_definition(d) { + // if !is_a_builtin_definition(d) { + // // println!("Found offset at: {:?}", offset); + + // offset = Some(idx); + // println!("Found offset at: {:?}", offset); + // break; + // } + // } + // } + + exprs.push(module_define); + + // exprs.append(&mut provide_definitions); + + let mut builtin_definitions = Vec::new(); - offset = Some(idx); - break; + exprs.retain_mut(|expr| { + if let ExprKind::Define(d) = expr { + if is_a_builtin_definition(d) { + builtin_definitions.push(std::mem::take(expr)); + false + } else { + true } + } else { + true } - } + }); - exprs.push(module_define); + builtin_definitions.append(&mut provide_definitions); + builtin_definitions.append(&mut exprs); - if let Some(offset) = offset { - for (idx, expr) in provide_definitions.into_iter().enumerate() { - exprs.insert(offset + idx, expr); - } - } else { - provide_definitions.append(&mut exprs); - } + // provide_definitions.append(&mut builtin_definitions); + // provide_definitions.append(&mut exprs); + + exprs = builtin_definitions; + + // if let Some(offset) = offset { + // for (idx, expr) in provide_definitions.into_iter().enumerate() { + // exprs.insert(offset + idx, expr); + // } + // } else { + // provide_definitions.append(&mut exprs); + // } + + // println!("MODULE DEFINITIONS----"); + + // exprs.pretty_print(); - // println!("------ {}", module_define.to_pretty(60)); + // println!("END MODULE DEFINITIONS"); // exprs.pretty_print(); diff --git a/crates/steel-core/src/compiler/passes/analysis.rs b/crates/steel-core/src/compiler/passes/analysis.rs index 392ce9d6f..673c75ab3 100644 --- a/crates/steel-core/src/compiler/passes/analysis.rs +++ b/crates/steel-core/src/compiler/passes/analysis.rs @@ -493,6 +493,7 @@ impl Analysis { // TODO: This needs to just take an iterator? pub fn run(&mut self, exprs: &[ExprKind]) { // let mut scope: ScopeMap = ScopeMap::new(); + self.scope.clear_all(); // TODO: Functions should be globally resolvable but top level identifiers cannot be used before they are defined // The way this is implemented right now doesn't respect that @@ -564,16 +565,16 @@ impl Analysis { } pub fn insert(&mut self, object: &SyntaxObject, metadata: SemanticInformation) { - // self.info.insert(object.syntax_object_id, metadata); - - match self.info.entry(object.syntax_object_id) { - hash_map::Entry::Occupied(m) => { - *m.into_mut() = metadata; - } - hash_map::Entry::Vacant(e) => { - e.insert(metadata); - } - } + self.info.insert(object.syntax_object_id, metadata); + + // match self.info.entry(object.syntax_object_id) { + // hash_map::Entry::Occupied(m) => { + // *m.into_mut() = metadata; + // } + // hash_map::Entry::Vacant(e) => { + // e.insert(metadata); + // } + // } // if let Some(existing_metadata) = self.info.get_mut(&object.syntax_object_id) { // *existing_metadata = metadata; @@ -849,7 +850,6 @@ impl<'a> AnalysisPass<'a> { // If this variable name is already in scope, we should mark that this variable // shadows the previous id if let Some(shadowed_var) = self.info.scope.get(name) { - // log::debug!("Redefining previous variable: {:?}", name); semantic_info = semantic_info.shadows(shadowed_var.id); } @@ -1949,7 +1949,6 @@ impl<'a> VisitorMutUnitRef<'a> for AnalysisPass<'a> { // TODO: We _really_ should be providing the built-ins in a better way thats not // passing around a thread local // if crate::steel_vm::primitives::PRELUDE_MODULE.with(|x| x.contains(ident.resolve())) - if crate::steel_vm::primitives::PRELUDE_INTERNED_STRINGS.with(|x| x.contains(ident)) { semantic_info.mark_builtin(); @@ -3293,8 +3292,8 @@ impl<'a> VisitorMutRefUnit for ReplaceBuiltinUsagesWithReservedPrimitiveReferenc // .map(|x| x.resolve().ends_with("get-test-mode")) // .unwrap_or_default() // { - // println!("Visiting: {}", a); - // println!("{:#?}", info); + // println!("Visiting: {}", a); + // println!("{:#?}", info); // } if info.builtin && !info.is_shadowed && info.shadows.is_none() { @@ -3322,6 +3321,10 @@ impl<'a> VisitorMutRefUnit for ReplaceBuiltinUsagesWithReservedPrimitiveReferenc if let Some(refers_to) = info.refers_to { if let Some(info) = self.analysis.info.get(&refers_to) { + // println!("Visiting: {}", a); + // println!("{:#?}", info); + // println!("Self id: {}", refers_to); + if info.builtin && !info.is_shadowed && info.shadows.is_none() { // todo!() @@ -3908,9 +3911,9 @@ impl<'a> SemanticAnalysis<'a> { changed: false, }; - if macro_replacer.identifiers_to_replace.is_empty() { - return self; - } + // if macro_replacer.identifiers_to_replace.is_empty() { + // return self; + // } for steel_macro in macros.values_mut() { if !steel_macro.is_mangled() { @@ -3945,8 +3948,8 @@ impl<'a> SemanticAnalysis<'a> { ); // if macro_replacer.changed || !macro_replacer.identifiers_to_replace.is_empty() { - // log::info!(target: "pipeline_time", "Skipping analysis..."); - // self.analysis.fresh_from_exprs(self.exprs); + // log::info!(target: "pipeline_time", "Skipping analysis..."); + self.analysis.fresh_from_exprs(self.exprs); // } self @@ -3990,6 +3993,8 @@ impl<'a> SemanticAnalysis<'a> { if *func == module_get_interned || *func == proto_hash_get { // If this is found inside of a macro, do not remove it + // println!("REMOVING: {}", define); + found = true; break; } @@ -4016,6 +4021,7 @@ impl<'a> SemanticAnalysis<'a> { if *func == module_get_interned || *func == proto_hash_get { + // println!("REMOVING: {}", define); found = true; break; } @@ -4163,10 +4169,10 @@ impl<'a> SemanticAnalysis<'a> { // self.exprs.push(ExprKind::ident("void")); - // log::debug!("Re-running the semantic analysis after removing unused globals"); + log::debug!("Re-running the semantic analysis after removing unused globals"); // Skip running the analysis here? Nothing will have changed? - // self.analysis.fresh_from_exprs(self.exprs); + self.analysis.fresh_from_exprs(self.exprs); self } @@ -4564,7 +4570,7 @@ impl<'a> SemanticAnalysis<'a> { if elider.re_run_analysis { // log::debug!( - // "Re-running the semantic analysis after modifications during lambda lifting" + // "Re-running the semantic analysis after modifications during lambda lifting" // ); self.analysis.fresh_from_exprs(self.exprs); diff --git a/crates/steel-core/src/env.rs b/crates/steel-core/src/env.rs index 27b69ce76..33581bc05 100644 --- a/crates/steel-core/src/env.rs +++ b/crates/steel-core/src/env.rs @@ -11,6 +11,10 @@ impl Env { self.bindings_vec.get(idx).cloned() } + pub fn len(&self) -> usize { + self.bindings_vec.len() + } + /// top level global env has no parent pub fn root() -> Self { Env { diff --git a/crates/steel-core/src/scheme/modules/parameters.scm b/crates/steel-core/src/scheme/modules/parameters.scm index fb82cb2c5..fc4687deb 100644 --- a/crates/steel-core/src/scheme/modules/parameters.scm +++ b/crates/steel-core/src/scheme/modules/parameters.scm @@ -151,7 +151,7 @@ ; (displayln winders) - (displayln "catching exception here") + ; (displayln "catching exception here") (set! winders (cdr winders)) (out) diff --git a/crates/steel-core/src/steel_vm/engine.rs b/crates/steel-core/src/steel_vm/engine.rs index 80ff13f89..b70e76254 100644 --- a/crates/steel-core/src/steel_vm/engine.rs +++ b/crates/steel-core/src/steel_vm/engine.rs @@ -145,6 +145,12 @@ pub struct EngineStatistics { pub sources_size: usize, } +#[derive(Debug, Clone, Copy)] +pub struct GlobalCheckpoint { + symbol_map_offset: usize, + globals_offset: usize, +} + #[derive(Clone)] pub struct Engine { virtual_machine: SteelThread, @@ -1489,6 +1495,29 @@ impl Engine { result } + // TODO: Add doc for this + #[doc(hidden)] + pub fn environment_offset(&self) -> GlobalCheckpoint { + GlobalCheckpoint { + symbol_map_offset: self.compiler.symbol_map.len(), + globals_offset: self.virtual_machine.global_env.len(), + } + } + + // TODO: Add doc for this + #[doc(hidden)] + pub fn rollback_to_checkpoint(&mut self, checkpoint: GlobalCheckpoint) -> Result<()> { + self.compiler + .symbol_map + .roll_back(checkpoint.symbol_map_offset); + self.virtual_machine + .global_env + .bindings_vec + .truncate(checkpoint.globals_offset); + + Ok(()) + } + pub fn run_raw_program(&mut self, program: RawProgramWithSymbols) -> Result> { let executable = self.raw_program_to_executable(program)?; diff --git a/crates/steel-core/src/steel_vm/primitives.rs b/crates/steel-core/src/steel_vm/primitives.rs index 6abf0bf61..2291a8c2a 100644 --- a/crates/steel-core/src/steel_vm/primitives.rs +++ b/crates/steel-core/src/steel_vm/primitives.rs @@ -1571,6 +1571,10 @@ fn meta_module() -> BuiltInModule { SteelVal::BuiltIn(super::vm::call_with_exception_handler), ) .register_value("breakpoint!", SteelVal::BuiltIn(super::vm::breakpoint)) + .register_value( + "#%environment-length", + SteelVal::BuiltIn(super::vm::environment_offset), + ) .register_value( "call-with-current-continuation", SteelVal::BuiltIn(super::vm::call_cc), diff --git a/crates/steel-core/src/steel_vm/vm.rs b/crates/steel-core/src/steel_vm/vm.rs index 067179641..09db9b186 100644 --- a/crates/steel-core/src/steel_vm/vm.rs +++ b/crates/steel-core/src/steel_vm/vm.rs @@ -4443,6 +4443,10 @@ pub(crate) fn list_modules(ctx: &mut VmCore, _args: &[SteelVal]) -> Option Option> { + Some(Ok(ctx.thread.global_env.len().into_steelval().unwrap())) +} + // TODO: This apply does not respect tail position // Something like this: (define (loop) (apply loop '())) // _should_ result in an infinite loop. In the current form, this is a Rust stack overflow.