Skip to content

Commit a356ed7

Browse files
JakkuSakuraclaude
andcommitted
refactor(compiler,cli): drive whole-workspace compiles through the driver instead of a per-package CLI loop
compile_project looped over every workspace member calling typecheck_package, which built a brand-new executor/workspace/session per package -- discarding CompilerDriver::compile_package's own dependency cache every iteration, so std/libc and shared sibling dependencies got recompiled once per package. CompilerDriver::compile_workspace now treats members as dependencies of a synthetic root and reuses compile_package's existing recursive, cached walk in one call, so std is compiled once per workspace and a single member's failure/panic fails the whole compile rather than being silently isolated. Also collapses fp-kotlin's ad hoc 5-tuple + separately-merged map of workspace-wide serialization facts into one KotlinWorkspaceContext struct. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent a3a8a76 commit a356ed7

5 files changed

Lines changed: 264 additions & 154 deletions

File tree

crates/fp-cli/src/commands/compile.rs

Lines changed: 98 additions & 103 deletions
Original file line numberDiff line numberDiff line change
@@ -1003,115 +1003,110 @@ async fn compile_project(
10031003
// to decide `val` vs `var` when emitting the struct, so that has to be
10041004
// computed from every package's fully-processed AST, not just the one
10051005
// currently being serialized.
1006-
let mut prepared: Vec<(PackageId, PackageSource)> = Vec::with_capacity(packages.len());
1006+
//
1007+
// Every member is compiled through ONE `CompilerDriver::compile_workspace`
1008+
// call, which treats the members as if they were dependencies of a
1009+
// synthetic root package and walks them via `compile_package`'s own
1010+
// recursive, cached, cycle-safe dependency machinery — so `std`/`libc`
1011+
// and any inter-member dependency (a workspace path dependency) is
1012+
// compiled exactly once for the whole workspace, not once per member. A
1013+
// typecheck error or panic in any one member fails/falls back the whole
1014+
// workspace compile (not just that member) — a package's own
1015+
// dependencies already work this way (an unresolvable dependency fails
1016+
// its dependent), so treating members the same way needs no special
1017+
// per-member recovery bookkeeping in the shared driver.
1018+
let untyped_prepared = |packages: &[PackageId]| -> Result<Vec<(PackageId, PackageSource)>> {
1019+
packages
1020+
.iter()
1021+
.map(|package_id| {
1022+
materializing_provider
1023+
.load_package_source(package_id)
1024+
.map(|source| (package_id.clone(), source))
1025+
.map_err(|e| CliError::Compilation(e.to_string()))
1026+
})
1027+
.collect()
1028+
};
10071029

1008-
for package_id in &packages {
1009-
// Typecheck: resolve types via HIR to populate AST type slots.
1010-
//
1011-
// Batched by whole *package*, not per-file: a package's `impl SomeType`
1012-
// block routinely lives in a different file than `SomeType`'s own
1013-
// definition (e.g. types.rs defines the struct, other files add impls
1014-
// for it) — typechecking file-by-file makes those siblings invisible
1015-
// to each other, causing spurious "unresolved impl self type" errors
1016-
// for essentially every real multi-file package. Whole-package batching
1017-
// gives the typechecker the full context it needs at the cost of
1018-
// coarser fault isolation (one bad item anywhere in the package falls
1019-
// the *whole* package back to untyped, not just its one file) — still
1020-
// safe either way, since the call is wrapped in `catch_unwind` below.
1021-
let source = if !args.skip_typing {
1022-
let provider_for_typecheck = materializing_provider.clone();
1023-
let package_id_for_typecheck = package_id.clone();
1024-
let lossy = LossyCompileOptions {
1025-
enabled: args.lossy || fp_core::config::lossy_mode(),
1026-
};
1027-
let lang = lang.to_string();
1028-
let capabilities = crate::languages::backend::capabilities_for_target(target);
1029-
match std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
1030-
compiler::typecheck_package(
1031-
provider_for_typecheck,
1032-
&package_id_for_typecheck,
1033-
lossy,
1034-
&lang,
1035-
capabilities,
1036-
)
1037-
})) {
1038-
Ok(Ok(typed_source)) => typed_source,
1039-
Ok(Err(e)) => {
1040-
if !lossy.enabled {
1041-
return Err(CliError::Compilation(format!(
1042-
"typecheck failed for {}: {}",
1043-
package_id.as_str(),
1044-
e
1045-
)));
1046-
}
1047-
warn!(
1048-
"typecheck failed for {}: {} — falling back to untyped (lossy mode)",
1049-
package_id.as_str(),
1030+
let capabilities = crate::languages::backend::capabilities_for_target(target);
1031+
let prepared: Vec<(PackageId, PackageSource)> = if !args.skip_typing {
1032+
let lossy = LossyCompileOptions {
1033+
enabled: args.lossy || fp_core::config::lossy_mode(),
1034+
};
1035+
let root_name = input
1036+
.file_name()
1037+
.and_then(|n| n.to_str())
1038+
.unwrap_or("workspace");
1039+
let root_id = PackageId::new(format!("{root_name}::__workspace_root__"));
1040+
let (executor, mut session) = compiler::build_workspace_session(
1041+
materializing_provider.clone(),
1042+
lang,
1043+
lossy,
1044+
capabilities,
1045+
);
1046+
match std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
1047+
executor.run(session.driver().compile_workspace(&root_id, &packages))
1048+
})) {
1049+
Ok(Ok(compiled)) => {
1050+
compiler::drain_driver(session.driver(), lossy)?;
1051+
packages
1052+
.iter()
1053+
.zip(compiled.iter())
1054+
.map(|(package_id, compiled_package)| {
1055+
(
1056+
package_id.clone(),
1057+
compiler::package_source_from_compiled(package_id, compiled_package),
1058+
)
1059+
})
1060+
.collect()
1061+
}
1062+
Ok(Err(e)) => {
1063+
if !lossy.enabled {
1064+
return Err(CliError::Compilation(format!(
1065+
"typecheck failed for project at {}: {}",
1066+
input.display(),
10501067
e
1051-
);
1052-
materializing_provider
1053-
.load_package_source(package_id)
1054-
.map_err(|e| CliError::Compilation(e.to_string()))?
1068+
)));
10551069
}
1056-
Err(panic_info) => {
1057-
let msg = panic_info
1058-
.downcast_ref::<String>()
1059-
.map(|s| s.as_str())
1060-
.or_else(|| panic_info.downcast_ref::<&str>().copied())
1061-
.unwrap_or("(unknown)");
1062-
if !lossy.enabled {
1063-
return Err(CliError::Compilation(format!(
1064-
"typecheck panicked for {}: {}",
1065-
package_id.as_str(),
1066-
msg
1067-
)));
1068-
}
1069-
warn!(
1070-
"typecheck panicked for {}: {} — falling back to untyped (lossy mode)",
1071-
package_id.as_str(),
1070+
warn!(
1071+
"typecheck failed for project at {}: {} — falling back to untyped (lossy mode)",
1072+
input.display(),
1073+
e
1074+
);
1075+
untyped_prepared(&packages)?
1076+
}
1077+
Err(panic_info) => {
1078+
let msg = panic_info
1079+
.downcast_ref::<String>()
1080+
.map(|s| s.as_str())
1081+
.or_else(|| panic_info.downcast_ref::<&str>().copied())
1082+
.unwrap_or("(unknown)");
1083+
if !lossy.enabled {
1084+
return Err(CliError::Compilation(format!(
1085+
"typecheck panicked for project at {}: {}",
1086+
input.display(),
10721087
msg
1073-
);
1074-
materializing_provider
1075-
.load_package_source(package_id)
1076-
.map_err(|e| CliError::Compilation(e.to_string()))?
1088+
)));
10771089
}
1090+
warn!(
1091+
"typecheck panicked for project at {}: {} — falling back to untyped (lossy mode)",
1092+
input.display(),
1093+
msg
1094+
);
1095+
untyped_prepared(&packages)?
10781096
}
1079-
} else {
1080-
materializing_provider
1081-
.load_package_source(package_id)
1082-
.map_err(|e| CliError::Compilation(e.to_string()))?
1083-
};
1084-
1085-
prepared.push((package_id.clone(), source));
1086-
}
1087-
1088-
// Field mutability (`val` vs `var`) and List-vs-String disambiguation
1089-
// (`.len()` -> `.size` not `.length`, range-index -> `.subList` not
1090-
// `.substring`) are both decided workspace-wide: a struct's fields can
1091-
// be defined in one package and mutated/read from another.
1092-
let (workspace_mutated_fields, workspace_list_fields, workspace_string_fields, workspace_enum_fields, workspace_enum_variant_names) =
1093-
if matches!(target, crate::languages::backend::BuiltinLanguageTarget::Kotlin) {
1094-
(
1095-
fp_kotlin::collect_mutated_field_names(prepared.iter().flat_map(|(_, src)| &src.items)),
1096-
fp_kotlin::collect_list_field_names(prepared.iter().flat_map(|(_, src)| &src.items)),
1097-
fp_kotlin::collect_string_field_names(prepared.iter().flat_map(|(_, src)| &src.items)),
1098-
fp_kotlin::collect_enum_field_names(prepared.iter().flat_map(|(_, src)| &src.items)),
1099-
fp_kotlin::collect_enum_variant_names(prepared.iter().flat_map(|(_, src)| &src.items)),
1100-
)
1101-
} else {
1102-
(Default::default(), Default::default(), Default::default(), Default::default(), Default::default())
1103-
};
1097+
}
1098+
} else {
1099+
untyped_prepared(&packages)?
1100+
};
11041101

