From c61064ccfcc0aae64fd0c6e6c80706c6630dc5ca Mon Sep 17 00:00:00 2001 From: Rory Campbell Date: Wed, 11 Mar 2026 12:09:01 +0100 Subject: [PATCH 1/7] better error display --- src/build/mod.rs | 3 + src/main.rs | 16 +++- src/typechecker/error.rs | 166 +++++++++++++++++++-------------------- tests/snapshots.rs | 14 ++-- 4 files changed, 105 insertions(+), 94 deletions(-) diff --git a/src/build/mod.rs b/src/build/mod.rs index 56a49d7e..60358303 100644 --- a/src/build/mod.rs +++ b/src/build/mod.rs @@ -647,6 +647,7 @@ fn build_from_sources_impl( module_results.push(ModuleResult { path: pm.path.clone(), module_name: pm.module_name.clone(), + type_errors: vec![], cached: true, }); @@ -723,6 +724,7 @@ fn build_from_sources_impl( module_results.push(ModuleResult { path: pm.path.clone(), module_name: pm.module_name.clone(), + type_errors: result.errors, cached: false, }); @@ -847,6 +849,7 @@ fn build_from_sources_impl( module_results.push(ModuleResult { path: pm.path.clone(), module_name: pm.module_name.clone(), + type_errors: result.errors, cached: false, }); diff --git a/src/main.rs b/src/main.rs index 88dac18d..fc952ea3 100644 --- a/src/main.rs +++ b/src/main.rs @@ -88,8 +88,22 @@ fn main() { } for module in &result.modules { + if module.type_errors.is_empty() { + continue; + } + let source = std::fs::read_to_string(&module.path).unwrap_or_default(); for err in &module.type_errors { - error_messages.push(format!("{}: {err}", module.module_name)); + let span = err.span(); + let location = match span.to_pos(&source) { + Some((start, end)) => format!( + "{}:{}:{} - {}:{}", + module.path.display(), + start.line, start.column, + end.line, end.column + ), + None => format!("{}", module.path.display()), + }; + error_messages.push(format!("{location}: {err}")); } } diff --git a/src/typechecker/error.rs b/src/typechecker/error.rs index 0675689d..7afdf152 100644 --- a/src/typechecker/error.rs +++ b/src/typechecker/error.rs @@ -10,7 +10,7 @@ use crate::typechecker::types::{TyVarId, Type}; #[derive(Debug, Clone, thiserror::Error)] pub enum TypeError { /// Two types could not be unified - #[error("Could not match type {expected} with type {found} at {span}")] + #[error("Could not match type {expected} with type {found}")] UnificationError { span: Span, expected: Type, @@ -18,34 +18,34 @@ pub enum TypeError { }, /// Occurs check failure (infinite type) - #[error("An infinite type was inferred for type variable t{}: {ty} at {span} ", var.0)] + #[error("An infinite type was inferred for type variable t{}: {ty}", var.0)] InfiniteType { span: Span, var: TyVarId, ty: Type }, - #[error("An infinite kind was inferred for type t{}: {ty} at {span} ", var.0)] + #[error("An infinite kind was inferred for type t{}: {ty}", var.0)] InfiniteKind { span: Span, var: TyVarId, ty: Type }, /// Variable not found in scope - #[error("Unknown value {} at {span}", interner::resolve(*name).unwrap_or_default())] + #[error("Unknown value {}", interner::resolve(*name).unwrap_or_default())] UndefinedVariable { span: Span, name: Symbol }, /// Name not found in scope (used during AST name resolution, corresponds to PureScript's UnknownName) - #[error("Unknown name {} at {span}", interner::resolve(*name).unwrap_or_default())] + #[error("Unknown name {}", interner::resolve(*name).unwrap_or_default())] UnknownName { span: Span, name: Symbol }, /// Type signature without a corresponding value declaration - #[error("The type declaration for {} has no corresponding value declaration at {span}", interner::resolve(*name).unwrap_or_default())] + #[error("The type declaration for {} has no corresponding value declaration", interner::resolve(*name).unwrap_or_default())] OrphanTypeSignature { span: Span, name: Symbol }, /// Duplicate type signature for the same name - #[error("Duplicate type declaration for {} at {span}", interner::resolve(*name).unwrap_or_default())] + #[error("Duplicate type declaration for {}", interner::resolve(*name).unwrap_or_default())] DuplicateTypeSignature { span: Span, name: Symbol }, /// Typed hole: ?name reports the inferred type at that point - #[error("Hole ?{} has the inferred type {ty} at {span}", interner::resolve(*name).unwrap_or_default())] + #[error("Hole ?{} has the inferred type {ty}", interner::resolve(*name).unwrap_or_default())] HoleInferredType { span: Span, name: Symbol, ty: Type }, /// Arity mismatch between equations of the same function - #[error("The function {} was defined with {expected} arguments in one equation but {found} in another at {span}", interner::resolve(*name).unwrap_or_default())] + #[error("The function {} was defined with {expected} arguments in one equation but {found} in another", interner::resolve(*name).unwrap_or_default())] ArityMismatch { span: Span, name: Symbol, @@ -54,7 +54,7 @@ pub enum TypeError { }, /// No instance found for a type class constraint - #[error("No type class instance was found for {class_name} {args} at {span}", + #[error("No type class instance was found for {class_name} {args}", args = type_args.iter().map(|ty| format!("{}", ty)).collect::>().join(" "), )] NoInstanceFound { @@ -64,7 +64,7 @@ pub enum TypeError { }, /// Non-exhaustive pattern match - #[error("A case expression could not be determined to cover all inputs. The following patterns are missing: {} at {span}", missing.join(", "))] + #[error("A case expression could not be determined to cover all inputs. The following patterns are missing: {}", missing.join(", "))] NonExhaustivePattern { span: Span, type_name: QualifiedIdent, @@ -72,51 +72,51 @@ pub enum TypeError { }, /// Export of undeclared name - #[error("Cannot export undeclared value {} at {span}", interner::resolve(*name).unwrap_or_default())] + #[error("Cannot export undeclared value {}", interner::resolve(*name).unwrap_or_default())] UnkownExport { span: Span, name: Symbol }, /// Unknown type - #[error("Unknown type {} at {span}", interner::resolve(*name).unwrap_or_default())] + #[error("Unknown type {}", interner::resolve(*name).unwrap_or_default())] UnknownType { span: Span, name: Symbol }, /// Duplicate value declaration for the same name - #[error("The value {} has been defined multiple times at {spans:?}", interner::resolve(*name).unwrap_or_default())] + #[error("The value {} has been defined multiple times", interner::resolve(*name).unwrap_or_default())] DuplicateValueDeclaration { spans: Vec, name: Symbol }, /// Kind declaration without a corresponding type declaration - #[error("The kind declaration for {} has no corresponding type declaration at {span}", interner::resolve(*name).unwrap_or_default())] + #[error("The kind declaration for {} has no corresponding type declaration", interner::resolve(*name).unwrap_or_default())] OrphanKindDeclaration { span: Span, name: Symbol }, /// Imported module not found - #[error("Module {} was not found at {span}", interner::resolve(*name).unwrap_or_default())] + #[error("Module {} was not found", interner::resolve(*name).unwrap_or_default())] ModuleNotFound { span: Span, name: Symbol }, /// Multiple fixity declarations for the same operator - #[error("Multiple fixity declarations for operator {} at {spans:?}", interner::resolve(*name).unwrap_or_default())] + #[error("Multiple fixity declarations for operator {}", interner::resolve(*name).unwrap_or_default())] MultipleValueOpFixities { spans: Vec, name: Symbol }, /// Multiple fixity declarations for the same type operator - #[error("Multiple fixity declarations for type operator {} at {spans:?}", interner::resolve(*name).unwrap_or_default())] + #[error("Multiple fixity declarations for type operator {}", interner::resolve(*name).unwrap_or_default())] MultipleTypeOpFixities { spans: Vec, name: Symbol }, /// Overlapping names in a let binding - #[error("The value {} has been defined multiple times in a let binding at {spans:?}", interner::resolve(*name).unwrap_or_default())] + #[error("The value {} has been defined multiple times in a let binding", interner::resolve(*name).unwrap_or_default())] OverlappingNamesInLet { spans: Vec, name: Symbol }, /// Overlapping pattern variable names in a pattern match - #[error("The variable {} appears more than once in a pattern at {spans:?}", interner::resolve(*name).unwrap_or_default())] + #[error("The variable {} appears more than once in a pattern", interner::resolve(*name).unwrap_or_default())] OverlappingPattern { spans: Vec, name: Symbol }, /// Unknown import (name not found in imported module) - #[error("Cannot import {}, as it is not exported by the module at {span}", interner::resolve(*name).unwrap_or_default())] + #[error("Cannot import {}, as it is not exported by the module", interner::resolve(*name).unwrap_or_default())] UnknownImport { span: Span, name: Symbol }, /// Unknown data constructor import: import A (MyType(Exists, DoesNotExist)) - #[error("Cannot import unknown data constructor {} at {span}", interner::resolve(*name).unwrap_or_default())] + #[error("Cannot import unknown data constructor {}", interner::resolve(*name).unwrap_or_default())] UnknownImportDataConstructor { span: Span, name: Symbol }, /// Incorrect number of arguments to a data constructor in a binder - #[error("Constructor {} expects {expected} arguments but was given {found} at {span}", interner::resolve(*name).unwrap_or_default())] + #[error("Constructor {} expects {expected} arguments but was given {found}", interner::resolve(*name).unwrap_or_default())] IncorrectConstructorArity { span: Span, name: Symbol, @@ -125,7 +125,7 @@ pub enum TypeError { }, /// Duplicate field labels in a record type or pattern - #[error("The label {} appears more than once in a record at {record_span}", interner::resolve(*name).unwrap_or_default())] + #[error("The label {} appears more than once in a record", interner::resolve(*name).unwrap_or_default())] DuplicateLabel { record_span: Span, field_spans: Vec, @@ -133,41 +133,35 @@ pub enum TypeError { }, /// Invalid newtype derived instance. E.g. derive instance Newtype NotANewtype - #[error( - "Cannot derive a Newtype instance for {}: it is not a newtype at {span}", - name - )] + #[error("Cannot derive a Newtype instance for {}: it is not a newtype", name)] InvalidNewtypeInstance { span: Span, name: QualifiedIdent }, /// derive newtype instance on a type that isn't an instance of Newtype. E.g. derive newtype instance MyClass NotANewtype - #[error( - "Cannot use newtype deriving for {} because it does not have a Newtype instance at {span}", - name - )] + #[error("Cannot use newtype deriving for {} because it does not have a Newtype instance", name)] InvalidNewtypeDerivation { span: Span, name: QualifiedIdent }, /// A constructor argument is not valid for the derived class (e.g. contravariant position for Functor) - #[error("Cannot derive instance: invalid constructor argument at {span}")] + #[error("Cannot derive instance: invalid constructor argument")] CannotDeriveInvalidConstructorArg { span: Span }, /// Multiple type classes with the same name - #[error("The type class {} has been defined multiple times at {spans:?}", interner::resolve(*name).unwrap_or_default())] + #[error("The type class {} has been defined multiple times", interner::resolve(*name).unwrap_or_default())] DuplicateTypeClass { spans: Vec, name: Symbol }, /// Multiple class instances with the same name - #[error("The instance {} has been defined multiple times at {spans:?}", interner::resolve(*name).unwrap_or_default())] + #[error("The instance {} has been defined multiple times", interner::resolve(*name).unwrap_or_default())] DuplicateInstance { spans: Vec, name: Symbol }, /// Multiple args to a type with the same name - #[error("The type variable {} appears more than once in a type declaration at {spans:?}", interner::resolve(*name).unwrap_or_default())] + #[error("The type variable {} appears more than once in a type declaration", interner::resolve(*name).unwrap_or_default())] DuplicateTypeArgument { spans: Vec, name: Symbol }, /// Invalid do bind. Eg on the last line of a do block - #[error("A bind statement cannot be the last statement in a do block at {span}")] + #[error("A bind statement cannot be the last statement in a do block")] InvalidDoBind { span: Span }, /// Invalid do let. Eg on the last line of a do block - #[error("A let statement cannot be the last statement in a do block at {span}")] + #[error("A let statement cannot be the last statement in a do block")] InvalidDoLet { span: Span }, /// Multiple type synonyms that reference each other in a cycle @@ -207,7 +201,7 @@ pub enum TypeError { }, /// Overlapping argument names in a function - #[error("The argument {} appears more than once in a function definition at {span}", interner::resolve(*name).unwrap_or_default())] + #[error("The argument {} appears more than once in a function definition", interner::resolve(*name).unwrap_or_default())] OverlappingArgNames { span: Span, name: Symbol, @@ -215,12 +209,12 @@ pub enum TypeError { }, /// Feature not yet implemented in the typechecker - #[error("Not yet implemented: {feature} at {span}")] + #[error("Not yet implemented: {feature}")] NotImplemented { span: Span, feature: String }, - #[error("The name {} cannot be brought into scope in a do notation block, since do notation uses the same name at {span}", interner::resolve(*name).unwrap_or_default())] + #[error("The name {} cannot be brought into scope in a do notation block, since do notation uses the same name", interner::resolve(*name).unwrap_or_default())] CannotUseBindWithDo { span: Span, name: Symbol }, - #[error("A cycle was found in declarations involving {} at {span}. Others: {}", + #[error("A cycle was found in declarations involving {}. Others: {}", interner::resolve(*name).unwrap_or_default(), others_in_cycle.iter().map(|(n, _)| interner::resolve(*n).unwrap_or_default()).collect::>().join(" -> ") )] @@ -230,10 +224,10 @@ pub enum TypeError { others_in_cycle: Vec<(Symbol, Span)>, }, - #[error("The class {name} is not defined at {span}")] + #[error("The class {name} is not defined")] UnknownClass { span: Span, name: QualifiedIdent }, - #[error("The class {class_name} is missing the following members at {span}: {}", + #[error("The class {class_name} is missing the following members: {}", members.iter().map(|(n, ty)| format!("{} :: {}", interner::resolve(*n).unwrap_or_default(), ty)).collect::>().join(", ") )] MissingClassMember { @@ -242,7 +236,7 @@ pub enum TypeError { members: Vec<(Symbol, Type)>, }, - #[error("The class {class_name} has the following extraneous member at {span}: {}", + #[error("The class {class_name} has the following extraneous member: {}", interner::resolve(*member_name).unwrap_or_default() )] ExtraneousClassMember { @@ -251,7 +245,7 @@ pub enum TypeError { member_name: Symbol, }, /// Declaration conflict: a name is used for two different kinds of declarations - #[error("Declaration for {new_kind} {} conflicts with an existing {existing_kind} of the same name at {span}", + #[error("Declaration for {new_kind} {} conflicts with an existing {existing_kind} of the same name", interner::resolve(*name).unwrap_or_default() )] DeclConflict { @@ -266,37 +260,37 @@ pub enum TypeError { // , markCodeBox $ indent $ prettyType ty // , line "Try adding a type signature." // ] - #[error("Unable to generalize the type of the recursive function {}. The inferred type was {} at {span}. Try adding a type signature.", interner::resolve(*name).unwrap_or_default(), type_)] + #[error("Unable to generalize the type of the recursive function {}. The inferred type was {}. Try adding a type signature.", interner::resolve(*name).unwrap_or_default(), type_)] CannotGeneralizeRecursiveFunction { span: Span, name: Symbol, type_: Type, }, - #[error("Cannot apply expression of type {type_} to a type argument at {span}")] + #[error("Cannot apply expression of type {type_} to a type argument")] CannotApplyExpressionOfTypeOnType { span: Span, type_: Type }, - #[error("An anonymous function argument _ appears in an invalid context at {span}")] + #[error("An anonymous function argument _ appears in an invalid context")] IncorrectAnonymousArgument { span: Span }, - #[error("Operator {} cannot be used in a pattern as it is an alias for a function, not a data constructor, at {span}", + #[error("Operator {} cannot be used in a pattern as it is an alias for a function, not a data constructor", interner::resolve(*op).unwrap_or_default() )] InvalidOperatorInBinder { span: Span, op: Symbol }, - #[error("Integer value {value} is out of range at {span}. Acceptable values fall within the range -2147483648 to 2147483647 (inclusive).")] + #[error("Integer value {value} is out of range. Acceptable values fall within the range -2147483648 to 2147483647 (inclusive).")] IntOutOfRange { span: Span, value: i64 }, - #[error("The role declaration for {} should follow its definition at {span}", interner::resolve(*name).unwrap_or_default())] + #[error("The role declaration for {} should follow its definition", interner::resolve(*name).unwrap_or_default())] OrphanRoleDeclaration { span: Span, name: Symbol }, - #[error("Duplicate role declaration for {} at {span}", interner::resolve(*name).unwrap_or_default())] + #[error("Duplicate role declaration for {}", interner::resolve(*name).unwrap_or_default())] DuplicateRoleDeclaration { span: Span, name: Symbol }, - #[error("Role declarations are only supported for data types, not for type synonyms nor type classes at {span}")] + #[error("Role declarations are only supported for data types, not for type synonyms nor type classes")] UnsupportedRoleDeclaration { span: Span, name: Symbol }, - #[error("Role declaration for {} declares {found} roles, but the type has {expected} parameters at {span}", interner::resolve(*name).unwrap_or_default())] + #[error("Role declaration for {} declares {found} roles, but the type has {expected} parameters", interner::resolve(*name).unwrap_or_default())] RoleDeclarationArityMismatch { span: Span, name: Symbol, @@ -304,41 +298,41 @@ pub enum TypeError { found: usize, }, - #[error("A case expression has {found} binders but {expected} scrutinee(s) at {span}")] + #[error("A case expression has {found} binders but {expected} scrutinee(s)")] CaseBinderLengthDiffers { span: Span, expected: usize, found: usize, }, - #[error("Non-associative operator {} used with another operator of the same precedence at {span}", interner::resolve(*op).unwrap_or_default())] + #[error("Non-associative operator {} used with another operator of the same precedence", interner::resolve(*op).unwrap_or_default())] NonAssociativeError { span: Span, op: Symbol }, - #[error("Operators with mixed associativity at the same precedence at {span}")] + #[error("Operators with mixed associativity at the same precedence")] MixedAssociativityError { span: Span }, - #[error("The name {} in a foreign import contains a prime character which is not allowed at {span}", interner::resolve(*name).unwrap_or_default())] + #[error("The name {} in a foreign import contains a prime character which is not allowed", interner::resolve(*name).unwrap_or_default())] DeprecatedFFIPrime { span: Span, name: Symbol }, - #[error("Type wildcards are not allowed in type definitions at {span}")] + #[error("Type wildcards are not allowed in type definitions")] WildcardInTypeDefinition { span: Span }, - #[error("Constraints are not allowed in foreign import types at {span}")] + #[error("Constraints are not allowed in foreign import types")] ConstraintInForeignImport { span: Span }, - #[error("A forall or wildcard is not allowed in a constraint argument at {span}")] + #[error("A forall or wildcard is not allowed in a constraint argument")] InvalidConstraintArgument { span: Span }, /// Syntax error in type expression (corresponds to PureScript's ErrorParsingModule) - #[error("Syntax error at {span}")] + #[error("Syntax error")] SyntaxError { span: Span }, /// Expected a wildcard type argument in a Newtype derive instance - #[error("Expected a wildcard (_) in the Newtype instance at {span}")] + #[error("Expected a wildcard (_) in the Newtype instance")] ExpectedWildcard { span: Span, name: QualifiedIdent }, #[error( - "Kind mismatch: type synonym {} expects {} argument(s) but was given {} at {span}", + "Kind mismatch: type synonym {} expects {} argument(s) but was given {}", name, expected, found @@ -350,7 +344,7 @@ pub enum TypeError { found: usize, }, - #[error("The type class {class_name} expects {expected} type argument(s), but the instance provided {found} at {span}")] + #[error("The type class {class_name} expects {expected} type argument(s), but the instance provided {found}")] ClassInstanceArityMismatch { span: Span, class_name: QualifiedIdent, @@ -358,17 +352,17 @@ pub enum TypeError { found: usize, }, - #[error("Type variable {} is undefined at {span}", interner::resolve(*name).unwrap_or_default())] + #[error("Type variable {} is undefined", interner::resolve(*name).unwrap_or_default())] UndefinedTypeVariable { span: Span, name: Symbol }, - #[error("Invalid type in instance head at {span}")] + #[error("Invalid type in instance head")] InvalidInstanceHead { span: Span }, - #[error("Type synonym {} is partially applied at {span}", name)] + #[error("Type synonym {} is partially applied", name)] PartiallyAppliedSynonym { span: Span, name: QualifiedIdent }, #[error( - "A transitive export error occurred: {} depends on {} which is not exported at {span}", + "A transitive export error occurred: {} depends on {} which is not exported", exported, dependency )] @@ -378,17 +372,17 @@ pub enum TypeError { dependency: QualifiedIdent, }, - #[error("Scope conflict: the name {} is ambiguous, imported from multiple modules at {span}", + #[error("Scope conflict: the name {} is ambiguous, imported from multiple modules", interner::resolve(*name).unwrap_or_default() )] ScopeConflict { span: Span, name: Symbol }, - #[error("Export conflict: the name {} is exported by multiple re-exported modules at {span}", + #[error("Export conflict: the name {} is exported by multiple re-exported modules", interner::resolve(*name).unwrap_or_default() )] ExportConflict { span: Span, name: Symbol }, - #[error("Overlapping instances found for {class_name} {args} at {span}", + #[error("Overlapping instances found for {class_name} {args}", args = type_args.iter().map(|ty| format!("{}", ty)).collect::>().join(" ") )] OverlappingInstances { @@ -397,13 +391,13 @@ pub enum TypeError { type_args: Vec, }, - #[error("An orphan instance was found for class {class_name} at {span}. Instances must be defined in the same module as the class or one of the types in the instance head." )] + #[error("An orphan instance was found for class {class_name}. Instances must be defined in the same module as the class or one of the types in the instance head.")] OrphanInstance { span: Span, class_name: QualifiedIdent, }, - #[error("Type class instance for {class_name} {args} is possibly infinite at {span}", + #[error("Type class instance for {class_name} {args} is possibly infinite", args = type_args.iter().map(|ty| format!("{}", ty)).collect::>().join(" ") )] PossiblyInfiniteInstance { @@ -412,18 +406,18 @@ pub enum TypeError { type_args: Vec, }, - #[error("The type variable {name_str} is ambiguous at {span}", + #[error("The type variable {name_str} is ambiguous", name_str = names.iter().map(|n| interner::resolve(*n).unwrap_or_default()).collect::>().join(", ") )] AmbiguousTypeVariables { span: Span, names: Vec }, - #[error("Invalid Coercible instance declaration at {span}")] + #[error("Invalid Coercible instance declaration")] InvalidCoercibleInstanceDeclaration { span: Span }, - #[error("Role mismatch for type {} at {span}", interner::resolve(*name).unwrap_or_default())] + #[error("Role mismatch for type {}", interner::resolve(*name).unwrap_or_default())] RoleMismatch { span: Span, name: Symbol }, - #[error("Possibly infinite Coercible instance at {span}")] + #[error("Possibly infinite Coercible instance")] PossiblyInfiniteCoercibleInstance { span: Span, class_name: QualifiedIdent, @@ -431,7 +425,7 @@ pub enum TypeError { }, /// Kind unification failure: two kinds could not be unified - #[error("Could not match kind {expected} with kind {found} at {span}")] + #[error("Could not match kind {expected} with kind {found}")] KindsDoNotUnify { span: Span, expected: Type, @@ -439,27 +433,27 @@ pub enum TypeError { }, /// Expected a type of kind Type, but found a higher-kinded type - #[error("Expected type of kind Type, but found kind {found} at {span}")] + #[error("Expected type of kind Type, but found kind {found}")] ExpectedType { span: Span, found: Type }, /// Constraint used in kind position (e.g., `foreign data Bad :: Ok => Type`) - #[error("Unsupported type in kind at {span}")] + #[error("Unsupported type in kind")] UnsupportedTypeInKind { span: Span }, /// A rigid type variable (skolem) has escaped its scope - #[error("A type variable has escaped its scope at {span}")] + #[error("A type variable has escaped its scope")] EscapedSkolem { span: Span, name: Symbol, ty: Type }, /// Implicit kind quantification would be needed inside a user-written forall (type-level) - #[error("Cannot unambiguously generalize type kinds for {ty} at {span}")] + #[error("Cannot unambiguously generalize type kinds for {ty}")] QuantificationCheckFailureInType { ty: Type, span: Span }, /// Implicit kind quantification would be needed inside a kind annotation - #[error("Cannot unambiguously generalize kinds for {ty} at {span}")] + #[error("Cannot unambiguously generalize kinds for {ty}")] QuantificationCheckFailureInKind { ty: Type, span: Span }, /// Visible dependent quantification is not supported - #[error("Visible dependent quantification is not supported at {span}")] + #[error("Visible dependent quantification is not supported")] VisibleQuantificationCheckFailureInType { span: Span }, } diff --git a/tests/snapshots.rs b/tests/snapshots.rs index 9cfc8d73..d8b0eead 100644 --- a/tests/snapshots.rs +++ b/tests/snapshots.rs @@ -100,7 +100,7 @@ fn snap_expr_negate() { fn snap_expr_error_branch_mismatch() { insta::assert_snapshot!( format_expr_type(r#"if true then 1 else "x""#), - @"ERROR: Could not match type Int with type String at 20:23" + @"ERROR: Could not match type Int with type String" ); } @@ -108,7 +108,7 @@ fn snap_expr_error_branch_mismatch() { fn snap_expr_error_not_boolean_cond() { insta::assert_snapshot!( format_expr_type("if 42 then 1 else 2"), - @"ERROR: Could not match type Int with type Boolean at 3:5" + @"ERROR: Could not match type Int with type Boolean" ); } @@ -116,7 +116,7 @@ fn snap_expr_error_not_boolean_cond() { fn snap_expr_error_undefined() { insta::assert_snapshot!( format_expr_type("undefinedVar"), - @"ERROR: Unknown value undefinedVar at 0:12" + @"ERROR: Unknown value undefinedVar" ); } @@ -124,7 +124,7 @@ fn snap_expr_error_undefined() { fn snap_expr_error_negate_string() { insta::assert_snapshot!( format_expr_type(r#"-"hello""#), - @"ERROR: No type class instance was found for Ring String at 0:8" + @"ERROR: No type class instance was found for Ring String" ); } @@ -167,7 +167,7 @@ y = Nothing")); fn snap_module_error_sig_mismatch() { insta::assert_snapshot!( format_module_error("module T where\nx :: Int\nx = true"), - @"Could not match type Boolean with type Int at 24:32" + @"Could not match type Boolean with type Int" ); } @@ -175,7 +175,7 @@ fn snap_module_error_sig_mismatch() { fn snap_module_error_duplicate_sig() { insta::assert_snapshot!( format_module_error("module T where\nx :: Int\nx :: String\nx = 42"), - @"Duplicate type declaration for x at 24:35" + @"Duplicate type declaration for x" ); } @@ -183,7 +183,7 @@ fn snap_module_error_duplicate_sig() { fn snap_module_error_orphan_sig() { insta::assert_snapshot!( format_module_error("module T where\nx :: Int"), - @"The type declaration for x has no corresponding value declaration at 15:23" + @"The type declaration for x has no corresponding value declaration" ); } From 37f6eb7f1f397c4eb3149d35d5ef652e0d6842d2 Mon Sep 17 00:00:00 2001 From: Rory Campbell Date: Wed, 11 Mar 2026 12:11:25 +0100 Subject: [PATCH 2/7] location on other line --- src/main.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main.rs b/src/main.rs index fc952ea3..68f7a1bd 100644 --- a/src/main.rs +++ b/src/main.rs @@ -103,7 +103,7 @@ fn main() { ), None => format!("{}", module.path.display()), }; - error_messages.push(format!("{location}: {err}")); + error_messages.push(format!("{location}:\n\n{err}")); } } From 29c5ba88c5660512ac76c60d7060083f4c4f195a Mon Sep 17 00:00:00 2001 From: Rory Campbell Date: Wed, 11 Mar 2026 12:14:28 +0100 Subject: [PATCH 3/7] dont cache modules with errors --- src/build/cache.rs | 7 ++++++ src/build/mod.rs | 56 ++++++++++++++++++++++++++++++---------------- tests/build.rs | 20 +++++++++++++++++ 3 files changed, 64 insertions(+), 19 deletions(-) diff --git a/src/build/cache.rs b/src/build/cache.rs index b3136ed9..27e8f155 100644 --- a/src/build/cache.rs +++ b/src/build/cache.rs @@ -267,6 +267,13 @@ impl ModuleCache { exports_changed } + /// Remove a module from the cache (e.g. when it fails typechecking). + pub fn remove(&mut self, module_name: &str) { + if self.entries.remove(module_name).is_some() { + self.index_dirty = true; + } + } + /// Build the reverse dependency graph from cached import data. pub fn build_reverse_deps(&mut self) { self.dependents.clear(); diff --git a/src/build/mod.rs b/src/build/mod.rs index 60358303..aa43dfcc 100644 --- a/src/build/mod.rs +++ b/src/build/mod.rs @@ -706,17 +706,26 @@ fn build_from_sources_impl( " [{}/{}] ok: {} ({:.2?})", done, total_modules, pm.module_name, elapsed ); - let import_names: Vec = pm.import_parts.iter() - .map(|parts| interner::resolve_module_name(parts)) - .collect(); - let exports_changed = if let Some(ref mut c) = cache { - c.update(pm.module_name.clone(), pm.source_hash, result.exports.clone(), import_names) - } else { - true - }; - // Only add to rebuilt_set if exports actually changed - if exports_changed { + let has_errors = !result.errors.is_empty(); + if has_errors { + // Don't cache modules with type errors + if let Some(ref mut c) = cache { + c.remove(&pm.module_name); + } rebuilt_set.insert(pm.module_name.clone()); + } else { + let import_names: Vec = pm.import_parts.iter() + .map(|parts| interner::resolve_module_name(parts)) + .collect(); + let exports_changed = if let Some(ref mut c) = cache { + c.update(pm.module_name.clone(), pm.source_hash, result.exports.clone(), import_names) + } else { + true + }; + // Only add to rebuilt_set if exports actually changed + if exports_changed { + rebuilt_set.insert(pm.module_name.clone()); + } } // Register exports immediately — result.exports is moved, // then result (with its types HashMap) is dropped. @@ -834,16 +843,25 @@ fn build_from_sources_impl( " [{}/{}] ok: {} ({:.2?})", done, total_modules, pm.module_name, elapsed ); - let import_names: Vec = pm.import_parts.iter() - .map(|parts| interner::resolve_module_name(parts)) - .collect(); - let exports_changed = if let Some(ref mut c) = cache { - c.update(pm.module_name.clone(), pm.source_hash, result.exports.clone(), import_names) - } else { - true - }; - if exports_changed { + let has_errors = !result.errors.is_empty(); + if has_errors { + // Don't cache modules with type errors + if let Some(ref mut c) = cache { + c.remove(&pm.module_name); + } rebuilt_set.insert(pm.module_name.clone()); + } else { + let import_names: Vec = pm.import_parts.iter() + .map(|parts| interner::resolve_module_name(parts)) + .collect(); + let exports_changed = if let Some(ref mut c) = cache { + c.update(pm.module_name.clone(), pm.source_hash, result.exports.clone(), import_names) + } else { + true + }; + if exports_changed { + rebuilt_set.insert(pm.module_name.clone()); + } } registry.register(&pm.module_parts, result.exports); module_results.push(ModuleResult { diff --git a/tests/build.rs b/tests/build.rs index 9c8f8565..fcc48233 100644 --- a/tests/build.rs +++ b/tests/build.rs @@ -1320,3 +1320,23 @@ fn incremental_build_disk_roundtrip() { // Cleanup let _ = std::fs::remove_dir_all(&tmp_dir); } + +#[test] +fn incremental_build_does_not_cache_errors() { + let sources: Vec<(&str, &str)> = vec![ + ("ModA.purs", "module ModA where\n\nvalA :: Int\nvalA = undefinedVar\n"), + ]; + + let options = BuildOptions::default(); + let mut cache = ModuleCache::new(); + + // First build: should report type error (undefinedVar is not defined) + let (result1, _, _) = build_from_sources_incremental(&sources, &None, None, &options, &mut cache); + let has_type_errors_1 = result1.modules.iter().any(|m| !m.type_errors.is_empty()); + assert!(has_type_errors_1, "First build should have type errors"); + + // Second build with same sources: error should NOT be cached away + let (result2, _, _) = build_from_sources_incremental(&sources, &None, None, &options, &mut cache); + let has_type_errors_2 = result2.modules.iter().any(|m| !m.type_errors.is_empty()); + assert!(has_type_errors_2, "Second build should still have type errors (not cached)"); +} From c9817d46f3224c834d03eb798b92f289bc488d7a Mon Sep 17 00:00:00 2001 From: Rory Campbell Date: Wed, 11 Mar 2026 12:22:23 +0100 Subject: [PATCH 4/7] prettier errors --- src/main.rs | 2 +- src/typechecker/error.rs | 266 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 267 insertions(+), 1 deletion(-) diff --git a/src/main.rs b/src/main.rs index 68f7a1bd..e726186b 100644 --- a/src/main.rs +++ b/src/main.rs @@ -103,7 +103,7 @@ fn main() { ), None => format!("{}", module.path.display()), }; - error_messages.push(format!("{location}:\n\n{err}")); + error_messages.push(format!("{location}:\n\n {}", err.format_pretty())); } } diff --git a/src/typechecker/error.rs b/src/typechecker/error.rs index 7afdf152..cf9a4ad0 100644 --- a/src/typechecker/error.rs +++ b/src/typechecker/error.rs @@ -1,3 +1,6 @@ +use std::collections::HashMap; +use std::fmt::Write; + use thiserror; use crate::cst::QualifiedIdent; @@ -649,4 +652,267 @@ impl TypeError { } } } + + /// Format the error with readable multi-line layout and normalized type variables. + /// Unification variables like `?120` become `t0`, `t1`, etc. + pub fn format_pretty(&self) -> String { + let var_map = self.build_var_map(); + match self { + TypeError::UnificationError { expected, found, .. } => { + format!( + "Could not match type\n\n {}\n\n with type\n\n {}", + pretty_type(expected, &var_map), + pretty_type(found, &var_map), + ) + } + TypeError::KindsDoNotUnify { expected, found, .. } => { + format!( + "Could not match kind\n\n {}\n\n with kind\n\n {}", + pretty_type(expected, &var_map), + pretty_type(found, &var_map), + ) + } + TypeError::HoleInferredType { name, ty, .. } => { + format!( + "Hole ?{} has the inferred type\n\n {}", + interner::resolve(*name).unwrap_or_default(), + pretty_type(ty, &var_map), + ) + } + TypeError::InfiniteType { var, ty, .. } => { + format!( + "An infinite type was inferred for type variable t{}\n\n {}", + var.0, + pretty_type(ty, &var_map), + ) + } + TypeError::InfiniteKind { var, ty, .. } => { + format!( + "An infinite kind was inferred for type t{}\n\n {}", + var.0, + pretty_type(ty, &var_map), + ) + } + TypeError::NoInstanceFound { class_name, type_args, .. } => { + let args: Vec = type_args.iter().map(|ty| pretty_type(ty, &var_map)).collect(); + format!( + "No type class instance was found for\n\n {} {}", + class_name, + args.join(" "), + ) + } + TypeError::CannotGeneralizeRecursiveFunction { name, type_, .. } => { + format!( + "Unable to generalize the type of the recursive function {}.\n The inferred type was\n\n {}\n\n Try adding a type signature.", + interner::resolve(*name).unwrap_or_default(), + pretty_type(type_, &var_map), + ) + } + TypeError::MissingClassMember { class_name, members, .. } => { + let mut s = format!("The class {} is missing the following members:\n", class_name); + for (name, ty) in members { + let _ = write!(s, "\n {} :: {}", interner::resolve(*name).unwrap_or_default(), pretty_type(ty, &var_map)); + } + s + } + TypeError::EscapedSkolem { name, ty, .. } => { + format!( + "A type variable has escaped its scope: {}\n\n {}", + interner::resolve(*name).unwrap_or_default(), + pretty_type(ty, &var_map), + ) + } + TypeError::ExpectedType { found, .. } => { + format!( + "Expected type of kind Type, but found kind\n\n {}", + pretty_type(found, &var_map), + ) + } + TypeError::CannotApplyExpressionOfTypeOnType { type_, .. } => { + format!( + "Cannot apply expression of type\n\n {}\n\n to a type argument", + pretty_type(type_, &var_map), + ) + } + // For all other errors, use the default Display but with normalized unif vars + _ => { + if var_map.is_empty() { + format!("{self}") + } else { + // Replace ?N patterns in the default display string + let s = format!("{self}"); + replace_unif_vars_in_string(&s, &var_map) + } + } + } + } + + /// Collect all types referenced by this error and build a normalized var mapping. + fn build_var_map(&self) -> HashMap { + let mut unif_ids = Vec::new(); + self.collect_types(&mut |ty| collect_unif_vars(ty, &mut unif_ids)); + let mut map = HashMap::new(); + for id in &unif_ids { + let len = map.len(); + map.entry(*id).or_insert(len); + } + map + } + + /// Visit all Type values in this error variant. + fn collect_types(&self, visitor: &mut dyn FnMut(&Type)) { + match self { + TypeError::UnificationError { expected, found, .. } => { visitor(expected); visitor(found); } + TypeError::InfiniteType { ty, .. } | TypeError::InfiniteKind { ty, .. } => visitor(ty), + TypeError::HoleInferredType { ty, .. } => visitor(ty), + TypeError::NoInstanceFound { type_args, .. } + | TypeError::OverlappingInstances { type_args, .. } + | TypeError::PossiblyInfiniteInstance { type_args, .. } + | TypeError::PossiblyInfiniteCoercibleInstance { type_args, .. } => { + for ty in type_args { visitor(ty); } + } + TypeError::CannotGeneralizeRecursiveFunction { type_, .. } + | TypeError::CannotApplyExpressionOfTypeOnType { type_, .. } => visitor(type_), + TypeError::MissingClassMember { members, .. } => { + for (_, ty) in members { visitor(ty); } + } + TypeError::KindsDoNotUnify { expected, found, .. } => { visitor(expected); visitor(found); } + TypeError::ExpectedType { found, .. } => visitor(found), + TypeError::EscapedSkolem { ty, .. } => visitor(ty), + TypeError::QuantificationCheckFailureInType { ty, .. } + | TypeError::QuantificationCheckFailureInKind { ty, .. } => visitor(ty), + _ => {} + } + } +} + +/// Collect unification variable IDs from a type in order of first appearance. +fn collect_unif_vars(ty: &Type, ids: &mut Vec) { + match ty { + Type::Unif(id) => { + if !ids.contains(&id.0) { + ids.push(id.0); + } + } + Type::App(f, a) => { collect_unif_vars(f, ids); collect_unif_vars(a, ids); } + Type::Fun(a, b) => { collect_unif_vars(a, ids); collect_unif_vars(b, ids); } + Type::Forall(_, t) => collect_unif_vars(t, ids), + Type::Record(fields, tail) => { + for (_, t) in fields { collect_unif_vars(t, ids); } + if let Some(t) = tail { collect_unif_vars(t, ids); } + } + _ => {} + } +} + +/// Format a type with normalized unification variable names. +fn pretty_type(ty: &Type, var_map: &HashMap) -> String { + if var_map.is_empty() { + return format!("{ty}"); + } + let mut out = String::new(); + fmt_type(&mut out, ty, var_map, false); + out +} + +fn fmt_type(out: &mut String, ty: &Type, var_map: &HashMap, nested: bool) { + match ty { + Type::Unif(id) => { + if let Some(&idx) = var_map.get(&id.0) { + let _ = write!(out, "t{idx}"); + } else { + let _ = write!(out, "t{}", id.0); + } + } + Type::Var(sym) => { + let _ = write!(out, "{}", interner::resolve(*sym).unwrap_or_default()); + } + Type::Con(sym) => { + let _ = write!(out, "{sym}"); + } + Type::App(func, arg) => { + if nested { out.push('('); } + match func.as_ref() { + Type::App(..) | Type::Con(..) | Type::Var(..) | Type::Unif(..) => fmt_type(out, func, var_map, false), + _ => fmt_type(out, func, var_map, true), + } + out.push(' '); + fmt_type(out, arg, var_map, true); + if nested { out.push(')'); } + } + Type::Fun(from, to) => { + if nested { out.push('('); } + fmt_type(out, from, var_map, true); + out.push_str(" -> "); + fmt_type(out, to, var_map, false); + if nested { out.push(')'); } + } + Type::Forall(vars, body) => { + if nested { out.push('('); } + out.push_str("forall"); + for (v, visible) in vars { + if *visible { + let _ = write!(out, " @{}", interner::resolve(*v).unwrap_or_default()); + } else { + let _ = write!(out, " {}", interner::resolve(*v).unwrap_or_default()); + } + } + out.push_str(". "); + fmt_type(out, body, var_map, false); + if nested { out.push(')'); } + } + Type::TypeString(sym) => { + let _ = write!(out, "\"{}\"", interner::resolve(*sym).unwrap_or_default()); + } + Type::TypeInt(n) => { + let _ = write!(out, "{n}"); + } + Type::Record(fields, tail) => { + out.push_str("{ "); + for (i, (label, field_ty)) in fields.iter().enumerate() { + if i > 0 { out.push_str(", "); } + let _ = write!(out, "{} :: ", interner::resolve(*label).unwrap_or_default()); + fmt_type(out, field_ty, var_map, false); + } + if let Some(tail) = tail { + if !fields.is_empty() { out.push_str(" | "); } + fmt_type(out, tail, var_map, false); + } + out.push_str(" }"); + } + } +} + +/// Replace `?N` patterns in a pre-formatted string with normalized `tN` names. +fn replace_unif_vars_in_string(s: &str, var_map: &HashMap) -> String { + let mut result = String::with_capacity(s.len()); + let mut chars = s.char_indices().peekable(); + while let Some((i, c)) = chars.next() { + if c == '?' { + // Try to parse a number after '?' + let start = i + 1; + let mut end = start; + while let Some(&(j, d)) = chars.peek() { + if d.is_ascii_digit() { + end = j + 1; + chars.next(); + } else { + break; + } + } + if end > start { + if let Ok(id) = s[start..end].parse::() { + if let Some(&idx) = var_map.get(&id) { + let _ = write!(result, "t{idx}"); + continue; + } + } + } + result.push(c); + result.push_str(&s[start..end]); + } else { + result.push(c); + } + } + result } From bcdc25a3d88112c32d5b719d510778873cfcd0ae Mon Sep 17 00:00:00 2001 From: Rory Campbell Date: Wed, 11 Mar 2026 12:34:55 +0100 Subject: [PATCH 5/7] adds newline to lsp diagnostics before code --- src/lsp/handlers/diagnostics.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lsp/handlers/diagnostics.rs b/src/lsp/handlers/diagnostics.rs index e0aa93ed..9ff71e8f 100644 --- a/src/lsp/handlers/diagnostics.rs +++ b/src/lsp/handlers/diagnostics.rs @@ -149,7 +149,7 @@ fn type_errors_to_diagnostics(errors: &[crate::typechecker::error::TypeError], s severity: Some(DiagnosticSeverity::ERROR), code: Some(NumberOrString::String(format!("TypeError.{}", err.code()))), source: Some("pfc".to_string()), - message: format!("{err}"), + message: format!("{}\n", err.format_pretty()), ..Default::default() } }) From b7d62449b582d0f612455268a711e5b37b00c1d2 Mon Sep 17 00:00:00 2001 From: Rory Campbell Date: Wed, 11 Mar 2026 12:43:09 +0100 Subject: [PATCH 6/7] typecheck on open/init --- src/lsp/handlers/diagnostics.rs | 22 ++++++- src/lsp/handlers/load_sources.rs | 108 ++++++++++++++++++++++++++++++- 2 files changed, 127 insertions(+), 3 deletions(-) diff --git a/src/lsp/handlers/diagnostics.rs b/src/lsp/handlers/diagnostics.rs index 9ff71e8f..e8d86452 100644 --- a/src/lsp/handlers/diagnostics.rs +++ b/src/lsp/handlers/diagnostics.rs @@ -124,9 +124,27 @@ impl Backend { self.info(format!("[on_change] total: {:.2?}", on_change_start.elapsed())).await; } + + /// Re-typecheck all files that are currently open in the editor. + /// Called after initialization completes to process files opened during loading. + pub(crate) async fn typecheck_open_files(&self) { + let open_files: Vec<(String, String)> = { + let files = self.files.read().await; + files.iter().map(|(uri, fs)| (uri.clone(), fs.source.clone())).collect() + }; + if open_files.is_empty() { + return; + } + self.info(format!("[lsp] typechecking {} open file(s) after init", open_files.len())).await; + for (uri_str, source) in open_files { + if let Ok(uri) = Url::parse(&uri_str) { + self.on_change(uri, source).await; + } + } + } } -fn type_errors_to_diagnostics(errors: &[crate::typechecker::error::TypeError], source: &str) -> Vec { +pub(crate) fn type_errors_to_diagnostics(errors: &[crate::typechecker::error::TypeError], source: &str) -> Vec { errors .iter() .map(|err| { @@ -156,7 +174,7 @@ fn type_errors_to_diagnostics(errors: &[crate::typechecker::error::TypeError], s .collect() } -fn error_to_range(err: &crate::diagnostics::CompilerError, source: &str) -> Range { +pub(crate) fn error_to_range(err: &crate::diagnostics::CompilerError, source: &str) -> Range { match err.get_span() { Some(span) => match span.to_pos(source) { Some((start, end)) => Range { diff --git a/src/lsp/handlers/load_sources.rs b/src/lsp/handlers/load_sources.rs index 337d6ba9..8ab77a04 100644 --- a/src/lsp/handlers/load_sources.rs +++ b/src/lsp/handlers/load_sources.rs @@ -1,15 +1,20 @@ use std::collections::HashMap; use std::sync::atomic::Ordering; +use std::sync::Arc; use rayon::prelude::*; +use tokio::sync::RwLock; use tower_lsp::lsp_types::*; +use tower_lsp::Client; use crate::build::cache; +use crate::build::cache::ModuleCache; use crate::build::BuildOptions; use crate::cst::{self, Decl}; use crate::interner; use crate::lsp::utils::find_definition::DefinitionIndex; -use crate::lsp::{CompletionEntry, CompletionEntryKind, CompletionIndex}; +use crate::lsp::{CompletionEntry, CompletionEntryKind, CompletionIndex, FileState}; +use crate::typechecker::registry::ModuleRegistry; use super::super::{Backend, LOAD_STATE_CACHE_LOADED, LOAD_STATE_READY}; @@ -95,6 +100,7 @@ impl Backend { if all_snapshots_loaded { self.load_state.store(LOAD_STATE_READY, Ordering::SeqCst); self.info(format!("[timing] Ready from cache in {:.2?} total", total_start.elapsed())).await; + self.typecheck_open_files().await; return; } @@ -145,6 +151,8 @@ impl Backend { let load_state = self.load_state.clone(); let cache_dir = self.cache_dir.clone(); let progress_token = token.clone(); + let files = self.files.clone(); + let registry = self.registry.clone(); let rt_handle = tokio::runtime::Handle::current(); std::thread::Builder::new() @@ -355,6 +363,9 @@ impl Backend { .await; }); info(format!("[timing] Phase B (index build) total: {:.2?}", build_start.elapsed())); + + // Typecheck any files that were opened during loading + typecheck_open_files(&files, ®istry, &module_cache, &client, &rt); }) .expect("failed to spawn load-sources thread"); } @@ -393,6 +404,7 @@ impl Backend { let load_state = self.load_state.clone(); let cache_dir = self.cache_dir.clone(); let progress_token = token.clone(); + let files = self.files.clone(); let rt_handle = tokio::runtime::Handle::current(); std::thread::Builder::new() @@ -640,11 +652,105 @@ impl Backend { .await; }); info(format!("[timing] Phase B (full build) total: {:.2?}", build_start.elapsed())); + + // Typecheck any files that were opened during loading + typecheck_open_files(&files, ®istry, &module_cache, &client, &rt); }) .expect("failed to spawn load-sources thread"); } } +/// Typecheck any files that were opened in the editor during loading. +/// Called from spawned threads after setting READY. +fn typecheck_open_files( + files: &Arc>>, + registry: &Arc>, + module_cache: &Arc>, + client: &Client, + rt: &tokio::runtime::Handle, +) { + let open_files: Vec<(String, String)> = rt.block_on(async { + let f = files.read().await; + f.iter().map(|(uri, fs)| (uri.clone(), fs.source.clone())).collect() + }); + if open_files.is_empty() { + return; + } + rt.block_on(client.log_message( + MessageType::INFO, + format!("[lsp] typechecking {} open file(s) after init", open_files.len()), + )); + for (uri_str, source) in open_files { + let uri = match Url::parse(&uri_str) { + Ok(u) => u, + Err(_) => continue, + }; + let module = match crate::parser::parse(&source) { + Ok(m) => m, + Err(err) => { + let range = super::diagnostics::error_to_range(&err, &source); + let diagnostics = vec![Diagnostic { + range, + severity: Some(DiagnosticSeverity::ERROR), + code: Some(NumberOrString::String(err.code())), + source: Some("pfc".to_string()), + message: err.get_message(), + ..Default::default() + }]; + rt.block_on(client.publish_diagnostics(uri, diagnostics, None)); + continue; + } + }; + + let module_name = interner::resolve_module_name(&module.name.value.parts); + let module_parts: Vec = module.name.value.parts.clone(); + + rt.block_on(async { + // Update file state with module name + { + let mut f = files.write().await; + if let Some(fs) = f.get_mut(&uri_str) { + fs.module_name = Some(module_name.clone()); + } + } + + let mut reg = registry.write().await; + + // Lazy-load imports from cache + for import_decl in &module.imports { + let import_parts = &import_decl.module.parts; + if reg.lookup(import_parts).is_some() { + continue; + } + let import_name = interner::resolve_module_name(import_parts); + let exports = { + let mut mc = module_cache.write().await; + mc.get_exports(&import_name).cloned() + }; + if let Some(exports) = exports { + reg.register(import_parts, exports); + } + } + + let check_result = crate::typechecker::check_module_with_registry(&module, ®); + reg.register(&module_parts, check_result.exports.clone()); + + let source_hash = ModuleCache::content_hash(&source); + let import_names: Vec = module.imports.iter() + .map(|imp| interner::resolve_module_name(&imp.module.parts)) + .collect(); + let mut mc = module_cache.write().await; + if check_result.errors.is_empty() { + mc.update(module_name.clone(), source_hash, check_result.exports, import_names); + } + drop(mc); + + let diagnostics = super::diagnostics::type_errors_to_diagnostics(&check_result.errors, &source); + client.publish_diagnostics(uri, diagnostics, None).await; + }); + } +} + /// Extract completion entries from a module's CST declarations and source text. fn extract_completion_entries(module: &cst::Module, source: &str) -> Vec { let mut entries = Vec::new(); From 9f8ea8a5b2cfcc4916d5d1dfaff63d6aa3f3941a Mon Sep 17 00:00:00 2001 From: Rory Campbell Date: Wed, 11 Mar 2026 13:53:16 +0100 Subject: [PATCH 7/7] more agressive caching via build plan --- src/build/cache.rs | 478 ++++++++++++++++++++++++++++++- src/build/mod.rs | 89 +++++- src/build/portable.rs | 60 ++-- src/lsp/handlers/diagnostics.rs | 5 +- src/lsp/handlers/load_sources.rs | 3 +- tests/build.rs | 319 +++++++++++++++++++++ 6 files changed, 907 insertions(+), 47 deletions(-) diff --git a/src/build/cache.rs b/src/build/cache.rs index 27e8f155..d2b6529d 100644 --- a/src/build/cache.rs +++ b/src/build/cache.rs @@ -12,10 +12,409 @@ use std::path::{Path, PathBuf}; use serde::{Deserialize, Serialize}; +use crate::cst; +use crate::interner; use crate::typechecker::registry::ModuleExports; use super::portable::{PModuleExports, StringTableBuilder, StringTableReader}; +// ===== Import Details for Smart Rebuild ===== + +/// What a module imports from a single dependency. +#[derive(Serialize, Deserialize, Clone, Debug)] +pub enum CachedImportKind { + /// `import Foo` — imports everything + Everything, + /// `import Foo (bar, Baz(..), class Qux)` — explicit import list + Explicit(Vec), + /// `import Foo hiding (bar)` — everything except listed items + Hiding(Vec), +} + +/// A single imported item. +#[derive(Serialize, Deserialize, Clone, Debug)] +pub enum CachedImportItem { + Value(String), + Type(String), + Class(String), + TypeOp(String), +} + +impl CachedImportItem { + pub fn name(&self) -> &str { + match self { + CachedImportItem::Value(n) + | CachedImportItem::Type(n) + | CachedImportItem::Class(n) + | CachedImportItem::TypeOp(n) => n, + } + } +} + +/// Extract import details from a CST ImportDecl. +pub fn extract_import_items(imports: &[cst::ImportDecl]) -> HashMap { + let mut result = HashMap::new(); + for import in imports { + let module_name = interner::resolve_module_name(&import.module.parts); + let kind = match &import.imports { + None => CachedImportKind::Everything, + Some(cst::ImportList::Explicit(items)) => { + CachedImportKind::Explicit(items.iter().map(convert_import).collect()) + } + Some(cst::ImportList::Hiding(items)) => { + CachedImportKind::Hiding(items.iter().map(convert_import).collect()) + } + }; + result.insert(module_name, kind); + } + result +} + +fn convert_import(import: &cst::Import) -> CachedImportItem { + match import { + cst::Import::Value(name) => { + CachedImportItem::Value(interner::resolve(name.value).unwrap_or_default().to_string()) + } + cst::Import::Type(name, _) => { + CachedImportItem::Type(interner::resolve(name.value).unwrap_or_default().to_string()) + } + cst::Import::Class(name) => { + CachedImportItem::Class(interner::resolve(name.value).unwrap_or_default().to_string()) + } + cst::Import::TypeOp(name) => { + CachedImportItem::TypeOp(interner::resolve(name.value).unwrap_or_default().to_string()) + } + } +} + +// ===== Export Diff for Smart Rebuild ===== + +/// Tracks what changed between old and new exports of a module. +#[derive(Debug, Default)] +pub struct ExportDiff { + pub changed_values: HashSet, + pub changed_types: HashSet, + pub changed_classes: HashSet, + pub instances_changed: bool, + pub operators_changed: bool, +} + +impl ExportDiff { + /// Returns true if nothing actually changed. + pub fn is_empty(&self) -> bool { + self.changed_values.is_empty() + && self.changed_types.is_empty() + && self.changed_classes.is_empty() + && !self.instances_changed + && !self.operators_changed + } + + /// Compute the diff between old and new exports. + pub fn compute(old: &ModuleExports, new: &ModuleExports) -> Self { + let mut diff = ExportDiff::default(); + + // Compare values (type schemes) + diff_map_by_debug(&old.values, &new.values, &mut diff.changed_values); + + // Compare data constructors + diff_map_by_debug(&old.data_constructors, &new.data_constructors, &mut diff.changed_types); + + // Compare constructor details + diff_map_by_debug(&old.ctor_details, &new.ctor_details, &mut diff.changed_types); + + // Compare type aliases + diff_map_by_debug(&old.type_aliases, &new.type_aliases, &mut diff.changed_types); + + // Compare type constructor arities + diff_map_by_eq(&old.type_con_arities, &new.type_con_arities, &mut diff.changed_types); + + // Compare type roles + diff_sym_map_by_eq(&old.type_roles, &new.type_roles, &mut diff.changed_types); + + // Compare newtype names + diff_sym_set(&old.newtype_names, &new.newtype_names, &mut diff.changed_types); + + // Compare type kinds + diff_sym_map_by_debug(&old.type_kinds, &new.type_kinds, &mut diff.changed_types); + + // Compare class methods + diff_map_by_debug(&old.class_methods, &new.class_methods, &mut diff.changed_classes); + + // Compare class param counts + diff_map_by_eq(&old.class_param_counts, &new.class_param_counts, &mut diff.changed_classes); + + // Compare class superclasses + diff_map_by_debug(&old.class_superclasses, &new.class_superclasses, &mut diff.changed_classes); + + // Compare class type kinds + diff_sym_map_by_debug(&old.class_type_kinds, &new.class_type_kinds, &mut diff.changed_classes); + + // Compare class functional dependencies + diff_sym_map_by_debug(&old.class_fundeps, &new.class_fundeps, &mut diff.changed_classes); + + // Compare constrained class methods + diff_qi_set(&old.constrained_class_methods, &new.constrained_class_methods, &mut diff.changed_classes); + + // Compare method own constraints + diff_map_by_debug(&old.method_own_constraints, &new.method_own_constraints, &mut diff.changed_classes); + + // Compare class origins + diff_sym_map_by_eq(&old.class_origins, &new.class_origins, &mut diff.changed_classes); + + // Instances — compare deterministically by sorting keys + diff.instances_changed = !qi_maps_debug_equal(&old.instances, &new.instances); + + // Operators — compare each map deterministically + if !qi_maps_debug_equal(&old.type_operators, &new.type_operators) + || !qi_maps_debug_equal(&old.value_fixities, &new.value_fixities) + || !qi_maps_debug_equal(&old.type_fixities, &new.type_fixities) + || old.function_op_aliases != new.function_op_aliases + || !qi_maps_debug_equal(&old.value_operator_targets, &new.value_operator_targets) + || !sym_maps_debug_equal(&old.operator_class_targets, &new.operator_class_targets) + { + diff.operators_changed = true; + } + + diff + } + + /// Check if a given import kind overlaps with this diff. + pub fn affects_import(&self, import_kind: &CachedImportKind) -> bool { + // Instances are always implicitly imported — any instance change requires rebuild + if self.instances_changed { + return true; + } + + match import_kind { + CachedImportKind::Everything => { + // Wildcard import — any change matters + !self.changed_values.is_empty() + || !self.changed_types.is_empty() + || !self.changed_classes.is_empty() + || self.operators_changed + } + CachedImportKind::Explicit(items) => { + // Only rebuild if an explicitly imported item changed + for item in items { + match item { + CachedImportItem::Value(name) => { + if self.changed_values.contains(name) { + return true; + } + } + CachedImportItem::Type(name) => { + if self.changed_types.contains(name) { + return true; + } + // Type import also brings in constructors as values + if self.changed_values.contains(name) { + return true; + } + } + CachedImportItem::Class(name) => { + if self.changed_classes.contains(name) { + return true; + } + } + CachedImportItem::TypeOp(name) => { + if self.operators_changed { + // Conservative: any operator change + return true; + } + // Also check if the type operator target changed + if self.changed_types.contains(name) { + return true; + } + } + } + } + false + } + CachedImportKind::Hiding(items) => { + // Everything except listed items — check if any changed name is NOT hidden + let hidden: HashSet<&str> = items.iter().map(|i| i.name()).collect(); + + for name in &self.changed_values { + if !hidden.contains(name.as_str()) { + return true; + } + } + for name in &self.changed_types { + if !hidden.contains(name.as_str()) { + return true; + } + } + for name in &self.changed_classes { + if !hidden.contains(name.as_str()) { + return true; + } + } + if self.operators_changed { + return true; // Conservative for operator changes with hiding + } + false + } + } + } +} + +/// Helper: diff two HashMap where V: Debug, collecting changed names. +fn diff_map_by_debug( + old: &HashMap, + new: &HashMap, + changed: &mut HashSet, +) { + for (key, old_val) in old { + match new.get(key) { + None => { changed.insert(format!("{}", key)); } + Some(new_val) => { + if format!("{:?}", old_val) != format!("{:?}", new_val) { + changed.insert(format!("{}", key)); + } + } + } + } + for key in new.keys() { + if !old.contains_key(key) { + changed.insert(format!("{}", key)); + } + } +} + +/// Helper: diff two HashMap where V: PartialEq, collecting changed names. +fn diff_map_by_eq( + old: &HashMap, + new: &HashMap, + changed: &mut HashSet, +) { + for (key, old_val) in old { + match new.get(key) { + None => { changed.insert(format!("{}", key)); } + Some(new_val) => { + if old_val != new_val { + changed.insert(format!("{}", key)); + } + } + } + } + for key in new.keys() { + if !old.contains_key(key) { + changed.insert(format!("{}", key)); + } + } +} + +/// Helper: diff two HashMap where V: Debug, collecting changed names. +fn diff_sym_map_by_debug( + old: &HashMap, + new: &HashMap, + changed: &mut HashSet, +) { + for (key, old_val) in old { + match new.get(key) { + None => { changed.insert(interner::resolve(*key).unwrap_or_default().to_string()); } + Some(new_val) => { + if format!("{:?}", old_val) != format!("{:?}", new_val) { + changed.insert(interner::resolve(*key).unwrap_or_default().to_string()); + } + } + } + } + for key in new.keys() { + if !old.contains_key(key) { + changed.insert(interner::resolve(*key).unwrap_or_default().to_string()); + } + } +} + +/// Helper: diff two HashMap where V: PartialEq, collecting changed names. +fn diff_sym_map_by_eq( + old: &HashMap, + new: &HashMap, + changed: &mut HashSet, +) { + for (key, old_val) in old { + match new.get(key) { + None => { changed.insert(interner::resolve(*key).unwrap_or_default().to_string()); } + Some(new_val) => { + if old_val != new_val { + changed.insert(interner::resolve(*key).unwrap_or_default().to_string()); + } + } + } + } + for key in new.keys() { + if !old.contains_key(key) { + changed.insert(interner::resolve(*key).unwrap_or_default().to_string()); + } + } +} + +/// Helper: diff two HashSet, collecting changed names. +fn diff_sym_set( + old: &HashSet, + new: &HashSet, + changed: &mut HashSet, +) { + for sym in old.symmetric_difference(new) { + changed.insert(interner::resolve(*sym).unwrap_or_default().to_string()); + } +} + +/// Helper: diff two HashSet, collecting changed names. +fn diff_qi_set( + old: &HashSet, + new: &HashSet, + changed: &mut HashSet, +) { + for qi in old.symmetric_difference(new) { + changed.insert(format!("{}", qi)); + } +} + +/// Deterministic equality for HashMap where V: Debug. +/// Sorts keys to avoid non-deterministic HashMap iteration order. +fn qi_maps_debug_equal( + a: &HashMap, + b: &HashMap, +) -> bool { + if a.len() != b.len() { + return false; + } + for (key, a_val) in a { + match b.get(key) { + None => return false, + Some(b_val) => { + if format!("{:?}", a_val) != format!("{:?}", b_val) { + return false; + } + } + } + } + true +} + +/// Deterministic equality for HashMap where V: Debug. +fn sym_maps_debug_equal( + a: &HashMap, + b: &HashMap, +) -> bool { + if a.len() != b.len() { + return false; + } + for (key, a_val) in a { + match b.get(key) { + None => return false, + Some(b_val) => { + if format!("{:?}", a_val) != format!("{:?}", b_val) { + return false; + } + } + } + } + true +} + // ===== Cache Index (loaded eagerly, small) ===== #[derive(Serialize, Deserialize, Default)] @@ -31,6 +430,9 @@ struct CacheIndexEntry { content_hash: u64, exports_hash: u64, imports: Vec, + /// Per-dependency import details for smart rebuild decisions + #[serde(default)] + import_items: HashMap, } // ===== Per-Module Cache File ===== @@ -49,12 +451,14 @@ enum CachedModule { content_hash: u64, exports_hash: u64, imports: Vec, + import_items: HashMap, }, /// Fully loaded in memory (from disk or from typechecking) Loaded { content_hash: u64, exports_hash: u64, imports: Vec, + import_items: HashMap, exports: ModuleExports, dirty: bool, }, @@ -82,6 +486,13 @@ impl CachedModule { } } + fn import_items(&self) -> &HashMap { + match self { + CachedModule::Indexed { import_items, .. } => import_items, + CachedModule::Loaded { import_items, .. } => import_items, + } + } + fn is_dirty(&self) -> bool { match self { CachedModule::Indexed { .. } => false, @@ -169,6 +580,61 @@ impl ModuleCache { } } + /// Smart rebuild check using per-symbol export diffs and import details. + /// + /// Returns true if: + /// - The module is not in the cache + /// - Its content hash has changed + /// - Any import has a diff that affects the symbols this module actually imports + pub fn needs_rebuild_smart( + &self, + module_name: &str, + content_hash: u64, + export_diffs: &HashMap, + ) -> bool { + match self.entries.get(module_name) { + None => { + log::debug!("[build-plan] {module_name}: rebuild (not in cache)"); + true + } + Some(cached) => { + if cached.content_hash() != content_hash { + log::debug!("[build-plan] {module_name}: rebuild (source changed)"); + return true; + } + let my_import_items = cached.import_items(); + for dep in cached.imports() { + if let Some(diff) = export_diffs.get(dep) { + let import_kind = my_import_items.get(dep); + match import_kind { + Some(kind) => { + if diff.affects_import(kind) { + log::debug!( + "[build-plan] {module_name}: rebuild (import from {dep} affected: \ + values={:?}, types={:?}, classes={:?}, instances={}, operators={})", + diff.changed_values, diff.changed_types, diff.changed_classes, + diff.instances_changed, diff.operators_changed + ); + return true; + } + } + None => { + log::debug!("[build-plan] {module_name}: rebuild (no import details for {dep}, conservative)"); + return true; + } + } + } + } + false + } + } + } + + /// Get the cached import details for a module. + pub fn get_import_items(&self, module_name: &str) -> Option<&HashMap> { + self.entries.get(module_name).map(|c| c.import_items()) + } + /// Look up the module name associated with a file path. /// Canonicalizes the path for consistent lookups regardless of relative/absolute form. pub fn module_name_for_path(&self, path: &str) -> Option<&str> { @@ -210,9 +676,9 @@ impl ModuleCache { let module_path = module_file_path(cache_dir, module_name); if let Ok(exports) = load_module_file(&module_path) { if let Some(entry) = self.entries.remove(module_name) { - let (content_hash, exports_hash, imports) = match entry { - CachedModule::Indexed { content_hash, exports_hash, imports } => { - (content_hash, exports_hash, imports) + let (content_hash, exports_hash, imports, import_items) = match entry { + CachedModule::Indexed { content_hash, exports_hash, imports, import_items } => { + (content_hash, exports_hash, imports, import_items) } _ => unreachable!(), }; @@ -220,6 +686,7 @@ impl ModuleCache { content_hash, exports_hash, imports, + import_items, exports, dirty: false, }); @@ -250,6 +717,7 @@ impl ModuleCache { content_hash: u64, exports: ModuleExports, imports: Vec, + import_items: HashMap, ) -> bool { let new_exports_hash = Self::exports_hash(&exports); @@ -260,6 +728,7 @@ impl ModuleCache { content_hash, exports_hash: new_exports_hash, imports, + import_items, exports, dirty: true, }); @@ -345,6 +814,7 @@ impl ModuleCache { content_hash: cached.content_hash(), exports_hash: cached.exports_hash(), imports: cached.imports().to_vec(), + import_items: cached.import_items().clone(), }) }).collect(), path_to_module: self.path_index.clone(), @@ -375,6 +845,7 @@ impl ModuleCache { content_hash: entry.content_hash, exports_hash: entry.exports_hash, imports: entry.imports, + import_items: entry.import_items, }) }).collect(); @@ -427,7 +898,6 @@ fn load_module_file(path: &Path) -> io::Result { // ===== Registry Snapshot (single-file save/load for entire ModuleRegistry) ===== use crate::typechecker::registry::ModuleRegistry; -use crate::interner; #[derive(Serialize, Deserialize)] struct RegistrySnapshot { diff --git a/src/build/mod.rs b/src/build/mod.rs index aa43dfcc..f11357f2 100644 --- a/src/build/mod.rs +++ b/src/build/mod.rs @@ -622,7 +622,7 @@ fn build_from_sources_impl( effective_timeout.map(|t| t.as_secs()).unwrap_or(0)); let mut done = 0usize; - let mut rebuilt_set: HashSet = HashSet::new(); + let mut export_diffs: HashMap = HashMap::new(); let mut cached_count = 0usize; for level in &levels { @@ -635,7 +635,7 @@ fn build_from_sources_impl( { let pm = &parsed[idx]; if let Some(ref mut cache) = cache { - if !cache.needs_rebuild(&pm.module_name, pm.source_hash, &rebuilt_set) { + if !cache.needs_rebuild_smart(&pm.module_name, pm.source_hash, &export_diffs) { if let Some(exports) = cache.get_exports(&pm.module_name) { done += 1; cached_count += 1; @@ -709,22 +709,59 @@ fn build_from_sources_impl( let has_errors = !result.errors.is_empty(); if has_errors { // Don't cache modules with type errors + // Compute a full diff so downstream modules rebuild + let old_exports = cache.as_mut().and_then(|c| c.get_exports(&pm.module_name).cloned()); if let Some(ref mut c) = cache { c.remove(&pm.module_name); } - rebuilt_set.insert(pm.module_name.clone()); + // Treat error modules as having all exports changed + let diff = if let Some(old) = old_exports { + cache::ExportDiff::compute(&old, &result.exports) + } else { + // New module with errors — force downstream rebuild + let mut d = cache::ExportDiff::default(); + d.instances_changed = true; + d + }; + if !diff.is_empty() { + log::debug!( + "[build-plan] {} exports changed: values={:?}, types={:?}, classes={:?}, instances={}, operators={}", + pm.module_name, diff.changed_values, diff.changed_types, diff.changed_classes, + diff.instances_changed, diff.operators_changed + ); + export_diffs.insert(pm.module_name.clone(), diff); + } } else { let import_names: Vec = pm.import_parts.iter() .map(|parts| interner::resolve_module_name(parts)) .collect(); + let module_ref = pm.module.as_ref().unwrap(); + let import_items = cache::extract_import_items(&module_ref.imports); + // Get old exports before updating for diff computation + let old_exports = cache.as_mut().and_then(|c| c.get_exports(&pm.module_name).cloned()); let exports_changed = if let Some(ref mut c) = cache { - c.update(pm.module_name.clone(), pm.source_hash, result.exports.clone(), import_names) + c.update(pm.module_name.clone(), pm.source_hash, result.exports.clone(), import_names, import_items) } else { true }; - // Only add to rebuilt_set if exports actually changed + // Compute per-symbol diff for smart rebuild if exports_changed { - rebuilt_set.insert(pm.module_name.clone()); + let diff = if let Some(old) = old_exports { + cache::ExportDiff::compute(&old, &result.exports) + } else { + // New module — force downstream rebuild + let mut d = cache::ExportDiff::default(); + d.instances_changed = true; + d + }; + if !diff.is_empty() { + log::debug!( + "[build-plan] {} exports changed: values={:?}, types={:?}, classes={:?}, instances={}, operators={}", + pm.module_name, diff.changed_values, diff.changed_types, diff.changed_classes, + diff.instances_changed, diff.operators_changed + ); + export_diffs.insert(pm.module_name.clone(), diff); + } } } // Register exports immediately — result.exports is moved, @@ -757,7 +794,7 @@ fn build_from_sources_impl( for &idx in level.iter() { let pm = &parsed[idx]; if let Some(ref mut cache) = cache { - if !cache.needs_rebuild(&pm.module_name, pm.source_hash, &rebuilt_set) { + if !cache.needs_rebuild_smart(&pm.module_name, pm.source_hash, &export_diffs) { if let Some(exports) = cache.get_exports(&pm.module_name) { done += 1; cached_count += 1; @@ -846,21 +883,53 @@ fn build_from_sources_impl( let has_errors = !result.errors.is_empty(); if has_errors { // Don't cache modules with type errors + let old_exports = cache.as_mut().and_then(|c| c.get_exports(&pm.module_name).cloned()); if let Some(ref mut c) = cache { c.remove(&pm.module_name); } - rebuilt_set.insert(pm.module_name.clone()); + let diff = if let Some(old) = old_exports { + cache::ExportDiff::compute(&old, &result.exports) + } else { + let mut d = cache::ExportDiff::default(); + d.instances_changed = true; + d + }; + if !diff.is_empty() { + log::debug!( + "[build-plan] {} exports changed: values={:?}, types={:?}, classes={:?}, instances={}, operators={}", + pm.module_name, diff.changed_values, diff.changed_types, diff.changed_classes, + diff.instances_changed, diff.operators_changed + ); + export_diffs.insert(pm.module_name.clone(), diff); + } } else { let import_names: Vec = pm.import_parts.iter() .map(|parts| interner::resolve_module_name(parts)) .collect(); + let module_ref = pm.module.as_ref().unwrap(); + let import_items = cache::extract_import_items(&module_ref.imports); + let old_exports = cache.as_mut().and_then(|c| c.get_exports(&pm.module_name).cloned()); let exports_changed = if let Some(ref mut c) = cache { - c.update(pm.module_name.clone(), pm.source_hash, result.exports.clone(), import_names) + c.update(pm.module_name.clone(), pm.source_hash, result.exports.clone(), import_names, import_items) } else { true }; if exports_changed { - rebuilt_set.insert(pm.module_name.clone()); + let diff = if let Some(old) = old_exports { + cache::ExportDiff::compute(&old, &result.exports) + } else { + let mut d = cache::ExportDiff::default(); + d.instances_changed = true; + d + }; + if !diff.is_empty() { + log::debug!( + "[build-plan] {} exports changed: values={:?}, types={:?}, classes={:?}, instances={}, operators={}", + pm.module_name, diff.changed_values, diff.changed_types, diff.changed_classes, + diff.instances_changed, diff.operators_changed + ); + export_diffs.insert(pm.module_name.clone(), diff); + } } } registry.register(&pm.module_parts, result.exports); diff --git a/src/build/portable.rs b/src/build/portable.rs index c219ba23..aeeec1b2 100644 --- a/src/build/portable.rs +++ b/src/build/portable.rs @@ -3,7 +3,7 @@ //! Uses a deduplicated string table so each symbol is stored once. //! Symbol references are u32 indices into the string table. -use std::collections::{HashMap, HashSet}; +use std::collections::{BTreeMap, BTreeSet, HashMap}; use serde::{Deserialize, Serialize}; @@ -64,7 +64,7 @@ impl StringTableReader { // ===== Portable QualifiedIdent ===== -#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Eq, Hash)] +#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Eq, Hash, PartialOrd, Ord)] pub struct PQI { pub module: Option, pub name: u32, @@ -227,34 +227,34 @@ fn rest_role(p: &PRole) -> Role { #[derive(Serialize, Deserialize, Clone, Debug)] pub struct PModuleExports { - pub values: HashMap, - pub class_methods: HashMap)>, - pub data_constructors: HashMap>, - pub ctor_details: HashMap, Vec)>, - pub instances: HashMap, Vec<(PQI, Vec)>)>>, - pub type_operators: HashMap, - pub value_fixities: HashMap, - pub type_fixities: HashMap, - pub function_op_aliases: HashSet, - pub value_operator_targets: HashMap, - pub constrained_class_methods: HashSet, - pub type_aliases: HashMap, PType)>, - pub class_param_counts: HashMap, - pub value_origins: HashMap, - pub type_origins: HashMap, - pub class_origins: HashMap, - pub operator_class_targets: HashMap, - pub class_fundeps: HashMap, Vec<(Vec, Vec)>)>, - pub type_con_arities: HashMap, - pub type_roles: HashMap>, - pub newtype_names: HashSet, - pub signature_constraints: HashMap)>>, - pub type_kinds: HashMap, - pub class_type_kinds: HashMap, - pub partial_dischargers: HashSet, - pub self_referential_aliases: HashSet, - pub class_superclasses: HashMap, Vec<(PQI, Vec)>)>, - pub method_own_constraints: HashMap>, + pub values: BTreeMap, + pub class_methods: BTreeMap)>, + pub data_constructors: BTreeMap>, + pub ctor_details: BTreeMap, Vec)>, + pub instances: BTreeMap, Vec<(PQI, Vec)>)>>, + pub type_operators: BTreeMap, + pub value_fixities: BTreeMap, + pub type_fixities: BTreeMap, + pub function_op_aliases: BTreeSet, + pub value_operator_targets: BTreeMap, + pub constrained_class_methods: BTreeSet, + pub type_aliases: BTreeMap, PType)>, + pub class_param_counts: BTreeMap, + pub value_origins: BTreeMap, + pub type_origins: BTreeMap, + pub class_origins: BTreeMap, + pub operator_class_targets: BTreeMap, + pub class_fundeps: BTreeMap, Vec<(Vec, Vec)>)>, + pub type_con_arities: BTreeMap, + pub type_roles: BTreeMap>, + pub newtype_names: BTreeSet, + pub signature_constraints: BTreeMap)>>, + pub type_kinds: BTreeMap, + pub class_type_kinds: BTreeMap, + pub partial_dischargers: BTreeSet, + pub self_referential_aliases: BTreeSet, + pub class_superclasses: BTreeMap, Vec<(PQI, Vec)>)>, + pub method_own_constraints: BTreeMap>, } impl PModuleExports { diff --git a/src/lsp/handlers/diagnostics.rs b/src/lsp/handlers/diagnostics.rs index e8d86452..c229cbb9 100644 --- a/src/lsp/handlers/diagnostics.rs +++ b/src/lsp/handlers/diagnostics.rs @@ -4,7 +4,7 @@ use tower_lsp::lsp_types::*; use crate::cst::Module; use crate::interner; -use crate::build::cache::ModuleCache; +use crate::build::cache::{ModuleCache, extract_import_items}; use crate::typechecker::registry::ModuleRegistry; use super::super::{Backend, FileState}; @@ -112,8 +112,9 @@ impl Backend { let import_names: Vec = module.imports.iter() .map(|imp| interner::resolve_module_name(&imp.module.parts)) .collect(); + let import_items = extract_import_items(&module.imports); let mut cache = self.module_cache.write().await; - cache.update(module_name.clone(), source_hash, check_result.exports, import_names); + cache.update(module_name.clone(), source_hash, check_result.exports, import_names, import_items); drop(cache); // Publish diagnostics for the changed module diff --git a/src/lsp/handlers/load_sources.rs b/src/lsp/handlers/load_sources.rs index 8ab77a04..7a67b19b 100644 --- a/src/lsp/handlers/load_sources.rs +++ b/src/lsp/handlers/load_sources.rs @@ -739,9 +739,10 @@ fn typecheck_open_files( let import_names: Vec = module.imports.iter() .map(|imp| interner::resolve_module_name(&imp.module.parts)) .collect(); + let import_items = cache::extract_import_items(&module.imports); let mut mc = module_cache.write().await; if check_result.errors.is_empty() { - mc.update(module_name.clone(), source_hash, check_result.exports, import_names); + mc.update(module_name.clone(), source_hash, check_result.exports, import_names, import_items); } drop(mc); diff --git a/tests/build.rs b/tests/build.rs index fcc48233..5f2ee902 100644 --- a/tests/build.rs +++ b/tests/build.rs @@ -1340,3 +1340,322 @@ fn incremental_build_does_not_cache_errors() { let has_type_errors_2 = result2.modules.iter().any(|m| !m.type_errors.is_empty()); assert!(has_type_errors_2, "Second build should still have type errors (not cached)"); } + +// ===== Smart rebuild tests ===== +// +// These tests verify the import-aware rebuild skipping logic: +// - Modules that import unchanged symbols should be SKIPPED +// - Modules that import changed symbols MUST be rebuilt + +/// Helper: check if a module was cached (skipped) in a build result +fn was_cached(result: &purescript_fast_compiler::build::BuildResult, module_name: &str) -> bool { + result.modules.iter() + .find(|m| m.module_name == module_name) + .map_or(false, |m| m.cached) +} + +// --- CORRECTNESS: modules that MUST be rebuilt --- + +#[test] +fn smart_rebuild_changed_imported_value_type() { + // ModA exports foo :: Int, ModB imports foo. + // Change foo :: Int to foo :: String → ModB MUST rebuild + let sources_v1: Vec<(&str, &str)> = vec![ + ("ModA.purs", "module ModA where\n\nfoo :: Int\nfoo = 42\n\nbar :: String\nbar = \"hi\"\n"), + ("ModB.purs", "module ModB where\n\nimport ModA (foo)\n\nvalB :: Int\nvalB = foo\n"), + ]; + let options = BuildOptions::default(); + let mut cache = ModuleCache::new(); + + let (r1, _, _) = build_from_sources_incremental(&sources_v1, &None, None, &options, &mut cache); + assert!(r1.build_errors.is_empty()); + assert!(r1.modules.iter().all(|m| m.type_errors.is_empty())); + + // Change foo's type from Int to String + let sources_v2: Vec<(&str, &str)> = vec![ + ("ModA.purs", "module ModA where\n\nfoo :: String\nfoo = \"changed\"\n\nbar :: String\nbar = \"hi\"\n"), + ("ModB.purs", "module ModB where\n\nimport ModA (foo)\n\nvalB :: Int\nvalB = foo\n"), + ]; + let (r2, _, _) = build_from_sources_incremental(&sources_v2, &None, None, &options, &mut cache); + // ModB should be rebuilt and now have a type error (Int vs String) + assert!(!was_cached(&r2, "ModB"), "ModB must be rebuilt when imported value type changes"); + let mod_b_errors = r2.modules.iter().find(|m| m.module_name == "ModB").unwrap(); + assert!(!mod_b_errors.type_errors.is_empty(), "ModB should have type error after foo changed type"); +} + +#[test] +fn smart_rebuild_changed_imported_type_constructors() { + // ModA exports data T = A | B, ModB imports T(..) + // Add constructor C → ModB MUST rebuild + let sources_v1: Vec<(&str, &str)> = vec![ + ("ModA.purs", "module ModA where\n\ndata T = A | B\n"), + ("ModB.purs", "module ModB where\n\nimport ModA (T(..))\n\nfoo :: T\nfoo = A\n"), + ]; + let options = BuildOptions::default(); + let mut cache = ModuleCache::new(); + + let (r1, _, _) = build_from_sources_incremental(&sources_v1, &None, None, &options, &mut cache); + assert!(r1.build_errors.is_empty()); + + let sources_v2: Vec<(&str, &str)> = vec![ + ("ModA.purs", "module ModA where\n\ndata T = A | B | C\n"), + ("ModB.purs", "module ModB where\n\nimport ModA (T(..))\n\nfoo :: T\nfoo = A\n"), + ]; + let (r2, _, _) = build_from_sources_incremental(&sources_v2, &None, None, &options, &mut cache); + assert!(!was_cached(&r2, "ModB"), "ModB must rebuild when T's constructors change"); +} + +#[test] +fn smart_rebuild_wildcard_import_any_change() { + // ModB uses wildcard import from ModA — any change must trigger rebuild + let sources_v1: Vec<(&str, &str)> = vec![ + ("ModA.purs", "module ModA where\n\nfoo :: Int\nfoo = 1\n"), + ("ModB.purs", "module ModB where\n\nimport ModA\n\nvalB :: Int\nvalB = foo\n"), + ]; + let options = BuildOptions::default(); + let mut cache = ModuleCache::new(); + + let (r1, _, _) = build_from_sources_incremental(&sources_v1, &None, None, &options, &mut cache); + assert!(r1.build_errors.is_empty()); + + // Add a new export to ModA + let sources_v2: Vec<(&str, &str)> = vec![ + ("ModA.purs", "module ModA where\n\nfoo :: Int\nfoo = 1\n\nbaz :: String\nbaz = \"new\"\n"), + ("ModB.purs", "module ModB where\n\nimport ModA\n\nvalB :: Int\nvalB = foo\n"), + ]; + let (r2, _, _) = build_from_sources_incremental(&sources_v2, &None, None, &options, &mut cache); + assert!(!was_cached(&r2, "ModB"), "ModB must rebuild on wildcard import when any export changes"); +} + +#[test] +fn smart_rebuild_transitive_chain() { + // A→B→C chain. Change A's exported type → B must rebuild → C must rebuild + let sources_v1: Vec<(&str, &str)> = vec![ + ("ModA.purs", "module ModA where\n\nfoo :: Int\nfoo = 1\n"), + ("ModB.purs", "module ModB where\n\nimport ModA (foo)\n\nbar :: Int\nbar = foo\n"), + ("ModC.purs", "module ModC where\n\nimport ModB (bar)\n\nbaz :: Int\nbaz = bar\n"), + ]; + let options = BuildOptions::default(); + let mut cache = ModuleCache::new(); + + let (r1, _, _) = build_from_sources_incremental(&sources_v1, &None, None, &options, &mut cache); + assert!(r1.build_errors.is_empty()); + assert!(r1.modules.iter().all(|m| m.type_errors.is_empty())); + + // Change foo's type + let sources_v2: Vec<(&str, &str)> = vec![ + ("ModA.purs", "module ModA where\n\nfoo :: String\nfoo = \"changed\"\n"), + ("ModB.purs", "module ModB where\n\nimport ModA (foo)\n\nbar :: Int\nbar = foo\n"), + ("ModC.purs", "module ModC where\n\nimport ModB (bar)\n\nbaz :: Int\nbaz = bar\n"), + ]; + let (r2, _, _) = build_from_sources_incremental(&sources_v2, &None, None, &options, &mut cache); + assert!(!was_cached(&r2, "ModB"), "ModB must rebuild when foo's type changes"); + // ModB will now have errors, which means C should also rebuild + // (error modules get an export diff) +} + +// --- OPTIMIZATION: modules that SHOULD be skipped --- + +#[test] +fn smart_rebuild_skip_unused_value_change() { + // ModA exports foo and bar, ModB imports only foo. + // Change bar's type → ModB should be SKIPPED + let sources_v1: Vec<(&str, &str)> = vec![ + ("ModA.purs", "module ModA where\n\nfoo :: Int\nfoo = 42\n\nbar :: String\nbar = \"hello\"\n"), + ("ModB.purs", "module ModB where\n\nimport ModA (foo)\n\nvalB :: Int\nvalB = foo\n"), + ]; + let options = BuildOptions::default(); + let mut cache = ModuleCache::new(); + + let (r1, _, _) = build_from_sources_incremental(&sources_v1, &None, None, &options, &mut cache); + assert!(r1.build_errors.is_empty()); + assert!(r1.modules.iter().all(|m| m.type_errors.is_empty())); + + // Change bar's type (foo stays the same) + let sources_v2: Vec<(&str, &str)> = vec![ + ("ModA.purs", "module ModA where\n\nfoo :: Int\nfoo = 42\n\nbar :: Boolean\nbar = true\n"), + ("ModB.purs", "module ModB where\n\nimport ModA (foo)\n\nvalB :: Int\nvalB = foo\n"), + ]; + let (r2, _, _) = build_from_sources_incremental(&sources_v2, &None, None, &options, &mut cache); + assert!(was_cached(&r2, "ModB"), "ModB should be skipped when only bar (not imported) changed"); +} + +#[test] +fn smart_rebuild_skip_body_only_change() { + // Change function body only (same types) → downstream should be SKIPPED + let sources_v1: Vec<(&str, &str)> = vec![ + ("ModA.purs", "module ModA where\n\nfoo :: Int\nfoo = 42\n"), + ("ModB.purs", "module ModB where\n\nimport ModA (foo)\n\nvalB :: Int\nvalB = foo\n"), + ]; + let options = BuildOptions::default(); + let mut cache = ModuleCache::new(); + + let (r1, _, _) = build_from_sources_incremental(&sources_v1, &None, None, &options, &mut cache); + assert!(r1.build_errors.is_empty()); + + // Change foo's body, not its type + let sources_v2: Vec<(&str, &str)> = vec![ + ("ModA.purs", "module ModA where\n\nfoo :: Int\nfoo = 99\n"), + ("ModB.purs", "module ModB where\n\nimport ModA (foo)\n\nvalB :: Int\nvalB = foo\n"), + ]; + let (r2, _, _) = build_from_sources_incremental(&sources_v2, &None, None, &options, &mut cache); + assert!(was_cached(&r2, "ModB"), "ModB should be skipped when only foo's body changed (type unchanged)"); +} + +#[test] +fn smart_rebuild_skip_new_export_not_imported() { + // ModA adds a new export baz, ModB explicitly imports only foo → skip + let sources_v1: Vec<(&str, &str)> = vec![ + ("ModA.purs", "module ModA where\n\nfoo :: Int\nfoo = 42\n"), + ("ModB.purs", "module ModB where\n\nimport ModA (foo)\n\nvalB :: Int\nvalB = foo\n"), + ]; + let options = BuildOptions::default(); + let mut cache = ModuleCache::new(); + + let (r1, _, _) = build_from_sources_incremental(&sources_v1, &None, None, &options, &mut cache); + assert!(r1.build_errors.is_empty()); + + // Add baz to ModA + let sources_v2: Vec<(&str, &str)> = vec![ + ("ModA.purs", "module ModA where\n\nfoo :: Int\nfoo = 42\n\nbaz :: String\nbaz = \"new\"\n"), + ("ModB.purs", "module ModB where\n\nimport ModA (foo)\n\nvalB :: Int\nvalB = foo\n"), + ]; + let (r2, _, _) = build_from_sources_incremental(&sources_v2, &None, None, &options, &mut cache); + assert!(was_cached(&r2, "ModB"), "ModB should be skipped when ModA adds a new export that ModB doesn't import"); +} + +#[test] +fn smart_rebuild_skip_chain() { + // A→B→C. Change in A doesn't affect what B imports → B skipped → C skipped + let sources_v1: Vec<(&str, &str)> = vec![ + ("ModA.purs", "module ModA where\n\nfoo :: Int\nfoo = 1\n\nbar :: String\nbar = \"x\"\n"), + ("ModB.purs", "module ModB where\n\nimport ModA (foo)\n\nbaz :: Int\nbaz = foo\n"), + ("ModC.purs", "module ModC where\n\nimport ModB (baz)\n\nqux :: Int\nqux = baz\n"), + ]; + let options = BuildOptions::default(); + let mut cache = ModuleCache::new(); + + let (r1, _, _) = build_from_sources_incremental(&sources_v1, &None, None, &options, &mut cache); + assert!(r1.build_errors.is_empty()); + assert!(r1.modules.iter().all(|m| m.type_errors.is_empty())); + + // Change bar in ModA (ModB doesn't import bar) + let sources_v2: Vec<(&str, &str)> = vec![ + ("ModA.purs", "module ModA where\n\nfoo :: Int\nfoo = 1\n\nbar :: Boolean\nbar = true\n"), + ("ModB.purs", "module ModB where\n\nimport ModA (foo)\n\nbaz :: Int\nbaz = foo\n"), + ("ModC.purs", "module ModC where\n\nimport ModB (baz)\n\nqux :: Int\nqux = baz\n"), + ]; + let (r2, _, _) = build_from_sources_incremental(&sources_v2, &None, None, &options, &mut cache); + assert!(was_cached(&r2, "ModB"), "ModB should be skipped (only bar changed, imports foo)"); + assert!(was_cached(&r2, "ModC"), "ModC should be skipped (ModB was skipped)"); +} + +#[test] +fn smart_rebuild_hiding_import_skip() { + // ModB does `import ModA hiding (bar)`. Change bar → ModB should be SKIPPED + let sources_v1: Vec<(&str, &str)> = vec![ + ("ModA.purs", "module ModA where\n\nfoo :: Int\nfoo = 1\n\nbar :: String\nbar = \"x\"\n"), + ("ModB.purs", "module ModB where\n\nimport ModA hiding (bar)\n\nvalB :: Int\nvalB = foo\n"), + ]; + let options = BuildOptions::default(); + let mut cache = ModuleCache::new(); + + let (r1, _, _) = build_from_sources_incremental(&sources_v1, &None, None, &options, &mut cache); + assert!(r1.build_errors.is_empty()); + + // Change bar's type + let sources_v2: Vec<(&str, &str)> = vec![ + ("ModA.purs", "module ModA where\n\nfoo :: Int\nfoo = 1\n\nbar :: Boolean\nbar = true\n"), + ("ModB.purs", "module ModB where\n\nimport ModA hiding (bar)\n\nvalB :: Int\nvalB = foo\n"), + ]; + let (r2, _, _) = build_from_sources_incremental(&sources_v2, &None, None, &options, &mut cache); + assert!(was_cached(&r2, "ModB"), "ModB should be skipped when hidden import bar changes"); +} + +#[test] +fn smart_rebuild_hiding_import_rebuild() { + // ModB does `import ModA hiding (bar)`. Change foo → ModB MUST rebuild + let sources_v1: Vec<(&str, &str)> = vec![ + ("ModA.purs", "module ModA where\n\nfoo :: Int\nfoo = 1\n\nbar :: String\nbar = \"x\"\n"), + ("ModB.purs", "module ModB where\n\nimport ModA hiding (bar)\n\nvalB :: Int\nvalB = foo\n"), + ]; + let options = BuildOptions::default(); + let mut cache = ModuleCache::new(); + + let (r1, _, _) = build_from_sources_incremental(&sources_v1, &None, None, &options, &mut cache); + assert!(r1.build_errors.is_empty()); + + // Change foo's type (not hidden) + let sources_v2: Vec<(&str, &str)> = vec![ + ("ModA.purs", "module ModA where\n\nfoo :: String\nfoo = \"changed\"\n\nbar :: String\nbar = \"x\"\n"), + ("ModB.purs", "module ModB where\n\nimport ModA hiding (bar)\n\nvalB :: Int\nvalB = foo\n"), + ]; + let (r2, _, _) = build_from_sources_incremental(&sources_v2, &None, None, &options, &mut cache); + assert!(!was_cached(&r2, "ModB"), "ModB must rebuild when non-hidden foo changes type"); +} + +#[test] +fn smart_rebuild_unchanged_source_skipped() { + // No changes at all → everything cached + let sources: Vec<(&str, &str)> = vec![ + ("ModA.purs", "module ModA where\n\nfoo :: Int\nfoo = 1\n"), + ("ModB.purs", "module ModB where\n\nimport ModA (foo)\n\nbar :: Int\nbar = foo\n"), + ]; + let options = BuildOptions::default(); + let mut cache = ModuleCache::new(); + + let (r1, _, _) = build_from_sources_incremental(&sources, &None, None, &options, &mut cache); + assert!(r1.build_errors.is_empty()); + + let (r2, _, _) = build_from_sources_incremental(&sources, &None, None, &options, &mut cache); + assert!(was_cached(&r2, "ModA"), "ModA should be cached (no changes)"); + assert!(was_cached(&r2, "ModB"), "ModB should be cached (no changes)"); +} + +#[test] +fn smart_rebuild_multiple_imports_partial_change() { + // ModC imports from both ModA and ModB. Only ModA changes, but ModC only imports + // the unchanged export from ModA. + let sources_v1: Vec<(&str, &str)> = vec![ + ("ModA.purs", "module ModA where\n\nfoo :: Int\nfoo = 1\n\nbar :: String\nbar = \"x\"\n"), + ("ModB.purs", "module ModB where\n\nbaz :: Boolean\nbaz = true\n"), + ("ModC.purs", "module ModC where\n\nimport ModA (foo)\nimport ModB (baz)\n\nqux :: Int\nqux = foo\n"), + ]; + let options = BuildOptions::default(); + let mut cache = ModuleCache::new(); + + let (r1, _, _) = build_from_sources_incremental(&sources_v1, &None, None, &options, &mut cache); + assert!(r1.build_errors.is_empty()); + + // Change bar in ModA (ModC doesn't import bar) + let sources_v2: Vec<(&str, &str)> = vec![ + ("ModA.purs", "module ModA where\n\nfoo :: Int\nfoo = 1\n\nbar :: Boolean\nbar = true\n"), + ("ModB.purs", "module ModB where\n\nbaz :: Boolean\nbaz = true\n"), + ("ModC.purs", "module ModC where\n\nimport ModA (foo)\nimport ModB (baz)\n\nqux :: Int\nqux = foo\n"), + ]; + let (r2, _, _) = build_from_sources_incremental(&sources_v2, &None, None, &options, &mut cache); + assert!(was_cached(&r2, "ModC"), "ModC should be skipped (only bar changed, ModC imports foo)"); +} + +#[test] +fn smart_rebuild_error_module_forces_downstream_rebuild() { + // Module with errors should force downstream rebuild + let sources_v1: Vec<(&str, &str)> = vec![ + ("ModA.purs", "module ModA where\n\nfoo :: Int\nfoo = 42\n"), + ("ModB.purs", "module ModB where\n\nimport ModA (foo)\n\nbar :: Int\nbar = foo\n"), + ]; + let options = BuildOptions::default(); + let mut cache = ModuleCache::new(); + + let (r1, _, _) = build_from_sources_incremental(&sources_v1, &None, None, &options, &mut cache); + assert!(r1.build_errors.is_empty()); + + // Introduce error in ModA + let sources_v2: Vec<(&str, &str)> = vec![ + ("ModA.purs", "module ModA where\n\nfoo :: Int\nfoo = undefinedVar\n"), + ("ModB.purs", "module ModB where\n\nimport ModA (foo)\n\nbar :: Int\nbar = foo\n"), + ]; + let (r2, _, _) = build_from_sources_incremental(&sources_v2, &None, None, &options, &mut cache); + // The build stops on first error module, but ModA should not be cached + assert!(!was_cached(&r2, "ModA"), "ModA with errors must not be cached"); +}