Skip to content

Commit

Permalink
Fix regression in module requiring (#137)
Browse files Browse the repository at this point in the history
  • Loading branch information
@mattwparas
mattwparas authored Jan 17, 2024
1 parent eb9bc4c commit e14d5ae
Show file tree
Hide file tree
Showing 11 changed files with 131 additions and 51 deletions.
11 changes: 0 additions & 11 deletions build.sh
View file

This file was deleted.

4 changes: 4 additions & 0 deletions cogs/module-tests/export.scm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
1 change: 1 addition & 0 deletions cogs/module-tests/require.scm
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
(require "import.scm")
8 changes: 8 additions & 0 deletions crates/steel-core/src/compiler/compiler.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
65 changes: 48 additions & 17 deletions crates/steel-core/src/compiler/modules.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down
50 changes: 28 additions & 22 deletions crates/steel-core/src/compiler/passes/analysis.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<InternedString, ScopeInfo> = 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
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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!()

Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;
}
Expand All @@ -4016,6 +4021,7 @@ impl<'a> SemanticAnalysis<'a> {
if *func == module_get_interned
|| *func == proto_hash_get
{
// println!("REMOVING: {}", define);
found = true;
break;
}
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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);
Expand Down
4 changes: 4 additions & 0 deletions crates/steel-core/src/env.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion crates/steel-core/src/scheme/modules/parameters.scm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@

; (displayln winders)

(displayln "catching exception here")
; (displayln "catching exception here")

(set! winders (cdr winders))
(out)
Expand Down
29 changes: 29 additions & 0 deletions crates/steel-core/src/steel_vm/engine.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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<Vec<SteelVal>> {
let executable = self.raw_program_to_executable(program)?;

Expand Down
4 changes: 4 additions & 0 deletions crates/steel-core/src/steel_vm/primitives.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
4 changes: 4 additions & 0 deletions crates/steel-core/src/steel_vm/vm.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4443,6 +4443,10 @@ pub(crate) fn list_modules(ctx: &mut VmCore, _args: &[SteelVal]) -> Option<Resul
Some(Ok(SteelVal::ListV(modules)))
}

pub(crate) fn environment_offset(ctx: &mut VmCore, args: &[SteelVal]) -> Option<Result<SteelVal>> {
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.
Expand Down

0 comments on commit e14d5ae

Please sign in to comment.