Merge pull request #4688 from roc-lang/i4671

Correct generalization of imported specializations
This commit is contained in:
Folkert de Vries 2022-12-05 23:22:58 +01:00 committed by GitHub
commit 66ce640fe5
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
6 changed files with 214 additions and 87 deletions

View file

@ -488,17 +488,19 @@ impl Constraints {
/// then need to find their way to the pool, and a convenient approach turned out to be to
/// tag them onto the `Let` that we used to add the imported values.
#[inline(always)]
pub fn let_import_constraint<I1, I2>(
pub fn let_import_constraint<I1, I2, I3>(
&mut self,
rigid_vars: I1,
def_types: I2,
flex_vars: I2,
def_types: I3,
module_constraint: Constraint,
pool_variables: &[Variable],
) -> Constraint
where
I1: IntoIterator<Item = Variable>,
I2: IntoIterator<Item = (Symbol, Loc<TypeOrVar>)>,
I2::IntoIter: ExactSizeIterator,
I2: IntoIterator<Item = Variable>,
I3: IntoIterator<Item = (Symbol, Loc<TypeOrVar>)>,
I3::IntoIter: ExactSizeIterator,
{
// defs and ret constraint are stored consequtively, so we only need to store one index
let defs_and_ret_constraint = Index::new(self.constraints.len() as _);
@ -508,7 +510,7 @@ impl Constraints {
let let_contraint = LetConstraint {
rigid_vars: self.variable_slice(rigid_vars),
flex_vars: Slice::default(),
flex_vars: self.variable_slice(flex_vars),
def_types: self.def_types_slice(def_types),
defs_and_ret_constraint,
};

View file

@ -1,3 +1,5 @@
#![allow(clippy::too_many_arguments)]
use crate::docs::ModuleDocumentation;
use bumpalo::Bump;
use crossbeam::channel::{bounded, Sender};
@ -51,7 +53,7 @@ use roc_reporting::report::{Annotation, Palette, RenderTarget};
use roc_solve::module::{extract_module_owned_implementations, Solved, SolvedModule};
use roc_solve_problem::TypeError;
use roc_target::TargetInfo;
use roc_types::subs::{ExposedTypesStorageSubs, Subs, VarStore, Variable};
use roc_types::subs::{CopiedImport, ExposedTypesStorageSubs, Subs, VarStore, Variable};
use roc_types::types::{Alias, Types};
use std::collections::hash_map::Entry::{Occupied, Vacant};
use std::collections::HashMap;
@ -976,7 +978,6 @@ impl<'a> State<'a> {
self.exec_mode.goal_phase()
}
#[allow(clippy::too_many_arguments)]
fn new(
root_id: ModuleId,
target_info: TargetInfo,
@ -1221,7 +1222,6 @@ fn enqueue_task<'a>(
Ok(())
}
#[allow(clippy::too_many_arguments)]
pub fn load_and_typecheck_str<'a>(
arena: &'a Bump,
filename: PathBuf,
@ -1508,7 +1508,6 @@ pub enum Threading {
/// and then linking them together, and possibly caching them by the hash of their
/// specializations, so if none of their specializations changed, we don't even need
/// to rebuild the module and can link in the cached one directly.)
#[allow(clippy::too_many_arguments)]
pub fn load<'a>(
arena: &'a Bump,
load_start: LoadStart<'a>,
@ -1569,7 +1568,6 @@ pub fn load<'a>(
}
/// Load using only a single thread; used when compiling to webassembly
#[allow(clippy::too_many_arguments)]
pub fn load_single_threaded<'a>(
arena: &'a Bump,
load_start: LoadStart<'a>,
@ -1834,7 +1832,6 @@ fn state_thread_step<'a>(
}
}
#[allow(clippy::too_many_arguments)]
fn load_multi_threaded<'a>(
arena: &'a Bump,
load_start: LoadStart<'a>,
@ -2010,7 +2007,6 @@ fn load_multi_threaded<'a>(
.unwrap()
}
#[allow(clippy::too_many_arguments)]
fn worker_task_step<'a>(
worker_arena: &'a Bump,
worker: &Worker<BuildTask<'a>>,
@ -2085,7 +2081,6 @@ fn worker_task_step<'a>(
}
}
#[allow(clippy::too_many_arguments)]
fn worker_task<'a>(
worker_arena: &'a Bump,
worker: Worker<BuildTask<'a>>,
@ -3214,7 +3209,6 @@ fn finish_specialization<'a>(
})
}
#[allow(clippy::too_many_arguments)]
fn finish(
mut state: State,
solved: Solved<Subs>,
@ -3653,7 +3647,6 @@ fn verify_interface_matches_file_path<'a>(
Err(problem)
}
#[allow(clippy::too_many_arguments)]
fn parse_header<'a>(
arena: &'a Bump,
read_file_duration: Duration,
@ -3940,7 +3933,6 @@ fn parse_header<'a>(
}
/// Load a module by its filename
#[allow(clippy::too_many_arguments)]
fn load_filename<'a>(
arena: &'a Bump,
filename: PathBuf,
@ -3979,7 +3971,6 @@ fn load_filename<'a>(
/// Load a module from a str
/// the `filename` is never read, but used for the module name
#[allow(clippy::too_many_arguments)]
fn load_from_str<'a>(
arena: &'a Bump,
filename: PathBuf,
@ -4019,7 +4010,6 @@ struct HeaderInfo<'a> {
extra: HeaderFor<'a>,
}
#[allow(clippy::too_many_arguments)]
fn build_header<'a>(
info: HeaderInfo<'a>,
parse_state: roc_parse::state::State<'a>,
@ -4245,7 +4235,6 @@ struct PlatformHeaderInfo<'a> {
}
// TODO refactor so more logic is shared with `send_header`
#[allow(clippy::too_many_arguments)]
fn send_header_two<'a>(
info: PlatformHeaderInfo<'a>,
parse_state: roc_parse::state::State<'a>,
@ -4497,7 +4486,6 @@ fn send_header_two<'a>(
impl<'a> BuildTask<'a> {
// TODO trim down these arguments - possibly by moving Constraint into Module
#[allow(clippy::too_many_arguments)]
fn solve_module(
module: Module,
ident_ids: IdentIds,
@ -4584,61 +4572,26 @@ pub fn add_imports(
mut pending_abilities: PendingAbilitiesStore,
exposed_for_module: &ExposedForModule,
def_types: &mut Vec<(Symbol, Loc<TypeOrVar>)>,
rigid_vars: &mut Vec<Variable>,
imported_rigid_vars: &mut Vec<Variable>,
imported_flex_vars: &mut Vec<Variable>,
) -> (Vec<Variable>, AbilitiesStore) {
let mut import_variables = Vec::new();
let mut cached_symbol_vars = VecMap::default();
macro_rules! import_var_for_symbol {
($subs:expr, $exposed_by_module:expr, $symbol:ident, $break:stmt) => {
let module_id = $symbol.module_id();
match $exposed_by_module.get(&module_id) {
Some(ExposedModuleTypes {
exposed_types_storage_subs: exposed_types,
resolved_implementations: _,
}) => {
let variable = match exposed_types.stored_vars_by_symbol.iter().find(|(s, _)| **s == $symbol) {
None => {
// Today we define builtins in each module that uses them
// so even though they have a different module name from
// the surrounding module, they are not technically imported
debug_assert!($symbol.is_builtin());
$break
}
Some((_, x)) => *x,
};
let copied_import = exposed_types.storage_subs.export_variable_to($subs, variable);
let copied_import_index = constraints.push_variable(copied_import.variable);
def_types.push((
$symbol,
Loc::at_zero(copied_import_index),
));
// not a typo; rigids are turned into flex during type inference, but when imported we must
// consider them rigid variables
rigid_vars.extend(copied_import.rigid);
rigid_vars.extend(copied_import.flex);
// Rigid vars bound to abilities are also treated like rigids.
rigid_vars.extend(copied_import.rigid_able);
rigid_vars.extend(copied_import.flex_able);
import_variables.extend(copied_import.registered);
cached_symbol_vars.insert($symbol, copied_import.variable);
}
None => {
internal_error!("Imported module {:?} is not available", module_id)
}
}
}
}
for &symbol in &exposed_for_module.imported_values {
import_var_for_symbol!(subs, exposed_for_module.exposed_by_module, symbol, continue);
import_variable_for_symbol(
subs,
constraints,
def_types,
&mut import_variables,
imported_rigid_vars,
imported_flex_vars,
&mut cached_symbol_vars,
&exposed_for_module.exposed_by_module,
symbol,
OnSymbolNotFound::AssertIsBuiltin,
);
}
// Patch used symbols from circular dependencies.
@ -4665,6 +4618,9 @@ pub fn add_imports(
struct Ctx<'a> {
subs: &'a mut Subs,
exposed_by_module: &'a ExposedByModule,
imported_variables: &'a mut Vec<Variable>,
imported_rigids: &'a mut Vec<Variable>,
imported_flex: &'a mut Vec<Variable>,
}
let abilities_store = pending_abilities.resolve_for_module(
@ -4672,16 +4628,26 @@ pub fn add_imports(
&mut Ctx {
subs,
exposed_by_module: &exposed_for_module.exposed_by_module,
imported_variables: &mut import_variables,
imported_rigids: imported_rigid_vars,
imported_flex: imported_flex_vars,
},
|ctx, symbol| match cached_symbol_vars.get(&symbol).copied() {
Some(var) => var,
None => {
import_var_for_symbol!(
import_variable_for_symbol(
ctx.subs,
ctx.exposed_by_module,
constraints,
def_types,
ctx.imported_variables,
ctx.imported_rigids,
ctx.imported_flex,
&mut cached_symbol_vars,
&exposed_for_module.exposed_by_module,
symbol,
internal_error!("Import ability member {:?} not available", symbol)
OnSymbolNotFound::AbilityMemberMustBeAvailable,
);
*cached_symbol_vars.get(&symbol).unwrap()
}
},
@ -4699,7 +4665,15 @@ pub fn add_imports(
.storage_subs
.export_variable_to(ctx.subs, *var);
copied_import.variable
#[allow(clippy::let_and_return)]
let copied_import_var = extend_imports_data_with_copied_import(
copied_import,
ctx.imported_variables,
ctx.imported_rigids,
ctx.imported_flex,
);
copied_import_var
}
None => internal_error!("Imported module {:?} is not available", module),
},
@ -4708,6 +4682,95 @@ pub fn add_imports(
(import_variables, abilities_store)
}
enum OnSymbolNotFound {
AssertIsBuiltin,
AbilityMemberMustBeAvailable,
}
fn extend_imports_data_with_copied_import(
copied_import: CopiedImport,
imported_variables: &mut Vec<Variable>,
imported_rigids: &mut Vec<Variable>,
imported_flex: &mut Vec<Variable>,
) -> Variable {
// not a typo; rigids are turned into flex during type inference, but when imported we must
// consider them rigid variables
imported_rigids.extend(copied_import.rigid);
imported_flex.extend(copied_import.flex);
// Rigid vars bound to abilities are also treated like rigids.
imported_rigids.extend(copied_import.rigid_able);
imported_flex.extend(copied_import.flex_able);
imported_variables.extend(copied_import.registered);
copied_import.variable
}
fn import_variable_for_symbol(
subs: &mut Subs,
constraints: &mut Constraints,
def_types: &mut Vec<(Symbol, Loc<TypeOrVar>)>,
imported_variables: &mut Vec<Variable>,
imported_rigids: &mut Vec<Variable>,
imported_flex: &mut Vec<Variable>,
cached_symbol_vars: &mut VecMap<Symbol, Variable>,
exposed_by_module: &ExposedByModule,
symbol: Symbol,
on_symbol_not_found: OnSymbolNotFound,
) {
let module_id = symbol.module_id();
match exposed_by_module.get(&module_id) {
Some(ExposedModuleTypes {
exposed_types_storage_subs: exposed_types,
resolved_implementations: _,
}) => {
let variable = match exposed_types
.stored_vars_by_symbol
.iter()
.find(|(s, _)| **s == symbol)
{
None => {
use OnSymbolNotFound::*;
match on_symbol_not_found {
AssertIsBuiltin => {
// Today we define builtins in each module that uses them
// so even though they have a different module name from
// the surrounding module, they are not technically imported
debug_assert!(symbol.is_builtin());
return;
}
AbilityMemberMustBeAvailable => {
internal_error!("Import ability member {:?} not available", symbol);
}
}
}
Some((_, x)) => *x,
};
let copied_import = exposed_types
.storage_subs
.export_variable_to(subs, variable);
let copied_import_var = extend_imports_data_with_copied_import(
copied_import,
imported_variables,
imported_rigids,
imported_flex,
);
let copied_import_index = constraints.push_variable(copied_import_var);
def_types.push((symbol, Loc::at_zero(copied_import_index)));
cached_symbol_vars.insert(symbol, copied_import_var);
}
None => {
internal_error!("Imported module {:?} is not available", module_id)
}
}
}
#[allow(clippy::complexity)]
fn run_solve_solve(
exposed_for_module: ExposedForModule,
@ -4733,7 +4796,8 @@ fn run_solve_solve(
..
} = module;
let mut rigid_vars: Vec<Variable> = Vec::new();
let mut imported_rigid_vars: Vec<Variable> = Vec::new();
let mut imported_flex_vars: Vec<Variable> = Vec::new();
let mut def_types: Vec<(Symbol, Loc<TypeOrVar>)> = Vec::new();
let mut subs = Subs::new_from_varstore(var_store);
@ -4745,11 +4809,17 @@ fn run_solve_solve(
pending_abilities,
&exposed_for_module,
&mut def_types,
&mut rigid_vars,
&mut imported_rigid_vars,
&mut imported_flex_vars,
);
let actual_constraint =
constraints.let_import_constraint(rigid_vars, def_types, constraint, &import_variables);
let actual_constraint = constraints.let_import_constraint(
imported_rigid_vars,
imported_flex_vars,
def_types,
constraint,
&import_variables,
);
let mut solve_aliases = roc_solve::solve::Aliases::with_capacity(aliases.len());
for (name, (_, alias)) in aliases.iter() {
@ -4814,7 +4884,6 @@ fn run_solve_solve(
)
}
#[allow(clippy::too_many_arguments)]
fn run_solve<'a>(
module: Module,
ident_ids: IdentIds,
@ -4928,7 +4997,6 @@ fn unspace<'a, T: Copy>(arena: &'a Bump, items: &[Loc<Spaced<'a, T>>]) -> &'a [L
.into_bump_slice()
}
#[allow(clippy::too_many_arguments)]
fn fabricate_platform_module<'a>(
arena: &'a Bump,
opt_shorthand: Option<&'a str>,
@ -4968,7 +5036,6 @@ fn fabricate_platform_module<'a>(
)
}
#[allow(clippy::too_many_arguments)]
#[allow(clippy::unnecessary_wraps)]
fn canonicalize_and_constrain<'a>(
arena: &'a Bump,
@ -5242,7 +5309,6 @@ fn ident_from_exposed(entry: &Spaced<'_, ExposedName<'_>>) -> Ident {
entry.extract_spaces().item.as_str().into()
}
#[allow(clippy::too_many_arguments)]
fn make_specializations<'a>(
arena: &'a Bump,
home: ModuleId,
@ -5319,7 +5385,6 @@ fn make_specializations<'a>(
}
}
#[allow(clippy::too_many_arguments)]
fn build_pending_specializations<'a>(
arena: &'a Bump,
solved_subs: Solved<Subs>,
@ -5750,7 +5815,6 @@ fn build_pending_specializations<'a>(
/// their specializations.
// TODO: right now, this runs sequentially, and no other modules are mono'd in parallel to the
// derived module.
#[allow(clippy::too_many_arguments)]
fn load_derived_partial_procs<'a>(
home: ModuleId,
arena: &'a Bump,

View file

@ -8431,4 +8431,29 @@ mod solve_expr {
@"n : Dec"
);
}
#[test]
fn resolve_set_eq_issue_4671() {
infer_queries!(
indoc!(
r#"
app "test" provides [main] to "./platform"
main =
s1 : Set U8
s1 = Set.empty
s2 : Set Str
s2 = Set.empty
Bool.isEq s1 s1 && Bool.isEq s2 s2
# ^^^^^^^^^ ^^^^^^^^^
"#
),
@r###"
Set#Bool.isEq(17) : Set U8, Set U8 -[[Set.isEq(17)]]-> Bool
Set#Bool.isEq(17) : Set Str, Set Str -[[Set.isEq(17)]]-> Bool
"###
);
}
}

View file

@ -378,6 +378,7 @@ fn check_derived_typechecks_and_golden(
);
let mut def_types = Default::default();
let mut rigid_vars = Default::default();
let mut flex_vars = Default::default();
let (import_variables, abilities_store) = add_imports(
test_module,
&mut constraints,
@ -386,9 +387,15 @@ fn check_derived_typechecks_and_golden(
&exposed_for_module,
&mut def_types,
&mut rigid_vars,
&mut flex_vars,
);
let constr = constraints.let_import_constraint(
rigid_vars,
flex_vars,
def_types,
constr,
&import_variables,
);
let constr =
constraints.let_import_constraint(rigid_vars, def_types, constr, &import_variables);
// run the solver, print and fail if we have errors
dbg_do!(

View file

@ -1,4 +1,7 @@
#![cfg(any(feature = "gen-llvm", feature = "gen-wasm"))]
#![cfg(all(
any(feature = "gen-llvm", feature = "gen-wasm"),
not(debug_assertions) // https://github.com/roc-lang/roc/issues/3898
))]
#[cfg(feature = "gen-llvm")]
use crate::helpers::llvm::assert_evals_to;

View file

@ -1,4 +1,7 @@
#![cfg(feature = "gen-llvm")]
#![cfg(all(
any(feature = "gen-llvm"),
not(debug_assertions) // https://github.com/roc-lang/roc/issues/3898
))]
#[cfg(feature = "gen-llvm")]
use crate::helpers::llvm::assert_evals_to;
@ -284,3 +287,26 @@ fn from_list_result() {
i64
);
}
#[test]
#[cfg(any(feature = "gen-llvm"))]
fn resolve_set_eq_issue_4671() {
assert_evals_to!(
indoc!(
r#"
app "test" provides [main] to "./platform"
main =
s1 : Set U8
s1 = Set.fromList [1, 2, 3]
s2 : Set U8
s2 = Set.fromList [3, 2, 1]
s1 == s2
"#
),
true,
bool
);
}