1105-
// Every item's own qualified path -> qualified paths it references,
1106-
// merged across every package in the workspace (see `PackageSource::
1107-
// referenced_paths`) — lets the Kotlin serializer compute imports for
1108-
// spliced-in content from actual usage rather than only echoing the
1109-
// source file's pre-existing `use` items.
1110-
let workspace_referenced_paths: std::collections::HashMap<Vec<String>, Vec<Vec<String>>> = prepared
1111-
.iter()
1112-
.flat_map(|(_, src)| src.referenced_paths.iter())
1113-
.map(|(path, refs)| (path.clone(), refs.clone()))
1114-
.collect();
1102+
// Cross-package facts (field mutability, List-vs-String disambiguation,
1103+
// referenced-path imports) the Kotlin backend needs before serializing
1104+
// any single package — see `KotlinWorkspaceContext`'s doc comment.
1105+
let kotlin_ctx = if matches!(target, crate::languages::backend::BuiltinLanguageTarget::Kotlin) {
1106+
fp_kotlin::KotlinWorkspaceContext::collect(prepared.iter().map(|(_, src)| src))
1107+
} else {
1108+
fp_kotlin::KotlinWorkspaceContext::default()
1109+
};
11151110

11161111
// Phase 2: serialize + write every package now that the workspace-wide
11171112
// mutability set (and any other cross-package info) is complete.
@@ -1146,7 +1141,7 @@ async fn compile_project(
11461141
let files = if let crate::languages::backend::BuiltinLanguageTarget::Kotlin = target {
11471142
let serializer = fp_kotlin::KotlinSerializer;
11481143
serializer
1149-
.serialize_package(source, &workspace_packages, &workspace_mutated_fields, &workspace_list_fields, &workspace_string_fields, &workspace_enum_fields, &workspace_referenced_paths, &workspace_enum_variant_names)
1144+
.serialize_package(source, &workspace_packages, &kotlin_ctx)
11501145
.map_err(|e| CliError::Compilation(e.to_string()))?
11511146
} else {
11521147
serialize_package_for_target(source, target, &args, &output.join(name))?

crates/fp-cli/src/compiler.rs

Lines changed: 41 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1090,7 +1090,7 @@ fn compile_source_file(
10901090
Ok(session.into_driver())
10911091
}
10921092

1093-
fn drain_driver(driver: &mut CompilerDriver, lossy: LossyCompileOptions) -> Result<()> {
1093+
pub fn drain_driver(driver: &mut CompilerDriver, lossy: LossyCompileOptions) -> Result<()> {
10941094
emit_typing_diagnostics(&driver.state.borrow().typing_ctx.diagnostics.borrow(), lossy)
10951095
}
10961096

@@ -1162,6 +1162,31 @@ pub fn parse_file_with_mode(
11621162
parse_file_with_context(path, source_language, parse_mode, lossy)
11631163
}
11641164

1165+
/// Builds the executor/provider/workspace/session a typechecking compile
1166+
/// needs — shared by `typecheck_package`'s single-package path and
1167+
/// `compile_project`'s (`fp-cli/src/commands/compile.rs`) whole-workspace
1168+
/// path, so a workspace compile builds this once for every member instead
1169+
/// of once per member.
1170+
pub fn build_workspace_session(
1171+
provider: Arc<dyn PackageProvider>,
1172+
language: &str,
1173+
lossy: LossyCompileOptions,
1174+
capabilities: fp_core::capabilities::LanguageCapabilities,
1175+
) -> (CompilerExecutor, CompilerSession) {
1176+
let executor = CompilerExecutor::new();
1177+
let std_provider = std_provider_for(language);
1178+
let combined = Arc::new(fp_core::package::provider::CompositeProvider::new(vec![
1179+
std_provider,
1180+
provider,
1181+
]));
1182+
let workspace = std::rc::Rc::new(fp_core::workspace::WorkspaceContext::new(combined));
1183+
let mut session = CompilerSession::new(data_layout(), &executor, workspace);
1184+
session.driver().pipeline = PipelineMode::TypecheckedTranspile;
1185+
session.driver().state.borrow_mut().set_lossy(lossy.enabled);
1186+
session.driver().state.borrow_mut().set_capabilities(capabilities);
1187+
(executor, session)
1188+
}
1189+
11651190
/// Typecheck a whole package by registering its real `PackageProvider` with
11661191
/// a fresh `CompilerDriver` under `PipelineMode::TypecheckedTranspile`,
11671192
/// instead of flattening the package's items into a single tag-less `File`
@@ -1184,17 +1209,7 @@ pub fn typecheck_package(
11841209
language: &str,
11851210
capabilities: fp_core::capabilities::LanguageCapabilities,
11861211
) -> Result<PackageSource> {
1187-
let executor = CompilerExecutor::new();
1188-
let std_provider = std_provider_for(language);
1189-
let combined = Arc::new(fp_core::package::provider::CompositeProvider::new(vec![
1190-
std_provider,
1191-
provider,
1192-
]));
1193-
let workspace = std::rc::Rc::new(fp_core::workspace::WorkspaceContext::new(combined));
1194-
let mut session = CompilerSession::new(data_layout(), &executor, workspace);
1195-
session.driver().pipeline = PipelineMode::TypecheckedTranspile;
1196-
session.driver().state.borrow_mut().set_lossy(lossy.enabled);
1197-
session.driver().state.borrow_mut().set_capabilities(capabilities);
1212+
let (executor, mut session) = build_workspace_session(provider, language, lossy, capabilities);
11981213
let package = executor
11991214
.run(session.driver().compile_package(package_id))
12001215
.map_err(|err| CliError::Compilation(err.to_string()))?;
@@ -1209,7 +1224,18 @@ pub fn typecheck_package(
12091224
// placeholders through as if nothing were wrong.
12101225
drain_driver(session.driver(), lossy)?;
12111226

1212-
let package = package.borrow();
1227+
Ok(package_source_from_compiled(package_id, &package))
1228+
}
1229+
1230+
/// Builds a `PackageSource` from a compiled package — shared by
1231+
/// `typecheck_package`'s single-package path and `compile_project`'s
1232+
/// (`fp-cli/src/commands/compile.rs`) whole-workspace path, so both read
1233+
/// back the same typed/normalized content the same way.
1234+
pub fn package_source_from_compiled(
1235+
package_id: &PackageId,
1236+
compiled: &std::rc::Rc<std::cell::RefCell<fp_core::package::CompiledPackage>>,
1237+
) -> PackageSource {
1238+
let package = compiled.borrow();
12131239
// Typed/normalized content is already spliced onto `package.items` by
12141240
// `CompilerDriver::compile_package` (qualified-path-keyed, including
12151241
// impl methods) — nothing left to reconcile here.
@@ -1229,15 +1255,14 @@ pub fn typecheck_package(
12291255
.collect()
12301256
})
12311257
.unwrap_or_default();
1232-
let source = PackageSource {
1258+
PackageSource {
12331259
package_id: package_id.clone(),
12341260
name: package.name.clone(),
12351261
graph: package.graph.clone(),
12361262
module_paths: package.module_paths.clone(),
12371263
items,
12381264
referenced_paths,
1239-
};
1240-
Ok(source)
1265+
}
12411266
}
12421267

12431268
fn parse_file_with_context(

0 commit comments

Comments
 (0)