From ae1d66984f48f5482535e4b29aefbc1591e622d0 Mon Sep 17 00:00:00 2001 From: Afsal Thaj Date: Wed, 2 Oct 2024 11:04:53 +1000 Subject: [PATCH 01/15] Clean up --- golem-rib/src/interpreter/mod.rs | 8 ++++ golem-rib/src/interpreter/rib_interpreter.rs | 4 +- .../src/api/custom_http_request_api.rs | 4 +- .../src/http/http_request.rs | 4 +- golem-worker-service-base/src/lib.rs | 1 + .../compiled_golem_worker_binding.rs | 9 ++-- .../src/worker_binding/mod.rs | 13 ----- .../worker_binding/worker_binding_resolver.rs | 18 +++---- .../worker_request_executor.rs | 14 ------ .../src/worker_service_rib_compiler/mod.rs | 28 +++++++++++ .../src/worker_service_rib_interpreter/mod.rs | 47 ++++--------------- 11 files changed, 65 insertions(+), 85 deletions(-) create mode 100644 golem-worker-service-base/src/worker_service_rib_compiler/mod.rs diff --git a/golem-rib/src/interpreter/mod.rs b/golem-rib/src/interpreter/mod.rs index d86dd8f7f..270f33c35 100644 --- a/golem-rib/src/interpreter/mod.rs +++ b/golem-rib/src/interpreter/mod.rs @@ -35,3 +35,11 @@ pub async fn interpret( let mut interpreter = Interpreter::new(rib_input, function_invoke); interpreter.run(rib.clone()).await } + +pub async fn interpret_pure( + rib: &RibByteCode, + rib_input: &HashMap, +) -> Result { + let mut interpreter = Interpreter::pure(rib_input.clone()); + interpreter.run(rib.clone()).await +} \ No newline at end of file diff --git a/golem-rib/src/interpreter/rib_interpreter.rs b/golem-rib/src/interpreter/rib_interpreter.rs index 8ee11279b..3588504e5 100644 --- a/golem-rib/src/interpreter/rib_interpreter.rs +++ b/golem-rib/src/interpreter/rib_interpreter.rs @@ -50,7 +50,9 @@ impl Interpreter { } } - pub fn from_input(env: HashMap) -> Self { + // Interpreter that's not expected to call a side-effecting function call. + // All it needs is environment with the required variables to evaluate the Rib script + pub fn pure(env: HashMap) -> Self { Interpreter { stack: InterpreterStack::new(), env: InterpreterEnv::from_input(env), diff --git a/golem-worker-service-base/src/api/custom_http_request_api.rs b/golem-worker-service-base/src/api/custom_http_request_api.rs index dc273e645..4cddc492f 100644 --- a/golem-worker-service-base/src/api/custom_http_request_api.rs +++ b/golem-worker-service-base/src/api/custom_http_request_api.rs @@ -2,7 +2,7 @@ use std::future::Future; use std::sync::Arc; use crate::api_definition::http::CompiledHttpApiDefinition; -use crate::worker_service_rib_interpreter::{DefaultEvaluator, WorkerServiceRibInterpreter}; +use crate::worker_service_rib_interpreter::{DefaultRibInterpreter, WorkerServiceRibInterpreter}; use futures_util::FutureExt; use hyper::header::HOST; use poem::http::StatusCode; @@ -31,7 +31,7 @@ impl CustomHttpRequestApi { dyn ApiDefinitionsLookup + Sync + Send, >, ) -> Self { - let evaluator = Arc::new(DefaultEvaluator::from_worker_request_executor( + let evaluator = Arc::new(DefaultRibInterpreter::from_worker_request_executor( worker_request_executor_service.clone(), )); diff --git a/golem-worker-service-base/src/http/http_request.rs b/golem-worker-service-base/src/http/http_request.rs index 2c046c2a5..d898c8a8b 100644 --- a/golem-worker-service-base/src/http/http_request.rs +++ b/golem-worker-service-base/src/http/http_request.rs @@ -133,7 +133,7 @@ mod tests { WorkerRequest, WorkerRequestExecutor, WorkerRequestExecutorError, WorkerResponse, }; use crate::worker_service_rib_interpreter::{ - DefaultEvaluator, EvaluationError, WorkerServiceRibInterpreter, + DefaultRibInterpreter, EvaluationError, WorkerServiceRibInterpreter, }; struct TestWorkerRequestExecutor {} @@ -249,7 +249,7 @@ mod tests { } fn get_test_evaluator() -> Arc { - Arc::new(DefaultEvaluator::from_worker_request_executor(Arc::new( + Arc::new(DefaultRibInterpreter::from_worker_request_executor(Arc::new( TestWorkerRequestExecutor {}, ))) } diff --git a/golem-worker-service-base/src/lib.rs b/golem-worker-service-base/src/lib.rs index 07fa51638..a04eb8a58 100644 --- a/golem-worker-service-base/src/lib.rs +++ b/golem-worker-service-base/src/lib.rs @@ -14,6 +14,7 @@ pub mod service; mod worker_binding; pub mod worker_bridge_execution; pub mod worker_service_rib_interpreter; +mod worker_service_rib_compiler; const VERSION: &str = golem_version!(); diff --git a/golem-worker-service-base/src/worker_binding/compiled_golem_worker_binding.rs b/golem-worker-service-base/src/worker_binding/compiled_golem_worker_binding.rs index 48f9d5eb5..80a6fcd83 100644 --- a/golem-worker-service-base/src/worker_binding/compiled_golem_worker_binding.rs +++ b/golem-worker-service-base/src/worker_binding/compiled_golem_worker_binding.rs @@ -1,8 +1,9 @@ -use crate::worker_binding::{compile_rib, GolemWorkerBinding, ResponseMapping}; +use crate::worker_binding::{GolemWorkerBinding, ResponseMapping}; use bincode::{Decode, Encode}; use golem_service_base::model::VersionedComponentId; use golem_wasm_ast::analysis::AnalysedExport; use rib::{Expr, RibByteCode, RibInputTypeInfo}; +use crate::worker_service_rib_compiler::{DefaultRibCompiler, WorkerServiceRibCompiler}; #[derive(Debug, Clone, PartialEq, Encode, Decode)] pub struct CompiledGolemWorkerBinding { @@ -54,7 +55,7 @@ impl WorkerNameCompiled { worker_name: &Expr, exports: &Vec, ) -> Result { - let worker_name_compiled = compile_rib(worker_name, exports)?; + let worker_name_compiled = DefaultRibCompiler::compile(worker_name, exports)?; Ok(WorkerNameCompiled { worker_name: worker_name.clone(), @@ -76,7 +77,7 @@ impl IdempotencyKeyCompiled { idempotency_key: &Expr, exports: &Vec, ) -> Result { - let idempotency_key_compiled = compile_rib(idempotency_key, exports)?; + let idempotency_key_compiled = DefaultRibCompiler::compile(idempotency_key, exports)?; Ok(IdempotencyKeyCompiled { idempotency_key: idempotency_key.clone(), @@ -98,7 +99,7 @@ impl ResponseMappingCompiled { response_mapping: &ResponseMapping, exports: &Vec, ) -> Result { - let response_compiled = compile_rib(&response_mapping.0, exports)?; + let response_compiled = DefaultRibCompiler::compile(&response_mapping.0, exports)?; Ok(ResponseMappingCompiled { response_rib_expr: response_mapping.0.clone(), diff --git a/golem-worker-service-base/src/worker_binding/mod.rs b/golem-worker-service-base/src/worker_binding/mod.rs index b283f7c1d..d9c88a9af 100644 --- a/golem-worker-service-base/src/worker_binding/mod.rs +++ b/golem-worker-service-base/src/worker_binding/mod.rs @@ -1,8 +1,6 @@ pub(crate) use compiled_golem_worker_binding::*; -use golem_wasm_ast::analysis::AnalysedExport; pub(crate) use golem_worker_binding::*; pub(crate) use request_details::*; -use rib::{CompilerOutput, Expr}; pub(crate) use rib_input_value_resolver::*; pub(crate) use worker_binding_resolver::*; @@ -11,14 +9,3 @@ mod golem_worker_binding; mod request_details; mod rib_input_value_resolver; mod worker_binding_resolver; - -pub fn compile_rib( - worker_name: &Expr, - export_metadata: &Vec, -) -> Result { - rib::compile_with_limited_globals( - worker_name, - export_metadata, - Some(vec!["request".to_string()]), - ) -} diff --git a/golem-worker-service-base/src/worker_binding/worker_binding_resolver.rs b/golem-worker-service-base/src/worker_binding/worker_binding_resolver.rs index 4d7e3babd..a1962bdf7 100644 --- a/golem-worker-service-base/src/worker_binding/worker_binding_resolver.rs +++ b/golem-worker-service-base/src/worker_binding/worker_binding_resolver.rs @@ -3,7 +3,7 @@ use crate::http::http_request::router; use crate::http::router::RouterPattern; use crate::http::InputHttpRequest; use crate::worker_service_rib_interpreter::WorkerServiceRibInterpreter; -use crate::worker_service_rib_interpreter::{DefaultEvaluator, EvaluationError}; +use crate::worker_service_rib_interpreter::{EvaluationError}; use async_trait::async_trait; use golem_common::model::IdempotencyKey; use golem_service_base::model::VersionedComponentId; @@ -129,8 +129,6 @@ impl RequestToWorkerBindingResolver for InputHttpRequ &self, api_definition: Vec, ) -> Result { - let default_evaluator = DefaultEvaluator::noop(); - let routes = api_definition .iter() .flat_map(|x| x.routes.clone()) @@ -172,11 +170,10 @@ impl RequestToWorkerBindingResolver for InputHttpRequ .map_err(|err| format!("Failed to resolve rib input value {}", err))?; // To evaluate worker-name, most probably - let worker_name: String = default_evaluator - .evaluate_pure( - &binding.worker_name_compiled.compiled_worker_name, - &resolve_rib_input, - ) + let worker_name: String = rib::interpret_pure( + &binding.worker_name_compiled.compiled_worker_name, + &resolve_rib_input.value + ) .await .map_err(|err| format!("Failed to evaluate worker name expression. {}", err))? .get_literal() @@ -187,10 +184,9 @@ impl RequestToWorkerBindingResolver for InputHttpRequ let idempotency_key = if let Some(idempotency_key_compiled) = &binding.idempotency_key_compiled { - let idempotency_key_value = default_evaluator - .evaluate_pure( + let idempotency_key_value = rib::interpret_pure( &idempotency_key_compiled.compiled_idempotency_key, - &resolve_rib_input, + &resolve_rib_input.value, ) .await .map_err(|err| err.to_string())?; diff --git a/golem-worker-service-base/src/worker_bridge_execution/worker_request_executor.rs b/golem-worker-service-base/src/worker_bridge_execution/worker_request_executor.rs index 52d5b243d..40488672e 100644 --- a/golem-worker-service-base/src/worker_bridge_execution/worker_request_executor.rs +++ b/golem-worker-service-base/src/worker_bridge_execution/worker_request_executor.rs @@ -38,17 +38,3 @@ impl> From for WorkerRequestExecutorError { WorkerRequestExecutorError(err.as_ref().to_string()) } } - -pub struct NoopWorkerRequestExecutor; - -#[async_trait] -impl WorkerRequestExecutor for NoopWorkerRequestExecutor { - async fn execute( - &self, - _worker_request_params: WorkerRequest, - ) -> Result { - Err(WorkerRequestExecutorError( - "NoopWorkerRequestExecutor".to_string(), - )) - } -} diff --git a/golem-worker-service-base/src/worker_service_rib_compiler/mod.rs b/golem-worker-service-base/src/worker_service_rib_compiler/mod.rs new file mode 100644 index 000000000..569419656 --- /dev/null +++ b/golem-worker-service-base/src/worker_service_rib_compiler/mod.rs @@ -0,0 +1,28 @@ +use golem_wasm_ast::analysis::AnalysedExport; +use rib::{CompilerOutput, Expr}; + +// A wrapper service over original Rib Compiler concerning +// the details of the worker bridge. +pub trait WorkerServiceRibCompiler { + + fn compile( + rib: &Expr, + export_metadata: &Vec, + ) -> Result; +} + + +pub struct DefaultRibCompiler; + +impl WorkerServiceRibCompiler for DefaultRibCompiler { + fn compile( + rib: &Expr, + export_metadata: &Vec, + ) -> Result { + rib::compile_with_limited_globals( + rib, + export_metadata, + Some(vec!["request".to_string()]), + ) + } +} \ No newline at end of file diff --git a/golem-worker-service-base/src/worker_service_rib_interpreter/mod.rs b/golem-worker-service-base/src/worker_service_rib_interpreter/mod.rs index f50c5ef3c..a3ed0a1ce 100644 --- a/golem-worker-service-base/src/worker_service_rib_interpreter/mod.rs +++ b/golem-worker-service-base/src/worker_service_rib_interpreter/mod.rs @@ -10,14 +10,15 @@ use golem_common::model::{ComponentId, IdempotencyKey}; use crate::worker_binding::RibInputValue; use rib::{RibByteCode, RibFunctionInvoke, RibInterpreterResult}; -use crate::worker_bridge_execution::{ - NoopWorkerRequestExecutor, WorkerRequest, WorkerRequestExecutor, -}; +use crate::worker_bridge_execution::{ WorkerRequest, WorkerRequestExecutor}; // A wrapper service over original RibInterpreter concerning // the details of the worker service. #[async_trait] pub trait WorkerServiceRibInterpreter { + + // Evaluate a Rib byte against a specific worker. + // RibByteCode may have actual function calls in a worker. async fn evaluate( &self, worker_name: &str, @@ -26,12 +27,6 @@ pub trait WorkerServiceRibInterpreter { rib_byte_code: &RibByteCode, rib_input: &RibInputValue, ) -> Result; - - async fn evaluate_pure( - &self, - expr: &RibByteCode, - rib_input: &RibInputValue, - ) -> Result; } #[derive(Debug, PartialEq)] @@ -49,28 +44,22 @@ impl From for EvaluationError { } } -pub struct DefaultEvaluator { +pub struct DefaultRibInterpreter { worker_request_executor: Arc, } -impl DefaultEvaluator { - pub fn noop() -> Self { - DefaultEvaluator { - worker_request_executor: Arc::new(NoopWorkerRequestExecutor), - } - } - +impl DefaultRibInterpreter { pub fn from_worker_request_executor( worker_request_executor: Arc, ) -> Self { - DefaultEvaluator { + DefaultRibInterpreter { worker_request_executor, } } } #[async_trait] -impl WorkerServiceRibInterpreter for DefaultEvaluator { +impl WorkerServiceRibInterpreter for DefaultRibInterpreter { async fn evaluate( &self, worker_name: &str, @@ -115,23 +104,5 @@ impl WorkerServiceRibInterpreter for DefaultEvaluator { .await .map_err(EvaluationError) } - - async fn evaluate_pure( - &self, - expr: &RibByteCode, - rib_input: &RibInputValue, - ) -> Result { - let worker_invoke_function: RibFunctionInvoke = Arc::new(|_, _| { - Box::pin( - async move { - Err("Worker invoke function is not allowed in pure evaluation".to_string()) - } - .boxed(), - ) - }); - - rib::interpret(expr, rib_input.value.clone(), worker_invoke_function) - .await - .map_err(EvaluationError) - } } + From c981d38179c569135c65fce5b23d2b41f856678a Mon Sep 17 00:00:00 2001 From: Afsal Thaj Date: Wed, 2 Oct 2024 12:37:07 +1000 Subject: [PATCH 02/15] Simplify type registry --- golem-rib/src/expr.rs | 8 +-- golem-rib/src/inferred_type.rs | 30 +++++++---- golem-rib/src/interpreter/mod.rs | 2 +- golem-rib/src/interpreter/rib_interpreter.rs | 2 + ...ference.rs => call_arguments_inference.rs} | 46 ++++++++++++---- .../src/type_inference/enum_resolution.rs | 8 +-- golem-rib/src/type_inference/mod.rs | 4 +- .../src/type_inference/variant_resolution.rs | 23 ++++---- golem-rib/src/type_registry.rs | 52 ++++++++++++++----- 9 files changed, 121 insertions(+), 54 deletions(-) rename golem-rib/src/type_inference/{function_type_inference.rs => call_arguments_inference.rs} (92%) diff --git a/golem-rib/src/expr.rs b/golem-rib/src/expr.rs index f33ed518e..cbdefb7ab 100644 --- a/golem-rib/src/expr.rs +++ b/golem-rib/src/expr.rs @@ -419,10 +419,10 @@ impl Expr { self.bind_types(); self.name_binding_pattern_match_variables(); self.name_binding_local_variables(); - self.infer_function_types(function_type_registry) - .map_err(|x| vec![x])?; self.infer_variants(function_type_registry); self.infer_enums(function_type_registry); + self.infer_call_arguments_type(function_type_registry) + .map_err(|x| vec![x])?; type_inference::type_inference_fix_point(Self::inference_scan, self) .map_err(|x| vec![x])?; self.unify_types()?; @@ -452,11 +452,11 @@ impl Expr { } // At this point we simply update the types to the parameter type expressions and the call expression itself. - pub fn infer_function_types( + pub fn infer_call_arguments_type( &mut self, function_type_registry: &FunctionTypeRegistry, ) -> Result<(), String> { - type_inference::infer_function_types(self, function_type_registry) + type_inference::infer_call_arguments_type(self, function_type_registry) } pub fn push_types_down(&mut self) -> Result<(), String> { diff --git a/golem-rib/src/inferred_type.rs b/golem-rib/src/inferred_type.rs index 6577def69..bee7f8269 100644 --- a/golem-rib/src/inferred_type.rs +++ b/golem-rib/src/inferred_type.rs @@ -1117,6 +1117,25 @@ impl InferredType { } } } + + pub fn from_variant_cases(type_variant: &TypeVariant) -> InferredType { + let cases = type_variant + .cases + .iter() + .map(|name_type_pair| { + ( + name_type_pair.name.clone(), + name_type_pair.typ.clone().map(|t| t.into()), + ) + }) + .collect(); + + InferredType::Variant(cases) + } + + pub fn from_enum_cases(type_enum: &TypeEnum) -> InferredType { + InferredType::Enum(type_enum.cases.clone()) + } } impl From for InferredType { @@ -1146,7 +1165,7 @@ impl From for InferredType { .collect(), ), AnalysedType::Flags(vs) => InferredType::Flags(vs.names), - AnalysedType::Enum(vs) => InferredType::Enum(vs.cases), + AnalysedType::Enum(vs) => InferredType::from_enum_cases(&vs), AnalysedType::Option(t) => InferredType::Option(Box::new((*t.inner).into())), AnalysedType::Result(golem_wasm_ast::analysis::TypeResult { ok, err, .. }) => { InferredType::Result { @@ -1154,14 +1173,7 @@ impl From for InferredType { error: err.map(|t| Box::new((*t).into())), } } - AnalysedType::Variant(vs) => InferredType::Variant( - vs.cases - .into_iter() - .map(|name_type_pair| { - (name_type_pair.name, name_type_pair.typ.map(|t| t.into())) - }) - .collect(), - ), + AnalysedType::Variant(vs) => InferredType::from_variant_cases(&vs), AnalysedType::Handle(golem_wasm_ast::analysis::TypeHandle { resource_id, mode }) => { InferredType::Resource { resource_id: resource_id.0, diff --git a/golem-rib/src/interpreter/mod.rs b/golem-rib/src/interpreter/mod.rs index 270f33c35..cf78699c1 100644 --- a/golem-rib/src/interpreter/mod.rs +++ b/golem-rib/src/interpreter/mod.rs @@ -42,4 +42,4 @@ pub async fn interpret_pure( ) -> Result { let mut interpreter = Interpreter::pure(rib_input.clone()); interpreter.run(rib.clone()).await -} \ No newline at end of file +} diff --git a/golem-rib/src/interpreter/rib_interpreter.rs b/golem-rib/src/interpreter/rib_interpreter.rs index 3588504e5..9aac657ff 100644 --- a/golem-rib/src/interpreter/rib_interpreter.rs +++ b/golem-rib/src/interpreter/rib_interpreter.rs @@ -535,6 +535,8 @@ mod internal { analysed_type: AnalysedType, interpreter: &mut Interpreter, ) -> Result<(), String> { + dbg!(variant_name.clone()); + dbg!(analysed_type.clone()); match analysed_type { AnalysedType::Variant(variants) => { let variant = variants diff --git a/golem-rib/src/type_inference/function_type_inference.rs b/golem-rib/src/type_inference/call_arguments_inference.rs similarity index 92% rename from golem-rib/src/type_inference/function_type_inference.rs rename to golem-rib/src/type_inference/call_arguments_inference.rs index 484d195ec..42f2154cd 100644 --- a/golem-rib/src/type_inference/function_type_inference.rs +++ b/golem-rib/src/type_inference/call_arguments_inference.rs @@ -16,7 +16,7 @@ use crate::type_registry::FunctionTypeRegistry; use crate::Expr; use std::collections::VecDeque; -pub fn infer_function_types( +pub fn infer_call_arguments_type( expr: &mut Expr, function_type_registry: &FunctionTypeRegistry, ) -> Result<(), String> { @@ -25,7 +25,7 @@ pub fn infer_function_types( while let Some(expr) = queue.pop_back() { match expr { Expr::Call(parsed_fn_name, args, inferred_type) => { - internal::resolve_call_expressions( + internal::resolve_call_argument_types( parsed_fn_name, function_type_registry, args, @@ -47,7 +47,7 @@ mod internal { use golem_wasm_ast::analysis::AnalysedType; use std::fmt::Display; - pub(crate) fn resolve_call_expressions( + pub(crate) fn resolve_call_argument_types( call_type: &mut CallType, function_type_registry: &FunctionTypeRegistry, args: &mut [Expr], @@ -112,7 +112,24 @@ mod internal { } } - _ => Ok(()), + CallType::EnumConstructor(_) => { + if args.is_empty() { + Ok(()) + } else { + Err("Enum constructor does not take any arguments".to_string()) + } + } + + CallType::VariantConstructor(variant_name) => { + let registry_key = RegistryKey::FunctionName(variant_name.clone()); + infer_types( + &FunctionNameInternal::VariantName(variant_name.clone()), + function_type_registry, + registry_key, + args, + inferred_type, + ) + } } } @@ -125,7 +142,7 @@ mod internal { ) -> Result<(), String> { if let Some(value) = function_type_registry.types.get(&key) { match value { - RegistryValue::Value(_) => {} + RegistryValue::Value(_) => Ok(()), RegistryValue::Function { parameter_types, return_types, @@ -153,26 +170,29 @@ mod internal { return_types.iter().map(|t| t.clone().into()).collect(), ) } - } + }; + + Ok(()) } else { - return Err(format!( + Err(format!( "Function {} expects {} arguments, but {} were provided", function_name, parameter_types.len(), args.len() - )); + )) } } } + } else { + Err(format!("Unknown function/variant call {}", function_name)) } - - Ok(()) } enum FunctionNameInternal { ResourceConstructorName(String), ResourceMethodName(String), Fqn(ParsedFunctionName), + VariantName(String), } impl Display for FunctionNameInternal { @@ -187,6 +207,9 @@ mod internal { FunctionNameInternal::Fqn(name) => { write!(f, "{}", name) } + FunctionNameInternal::VariantName(name) => { + write!(f, "{}", name) + } } } } @@ -408,7 +431,8 @@ mod function_parameters_inference_tests { let function_type_registry = get_function_type_registry(); let mut expr = Expr::from_text(rib_expr).unwrap(); - expr.infer_function_types(&function_type_registry).unwrap(); + expr.infer_call_arguments_type(&function_type_registry) + .unwrap(); let let_binding = Expr::let_binding("x", Expr::number(1f64)); diff --git a/golem-rib/src/type_inference/enum_resolution.rs b/golem-rib/src/type_inference/enum_resolution.rs index 11226529f..58937995e 100644 --- a/golem-rib/src/type_inference/enum_resolution.rs +++ b/golem-rib/src/type_inference/enum_resolution.rs @@ -23,6 +23,7 @@ pub fn infer_enums(expr: &mut Expr, function_type_registry: &FunctionTypeRegistr mod internal { use crate::call_type::CallType; use crate::{Expr, FunctionTypeRegistry, RegistryKey, RegistryValue}; + use golem_wasm_ast::analysis::AnalysedType; use std::collections::VecDeque; pub(crate) fn convert_identifiers_to_enum_function_calls( @@ -62,12 +63,13 @@ mod internal { match expr { Expr::Identifier(variable_id, inferred_type) => { // Retrieve the possible no-arg variant from the registry - let key = RegistryKey::EnumName(variable_id.name().clone()); - if let Some(RegistryValue::Value(analysed_type)) = + let key = RegistryKey::FunctionName(variable_id.name().clone()); + if let Some(RegistryValue::Value(AnalysedType::Enum(typed_enum))) = function_type_registry.types.get(&key) { enum_cases.push(variable_id.name()); - *inferred_type = inferred_type.merge(analysed_type.clone().into()); + *inferred_type = inferred_type + .merge(AnalysedType::Enum(typed_enum.clone()).clone().into()); } } diff --git a/golem-rib/src/type_inference/mod.rs b/golem-rib/src/type_inference/mod.rs index 9f2b96182..d0d2aa7a2 100644 --- a/golem-rib/src/type_inference/mod.rs +++ b/golem-rib/src/type_inference/mod.rs @@ -12,9 +12,9 @@ // See the License for the specific language governing permissions and // limitations under the License. +pub use call_arguments_inference::*; pub use enum_resolution::*; pub use expr_visitor::*; -pub use function_type_inference::*; pub use global_input_inference::*; pub use identifier_inference::*; pub use inference_fix_point::*; @@ -29,8 +29,8 @@ pub use type_reset::*; pub use type_unification::*; pub use variant_resolution::*; +mod call_arguments_inference; mod expr_visitor; -mod function_type_inference; mod identifier_inference; mod name_binding; mod pattern_match_binding; diff --git a/golem-rib/src/type_inference/variant_resolution.rs b/golem-rib/src/type_inference/variant_resolution.rs index 298b214d3..dc109c337 100644 --- a/golem-rib/src/type_inference/variant_resolution.rs +++ b/golem-rib/src/type_inference/variant_resolution.rs @@ -24,7 +24,8 @@ pub fn infer_variants(expr: &mut Expr, function_type_registry: &FunctionTypeRegi mod internal { use crate::call_type::CallType; - use crate::{Expr, FunctionTypeRegistry, RegistryKey, RegistryValue}; + use crate::{Expr, FunctionTypeRegistry, InferredType, RegistryKey, RegistryValue}; + use golem_wasm_ast::analysis::AnalysedType; use std::collections::VecDeque; pub(crate) fn convert_function_calls_to_variant_calls( @@ -88,26 +89,26 @@ mod internal { while let Some(expr) = queue.pop_back() { match expr { Expr::Identifier(variable_id, inferred_type) => { - let key = RegistryKey::VariantName(variable_id.name().clone()); - if let Some(RegistryValue::Value(analysed_type)) = + let key = RegistryKey::FunctionName(variable_id.name().clone()); + if let Some(RegistryValue::Value(AnalysedType::Variant(type_variant))) = function_type_registry.types.get(&key) { no_arg_variants.push(variable_id.name()); - *inferred_type = inferred_type.merge(analysed_type.clone().into()); + *inferred_type = + inferred_type.merge(InferredType::from_variant_cases(type_variant)); } } Expr::Call(CallType::Function(parsed_function_name), exprs, inferred_type) => { - let key = RegistryKey::VariantName(parsed_function_name.to_string()); - if let Some(RegistryValue::Function { return_types, .. }) = + let key = RegistryKey::FunctionName(parsed_function_name.to_string()); + if let Some(RegistryValue::Value(AnalysedType::Variant(type_variant))) = function_type_registry.types.get(&key) { - variant_with_args.push(parsed_function_name.to_string()); + let variant_inferred_type = + InferredType::from_variant_cases(type_variant); + *inferred_type = inferred_type.merge(variant_inferred_type); - // TODO; return type is only 1 in reality for variants - we can make this typed - if let Some(variant_type) = return_types.first() { - *inferred_type = inferred_type.merge(variant_type.clone().into()); - } + variant_with_args.push(parsed_function_name.to_string()); } for expr in exprs { diff --git a/golem-rib/src/type_registry.rs b/golem-rib/src/type_registry.rs index dad0f6a04..55d7ecf9c 100644 --- a/golem-rib/src/type_registry.rs +++ b/golem-rib/src/type_registry.rs @@ -29,8 +29,6 @@ use std::collections::{HashMap, HashSet}; // then the RegistryValue is simply an AnalysedType representing the variant type itself. #[derive(Hash, Eq, PartialEq, Clone, Debug)] pub enum RegistryKey { - VariantName(String), - EnumName(String), FunctionName(String), FunctionNameWithInterface { interface_name: String, @@ -51,9 +49,9 @@ impl RegistryKey { pub fn from_invocation_name(invocation_name: &CallType) -> RegistryKey { match invocation_name { CallType::VariantConstructor(variant_name) => { - RegistryKey::VariantName(variant_name.clone()) + RegistryKey::FunctionName(variant_name.clone()) } - CallType::EnumConstructor(enum_name) => RegistryKey::EnumName(enum_name.clone()), + CallType::EnumConstructor(enum_name) => RegistryKey::FunctionName(enum_name.clone()), CallType::Function(function_name) => match function_name.site.interface_name() { None => RegistryKey::FunctionName(function_name.function_name()), Some(interface_name) => RegistryKey::FunctionNameWithInterface { @@ -117,16 +115,16 @@ impl FunctionTypeRegistry { }) .collect::>(); - let registry_value = RegistryValue::Function { - parameter_types, - return_types, - }; - let registry_key = RegistryKey::FunctionNameWithInterface { interface_name: interface_name.clone(), function_name: function_name.clone(), }; + let registry_value = RegistryValue::Function { + parameter_types, + return_types, + }; + map.insert(registry_key, registry_value); } } @@ -179,7 +177,7 @@ impl FunctionTypeRegistry { mod internal { use crate::{RegistryKey, RegistryValue}; - use golem_wasm_ast::analysis::AnalysedType; + use golem_wasm_ast::analysis::{AnalysedType, TypeResult}; use std::collections::HashMap; pub(crate) fn update_registry( @@ -189,7 +187,7 @@ mod internal { match ty.clone() { AnalysedType::Variant(variant) => { for name_type_pair in variant.cases { - registry.insert(RegistryKey::VariantName(name_type_pair.name.clone()), { + registry.insert(RegistryKey::FunctionName(name_type_pair.name.clone()), { name_type_pair.typ.map_or( RegistryValue::Value(ty.clone()), |variant_parameter_typ| RegistryValue::Function { @@ -204,7 +202,7 @@ mod internal { AnalysedType::Enum(type_enum) => { for name_type_pair in type_enum.cases { registry.insert( - RegistryKey::EnumName(name_type_pair.clone()), + RegistryKey::FunctionName(name_type_pair.clone()), RegistryValue::Value(ty.clone()), ); } @@ -226,7 +224,35 @@ mod internal { } } - _ => {} + AnalysedType::Result(TypeResult {ok: Some(ok_type), err: Some(err_type)}) => { + update_registry(ok_type.as_ref(), registry); + update_registry(err_type.as_ref(), registry); + } + AnalysedType::Result(TypeResult {ok: None, err: Some(err_type)}) => { + update_registry(err_type.as_ref(), registry); + } + AnalysedType::Result(TypeResult {ok: Some(ok_type), err: None}) => { + update_registry(ok_type.as_ref(), registry); + } + AnalysedType::Option(type_option) => { + update_registry(type_option.inner.as_ref(), registry); + } + AnalysedType::Result(_) => {} + AnalysedType::Flags(_) => {} + AnalysedType::Str(_) => {} + AnalysedType::Chr(_) => {} + AnalysedType::F64(_) => {} + AnalysedType::F32(_) => {} + AnalysedType::U64(_) => {} + AnalysedType::S64(_) => {} + AnalysedType::U32(_) => {} + AnalysedType::S32(_) => {} + AnalysedType::U16(_) => {} + AnalysedType::S16(_) => {} + AnalysedType::U8(_) => {} + AnalysedType::S8(_) => {} + AnalysedType::Bool(_) => {} + AnalysedType::Handle(_) => {} } } } From add21366bc7a5a96ee181ed35ab73a9a03b2edb9 Mon Sep 17 00:00:00 2001 From: Afsal Thaj Date: Wed, 2 Oct 2024 12:57:47 +1000 Subject: [PATCH 03/15] Improve error message --- golem-rib/src/interpreter/result.rs | 7 ++-- golem-rib/src/interpreter/rib_interpreter.rs | 34 ++++++++++---------- 2 files changed, 21 insertions(+), 20 deletions(-) diff --git a/golem-rib/src/interpreter/result.rs b/golem-rib/src/interpreter/result.rs index c100ae567..934dbbc5b 100644 --- a/golem-rib/src/interpreter/result.rs +++ b/golem-rib/src/interpreter/result.rs @@ -47,20 +47,21 @@ impl RibInterpreterResult { pub fn get_bool(&self) -> Option { match self { RibInterpreterResult::Val(TypeAnnotatedValue::Bool(bool)) => Some(*bool), - _ => None, + RibInterpreterResult::Val(_) => None, + RibInterpreterResult::Unit => None, } } pub fn get_val(&self) -> Option { match self { RibInterpreterResult::Val(val) => Some(val.clone()), - _ => None, + RibInterpreterResult::Unit => None, } } pub fn get_literal(&self) -> Option { match self { RibInterpreterResult::Val(val) => val.get_literal(), - _ => None, + RibInterpreterResult::Unit => None, } } diff --git a/golem-rib/src/interpreter/rib_interpreter.rs b/golem-rib/src/interpreter/rib_interpreter.rs index 9aac657ff..399c6e73c 100644 --- a/golem-rib/src/interpreter/rib_interpreter.rs +++ b/golem-rib/src/interpreter/rib_interpreter.rs @@ -337,7 +337,7 @@ mod internal { .map(|interpreter_result| { interpreter_result .get_val() - .ok_or("Failed to get value from the stack".to_string()) + .ok_or("Internal Error: Failed to construct list".to_string()) }) .collect::, String>>()?; @@ -370,7 +370,7 @@ mod internal { .map(|interpreter_result| { interpreter_result .get_val() - .ok_or("Failed to get value from the stack".to_string()) + .ok_or("Internal Error: Failed to construct tuple".to_string()) }) .collect::, String>>()?; @@ -638,7 +638,7 @@ mod internal { .map(|interpreter_result| { interpreter_result .get_val() - .ok_or("Failed to get value from the stack".to_string()) + .ok_or("Internal Error: Failed to construct resource".to_string()) }) .collect::, String>>()?; @@ -672,7 +672,7 @@ mod internal { .map(|interpreter_result| { interpreter_result .get_val() - .ok_or("Failed to get value from the stack".to_string()) + .ok_or("Internal Error: Failed to call indexed resource method".to_string()) }) .collect::, String>>()?; @@ -700,14 +700,14 @@ mod internal { let last_n_elements = interpreter .stack .pop_n(arg_size) - .ok_or("Failed to get values from the stack".to_string())?; + .ok_or("Internal error: Failed to get arguments for static resource method".to_string())?; let type_anntoated_values = last_n_elements .iter() .map(|interpreter_result| { interpreter_result .get_val() - .ok_or("Failed to get value from the stack".to_string()) + .ok_or("Internal error: Failed to call static resource method".to_string()) }) .collect::, String>>()?; @@ -731,14 +731,14 @@ mod internal { let last_n_elements = interpreter .stack .pop_n(arg_size) - .ok_or("Failed to get values from the stack".to_string())?; + .ok_or("Internal Error: Failed to get resource parameters for indexed resource drop".to_string())?; - let type_anntoated_values = last_n_elements + let type_annotated_values = last_n_elements .iter() .map(|interpreter_result| { interpreter_result .get_val() - .ok_or("Failed to get value from the stack".to_string()) + .ok_or("Internal Error: Failed to call indexed resource drop".to_string()) }) .collect::, String>>()?; @@ -746,7 +746,7 @@ mod internal { site, function: ParsedFunctionReference::IndexedResourceDrop { resource, - resource_params: type_anntoated_values + resource_params: type_annotated_values .iter() .map(type_annotated_value_to_string) .collect::, String>>()?, @@ -770,19 +770,19 @@ mod internal { let function_name = interpreter .stack .pop_str() - .ok_or("Failed to get a function name from the stack".to_string())?; + .ok_or("Internal Error: Failed to get a function name".to_string())?; let last_n_elements = interpreter .stack .pop_n(argument_size) - .ok_or("Failed to get values from the stack".to_string())?; + .ok_or("Internal Error: Failed to get arguments for the function call".to_string())?; let type_anntoated_values = last_n_elements .iter() .map(|interpreter_result| { interpreter_result .get_val() - .ok_or("Failed to get value from the stack".to_string()) + .ok_or(format!("Internal Error: Failed to call function {}", function_name)) }) .collect::, String>>()?; @@ -925,19 +925,19 @@ mod internal { ) -> Result<(), String> { let last_n_elements = interpreter_stack .pop_n(arg_size) - .ok_or("Failed to get values from the stack".to_string())?; + .ok_or("Internal Error: Failed to get arguments for concatenation".to_string())?; - let type_anntoated_values = last_n_elements + let type_annotated_values = last_n_elements .iter() .map(|interpreter_result| { interpreter_result .get_val() - .ok_or("Failed to get value from the stack".to_string()) + .ok_or("Internal Error: Failed to execute concatenation".to_string()) }) .collect::, String>>()?; let mut str = String::new(); - for value in type_anntoated_values { + for value in type_annotated_values { let result = value .get_literal() .ok_or("Expected a literal value".to_string())? From 126e9006266aace9e3fc92551f0c51f34f1c488e Mon Sep 17 00:00:00 2001 From: Afsal Thaj Date: Wed, 2 Oct 2024 15:20:59 +1000 Subject: [PATCH 04/15] Improve error message --- golem-rib/src/compiler/byte_code.rs | 4 +- golem-rib/src/interpreter/rib_interpreter.rs | 53 +++++++++---------- .../call_arguments_inference.rs | 38 +++++++++++-- .../src/type_inference/variant_resolution.rs | 5 +- golem-rib/src/type_registry.rs | 30 ++++++++--- 5 files changed, 85 insertions(+), 45 deletions(-) diff --git a/golem-rib/src/compiler/byte_code.rs b/golem-rib/src/compiler/byte_code.rs index cebf097e8..94b66cb11 100644 --- a/golem-rib/src/compiler/byte_code.rs +++ b/golem-rib/src/compiler/byte_code.rs @@ -376,9 +376,9 @@ mod internal { convert_to_analysed_type_for(expr, inferred_type)?, )); } - CallType::EnumConstructor(enmum_name) => { + CallType::EnumConstructor(enum_name) => { instructions.push(RibIR::PushEnum( - enmum_name.clone(), + enum_name.clone(), convert_to_analysed_type_for(expr, inferred_type)?, )); } diff --git a/golem-rib/src/interpreter/rib_interpreter.rs b/golem-rib/src/interpreter/rib_interpreter.rs index 399c6e73c..087984dcc 100644 --- a/golem-rib/src/interpreter/rib_interpreter.rs +++ b/golem-rib/src/interpreter/rib_interpreter.rs @@ -142,8 +142,8 @@ impl Interpreter { internal::run_create_function_name_instruction(site, function_type, self)?; } - RibIR::InvokeFunction(arity, _) => { - internal::run_call_instruction(arity, self).await?; + RibIR::InvokeFunction(arg_size, _) => { + internal::run_call_instruction(arg_size, self).await?; } RibIR::PushVariant(variant_name, analysed_type) => { @@ -365,6 +365,7 @@ mod internal { .pop_n(list_size) .ok_or(format!("Expected {} value on the stack", list_size))?; + dbg!(last_list.clone()); let type_annotated_values = last_list .iter() .map(|interpreter_result| { @@ -535,8 +536,6 @@ mod internal { analysed_type: AnalysedType, interpreter: &mut Interpreter, ) -> Result<(), String> { - dbg!(variant_name.clone()); - dbg!(analysed_type.clone()); match analysed_type { AnalysedType::Variant(variants) => { let variant = variants @@ -670,9 +669,9 @@ mod internal { let type_anntoated_values = last_n_elements .iter() .map(|interpreter_result| { - interpreter_result - .get_val() - .ok_or("Internal Error: Failed to call indexed resource method".to_string()) + interpreter_result.get_val().ok_or( + "Internal Error: Failed to call indexed resource method".to_string(), + ) }) .collect::, String>>()?; @@ -697,17 +696,17 @@ mod internal { arg_size, method, } => { - let last_n_elements = interpreter - .stack - .pop_n(arg_size) - .ok_or("Internal error: Failed to get arguments for static resource method".to_string())?; + let last_n_elements = interpreter.stack.pop_n(arg_size).ok_or( + "Internal error: Failed to get arguments for static resource method" + .to_string(), + )?; let type_anntoated_values = last_n_elements .iter() .map(|interpreter_result| { - interpreter_result - .get_val() - .ok_or("Internal error: Failed to call static resource method".to_string()) + interpreter_result.get_val().ok_or( + "Internal error: Failed to call static resource method".to_string(), + ) }) .collect::, String>>()?; @@ -728,17 +727,17 @@ mod internal { .push_val(TypeAnnotatedValue::Str(parsed_function_name.to_string())); } FunctionReferenceType::IndexedResourceDrop { resource, arg_size } => { - let last_n_elements = interpreter - .stack - .pop_n(arg_size) - .ok_or("Internal Error: Failed to get resource parameters for indexed resource drop".to_string())?; + let last_n_elements = interpreter.stack.pop_n(arg_size).ok_or( + "Internal Error: Failed to get resource parameters for indexed resource drop" + .to_string(), + )?; let type_annotated_values = last_n_elements .iter() .map(|interpreter_result| { - interpreter_result - .get_val() - .ok_or("Internal Error: Failed to call indexed resource drop".to_string()) + interpreter_result.get_val().ok_or( + "Internal Error: Failed to call indexed resource drop".to_string(), + ) }) .collect::, String>>()?; @@ -762,9 +761,8 @@ mod internal { Ok(()) } - // Separate variant pub(crate) async fn run_call_instruction( - argument_size: usize, + arg_size: usize, interpreter: &mut Interpreter, ) -> Result<(), String> { let function_name = interpreter @@ -774,15 +772,16 @@ mod internal { let last_n_elements = interpreter .stack - .pop_n(argument_size) + .pop_n(arg_size) .ok_or("Internal Error: Failed to get arguments for the function call".to_string())?; let type_anntoated_values = last_n_elements .iter() .map(|interpreter_result| { - interpreter_result - .get_val() - .ok_or(format!("Internal Error: Failed to call function {}", function_name)) + interpreter_result.get_val().ok_or(format!( + "Internal Error: Failed to call function {}", + function_name + )) }) .collect::, String>>()?; diff --git a/golem-rib/src/type_inference/call_arguments_inference.rs b/golem-rib/src/type_inference/call_arguments_inference.rs index 42f2154cd..d35b8fde1 100644 --- a/golem-rib/src/type_inference/call_arguments_inference.rs +++ b/golem-rib/src/type_inference/call_arguments_inference.rs @@ -143,6 +143,26 @@ mod internal { if let Some(value) = function_type_registry.types.get(&key) { match value { RegistryValue::Value(_) => Ok(()), + RegistryValue::Variant { + parameter_types, + variant_type, + } => { + let parameter_types = parameter_types.clone(); + + if parameter_types.len() == args.len() { + tag_argument_types(args, ¶meter_types)?; + *inferred_type = InferredType::from_variant_cases(variant_type); + + Ok(()) + } else { + Err(format!( + "Variant {} expects {} arguments, but {} were provided", + function_name, + parameter_types.len(), + args.len() + )) + } + } RegistryValue::Function { parameter_types, return_types, @@ -156,11 +176,7 @@ mod internal { } if parameter_types.len() == args.len() { - for (arg, param_type) in args.iter_mut().zip(parameter_types) { - check_function_arguments(¶m_type, arg)?; - arg.add_infer_type_mut(param_type.clone().into()); - arg.push_types_down()? - } + tag_argument_types(args, ¶meter_types)?; *inferred_type = { if return_types.len() == 1 { @@ -387,6 +403,18 @@ mod internal { } } } + + fn tag_argument_types( + args: &mut [Expr], + parameter_types: &[AnalysedType], + ) -> Result<(), String> { + for (arg, param_type) in args.iter_mut().zip(parameter_types) { + check_function_arguments(param_type, arg)?; + arg.add_infer_type_mut(param_type.clone().into()); + } + + Ok(()) + } } #[cfg(test)] diff --git a/golem-rib/src/type_inference/variant_resolution.rs b/golem-rib/src/type_inference/variant_resolution.rs index dc109c337..28209d102 100644 --- a/golem-rib/src/type_inference/variant_resolution.rs +++ b/golem-rib/src/type_inference/variant_resolution.rs @@ -101,11 +101,10 @@ mod internal { Expr::Call(CallType::Function(parsed_function_name), exprs, inferred_type) => { let key = RegistryKey::FunctionName(parsed_function_name.to_string()); - if let Some(RegistryValue::Value(AnalysedType::Variant(type_variant))) = + if let Some(RegistryValue::Variant { variant_type, .. }) = function_type_registry.types.get(&key) { - let variant_inferred_type = - InferredType::from_variant_cases(type_variant); + let variant_inferred_type = InferredType::from_variant_cases(variant_type); *inferred_type = inferred_type.merge(variant_inferred_type); variant_with_args.push(parsed_function_name.to_string()); diff --git a/golem-rib/src/type_registry.rs b/golem-rib/src/type_registry.rs index 55d7ecf9c..aba79726e 100644 --- a/golem-rib/src/type_registry.rs +++ b/golem-rib/src/type_registry.rs @@ -14,8 +14,8 @@ use crate::call_type::CallType; use crate::ParsedFunctionSite; -use golem_wasm_ast::analysis::AnalysedExport; use golem_wasm_ast::analysis::AnalysedType; +use golem_wasm_ast::analysis::{AnalysedExport, TypeVariant}; use std::collections::{HashMap, HashSet}; // A type-registry is a mapping from a function name (global or part of an interface in WIT) @@ -66,6 +66,10 @@ impl RegistryKey { #[derive(PartialEq, Clone, Debug)] pub enum RegistryValue { Value(AnalysedType), + Variant { + parameter_types: Vec, + variant_type: TypeVariant, + }, Function { parameter_types: Vec, return_types: Vec, @@ -186,13 +190,14 @@ mod internal { ) { match ty.clone() { AnalysedType::Variant(variant) => { - for name_type_pair in variant.cases { + let type_variant = variant.clone(); + for name_type_pair in &type_variant.cases { registry.insert(RegistryKey::FunctionName(name_type_pair.name.clone()), { - name_type_pair.typ.map_or( + name_type_pair.typ.clone().map_or( RegistryValue::Value(ty.clone()), - |variant_parameter_typ| RegistryValue::Function { + |variant_parameter_typ| RegistryValue::Variant { parameter_types: vec![variant_parameter_typ], - return_types: vec![ty.clone()], + variant_type: type_variant.clone(), }, ) }); @@ -224,14 +229,23 @@ mod internal { } } - AnalysedType::Result(TypeResult {ok: Some(ok_type), err: Some(err_type)}) => { + AnalysedType::Result(TypeResult { + ok: Some(ok_type), + err: Some(err_type), + }) => { update_registry(ok_type.as_ref(), registry); update_registry(err_type.as_ref(), registry); } - AnalysedType::Result(TypeResult {ok: None, err: Some(err_type)}) => { + AnalysedType::Result(TypeResult { + ok: None, + err: Some(err_type), + }) => { update_registry(err_type.as_ref(), registry); } - AnalysedType::Result(TypeResult {ok: Some(ok_type), err: None}) => { + AnalysedType::Result(TypeResult { + ok: Some(ok_type), + err: None, + }) => { update_registry(ok_type.as_ref(), registry); } AnalysedType::Option(type_option) => { From 4fe3c6c1ea48fba538e309f4eb185c8d0a5e34d7 Mon Sep 17 00:00:00 2001 From: Afsal Thaj Date: Wed, 2 Oct 2024 16:43:32 +1000 Subject: [PATCH 05/15] Improve error message --- golem-rib/src/compiler/byte_code.rs | 209 +++++++---- golem-rib/src/expr.rs | 44 +++ golem-rib/src/function_name.rs | 30 ++ .../call_arguments_inference.rs | 327 ++++++++---------- 4 files changed, 364 insertions(+), 246 deletions(-) diff --git a/golem-rib/src/compiler/byte_code.rs b/golem-rib/src/compiler/byte_code.rs index 94b66cb11..5282dfddc 100644 --- a/golem-rib/src/compiler/byte_code.rs +++ b/golem-rib/src/compiler/byte_code.rs @@ -984,6 +984,47 @@ mod compiler_tests { assert_eq!(instructions, expected_instructions); } + #[cfg(test)] + mod invalid_function_invoke_tests { + use crate::{compiler, Expr}; + use crate::compiler::byte_code::compiler_tests::internal; + + #[test] + fn test_invalid_function_call() { + let expr = r#" + foo(request); + "success" + "#; + + let expr = Expr::from_text(expr).unwrap(); + let compiler_error = compiler::compile(&expr, &vec![]).unwrap_err(); + + assert_eq!( + compiler_error, + "Unknown function call foo" + ); + } + + #[test] + fn test_invalid_resource_method_call() { + let metadata = internal::metadata_with_resource_methods(); + let expr = r#" + let user_id = "user"; + golem:it/api.{cart(user_id).add-item}("apple"); + golem:it/api.{cart(user_id).foo}("apple"); + "success" + "#; + + let expr = Expr::from_text(expr).unwrap(); + let compiler_error = compiler::compile(&expr, &metadata).unwrap_err(); + assert_eq!( + compiler_error, + "Invalid resource method call golem:it/api.{cart(user_id).foo}. `foo` doesn't exist in resource `cart`" + ); + + } + } + #[cfg(test)] mod global_input_tests { use crate::compiler::byte_code::compiler_tests::internal; @@ -993,6 +1034,64 @@ mod compiler_tests { TypeRecord, TypeResult, TypeStr, TypeTuple, TypeU32, TypeU64, TypeVariant, }; + #[tokio::test] + async fn test_str_global_input() { + let request_value_type = AnalysedType::Str(TypeStr); + + let output_analysed_type = AnalysedType::Str(TypeStr); + + let analysed_exports = internal::get_component_metadata( + "my-worker-function", + vec![request_value_type.clone()], + output_analysed_type, + ); + + let expr = r#" + let x = request; + my-worker-function(x); + match x { + "foo" => "success", + _ => "fallback" + } + "#; + + let expr = Expr::from_text(expr).unwrap(); + let compiled = compiler::compile(&expr, &analysed_exports).unwrap(); + let expected_type_info = + internal::rib_input_type_info(vec![("request", request_value_type)]); + + assert_eq!(compiled.global_input_type_info, expected_type_info); + } + + #[tokio::test] + async fn test_number_global_input() { + let request_value_type = AnalysedType::U32(TypeU32); + + let output_analysed_type = AnalysedType::Str(TypeStr); + + let analysed_exports = internal::get_component_metadata( + "my-worker-function", + vec![request_value_type.clone()], + output_analysed_type, + ); + + let expr = r#" + let x = request; + my-worker-function(x); + match x { + 1 => "success", + 0 => "failure" + } + "#; + + let expr = Expr::from_text(expr).unwrap(); + let compiled = compiler::compile(&expr, &analysed_exports).unwrap(); + let expected_type_info = + internal::rib_input_type_info(vec![("request", request_value_type)]); + + assert_eq!(compiled.global_input_type_info, expected_type_info); + } + #[tokio::test] async fn test_variant_type_info() { let request_value_type = AnalysedType::Variant(TypeVariant { @@ -1280,72 +1379,58 @@ mod compiler_tests { } } - #[tokio::test] - async fn test_str_global_input() { - let request_value_type = AnalysedType::Str(TypeStr); - - let output_analysed_type = AnalysedType::Str(TypeStr); - - let analysed_exports = internal::get_component_metadata( - "my-worker-function", - vec![request_value_type.clone()], - output_analysed_type, - ); - - let expr = r#" - let x = request; - my-worker-function(x); - match x { - "foo" => "success", - _ => "fallback" - } - "#; - - let expr = Expr::from_text(expr).unwrap(); - let compiled = compiler::compile(&expr, &analysed_exports).unwrap(); - let expected_type_info = - internal::rib_input_type_info(vec![("request", request_value_type)]); - - assert_eq!(compiled.global_input_type_info, expected_type_info); - } - - #[tokio::test] - async fn test_number_global_input() { - let request_value_type = AnalysedType::U32(TypeU32); - - let output_analysed_type = AnalysedType::Str(TypeStr); - - let analysed_exports = internal::get_component_metadata( - "my-worker-function", - vec![request_value_type.clone()], - output_analysed_type, - ); - - let expr = r#" - let x = request; - my-worker-function(x); - match x { - 1 => "success", - 0 => "failure" - } - "#; - - let expr = Expr::from_text(expr).unwrap(); - let compiled = compiler::compile(&expr, &analysed_exports).unwrap(); - let expected_type_info = - internal::rib_input_type_info(vec![("request", request_value_type)]); - - assert_eq!(compiled.global_input_type_info, expected_type_info); - } - mod internal { use crate::RibInputTypeInfo; - use golem_wasm_ast::analysis::{ - AnalysedExport, AnalysedFunction, AnalysedFunctionParameter, AnalysedFunctionResult, - AnalysedType, - }; + use golem_wasm_ast::analysis::{AnalysedExport, AnalysedFunction, AnalysedFunctionParameter, AnalysedFunctionResult, AnalysedInstance, AnalysedResourceId, AnalysedResourceMode, AnalysedType, NameOptionTypePair, NameTypePair, TypeF32, TypeHandle, TypeList, TypeRecord, TypeStr, TypeU32, TypeVariant}; use std::collections::HashMap; + pub(crate) fn metadata_with_resource_methods() -> Vec { + let instance = AnalysedExport::Instance(AnalysedInstance { + name: "golem:it/api".to_string(), + functions: vec![ + AnalysedFunction { + name: "[constructor]cart".to_string(), + parameters: vec![AnalysedFunctionParameter { + name: "param1".to_string(), + typ: AnalysedType::Str(TypeStr), + }], + results: vec![AnalysedFunctionResult { + name: None, + typ: AnalysedType::Handle(TypeHandle { + resource_id: AnalysedResourceId(0), + mode: AnalysedResourceMode::Owned, + }), + }], + }, + AnalysedFunction { + name: "[method]cart.add-item".to_string(), + parameters: vec![ + AnalysedFunctionParameter { + name: "self".to_string(), + typ: AnalysedType::Handle(TypeHandle { + resource_id: AnalysedResourceId(0), + mode: AnalysedResourceMode::Borrowed, + }), + }, + AnalysedFunctionParameter { + name: "item".to_string(), + typ: AnalysedType::Record(TypeRecord { + fields: vec![ + NameTypePair { + name: "name".to_string(), + typ: AnalysedType::Str(TypeStr), + }, + ], + }), + }, + ], + results: vec![], + } + ], + }); + + vec![instance] + } pub(crate) fn get_component_metadata( function_name: &str, input_types: Vec, diff --git a/golem-rib/src/expr.rs b/golem-rib/src/expr.rs index cbdefb7ab..11d11cc06 100644 --- a/golem-rib/src/expr.rs +++ b/golem-rib/src/expr.rs @@ -132,6 +132,10 @@ impl Expr { matches!(self, Expr::Cond(_, _, _, _)) } + pub fn is_function_call(&self) -> bool { + matches!(self, Expr::Call(_, _, _)) + } + pub fn is_match_expr(&self) -> bool { matches!(self, Expr::PatternMatch(_, _, _)) } @@ -379,6 +383,46 @@ impl Expr { Expr::Sequence(expressions, inferred_type) } + // Useful in various scenarios such as error messages, where we need to specify the + // kind of expression even before identifying the types fully + pub fn expr_kind(&self) -> &'static str { + match self { + Expr::Let(_, _, _, _) => "let binding", + Expr::SelectField(_, _, _) => "select field", + Expr::SelectIndex(_, _, _) => "select index", + Expr::Sequence(_, _) => "list", + Expr::Record(_, _) => "record", + Expr::Tuple(_, _) => "tuple", + Expr::Literal(_, _) => "literal", + Expr::Number(_, _, _) => "number", + Expr::Flags(_, _) => "flags", + Expr::Identifier(_, _) => "identifer", + Expr::Boolean(_, _) => "boolean", + Expr::Concat(_, _) => "string", + Expr::Multiple(_, _) => "multline code block", + Expr::Not(_, _) => "not", + Expr::GreaterThan(_, _, _) => "boolean", + Expr::And(_, _, _) => "boolean", + Expr::GreaterThanOrEqualTo(_, _, _) => "boolean", + Expr::LessThanOrEqualTo(_, _, _) => "boolean", + Expr::EqualTo(_, _, _) => "boolean", + Expr::LessThan(_, _, _) => "boolean", + Expr::Cond(_, _, _, _) => "if else", + Expr::PatternMatch(_, _, _) => "pattern match", + Expr::Option(_, _) => "option", + Expr::Result(expr, _) => { + match expr { + Ok(_) => "result::ok", + Err(_) => "result::err", + } + }, + Expr::Call(_, _, _) => "function call", + Expr::Unwrap(_, _) => "unwrap", + Expr::Throw(_, _) => "throw", + Expr::GetTag(_, _) => "get tag" + } + } + pub fn inferred_type(&self) -> InferredType { match self { Expr::Let(_, _, _, inferred_type) diff --git a/golem-rib/src/function_name.rs b/golem-rib/src/function_name.rs index 52b277ec6..5a2e49022 100644 --- a/golem-rib/src/function_name.rs +++ b/golem-rib/src/function_name.rs @@ -495,6 +495,36 @@ impl ParsedFunctionReference { } } + pub fn resource_method_name(&self) -> Option { + match self { + Self::Function { .. } => None, + Self::RawResourceConstructor { resource, .. } => None, + Self::RawResourceDrop { resource, .. } => None, + Self::RawResourceMethod { + method, .. + } => Some(method.clone()), + Self::RawResourceStaticMethod { + method, .. + } => Some(method.clone()), + ParsedFunctionReference::IndexedResourceConstructor { .. } => { + None + } + ParsedFunctionReference::IndexedResourceMethod { + method, .. + } => { + Some(method.clone()) + } + ParsedFunctionReference::IndexedResourceStaticMethod { + method, .. + } => { + Some(method.clone()) + } + ParsedFunctionReference::IndexedResourceDrop { .. } => { + None + } + } + } + pub fn method_as_static(&self) -> Option { match self { Self::RawResourceMethod { resource, method } => Some(Self::RawResourceStaticMethod { diff --git a/golem-rib/src/type_inference/call_arguments_inference.rs b/golem-rib/src/type_inference/call_arguments_inference.rs index d35b8fde1..473d557db 100644 --- a/golem-rib/src/type_inference/call_arguments_inference.rs +++ b/golem-rib/src/type_inference/call_arguments_inference.rs @@ -45,7 +45,7 @@ mod internal { Expr, FunctionTypeRegistry, InferredType, ParsedFunctionName, RegistryKey, RegistryValue, }; use golem_wasm_ast::analysis::AnalysedType; - use std::fmt::Display; + use std::fmt::{Display, format}; pub(crate) fn resolve_call_argument_types( call_type: &mut CallType, @@ -57,6 +57,7 @@ mod internal { CallType::Function(dynamic_parsed_function_name) => { let parsed_function_static = dynamic_parsed_function_name.clone().to_static(); let function = parsed_function_static.clone().function; + let resource_name = function.resource_name().ok_or("Resource name not found")?; if function.resource_name().is_some() { let constructor_name = { let raw_str = function.resource_name().ok_or("Resource name not found")?; @@ -84,7 +85,24 @@ mod internal { registry_key, constructor_params, inferred_type, - )?; + ).map_err(|e| match e { + ArgTypesInferenceError::InvalidFunctionCall => { + format!("Invalid resource constructor call: {}", resource_name) + } + ArgTypesInferenceError::ArgumentSizeMisMatch { + expected, + provided, + } => format!( + "Invalid resource constructor call. {} expects {} arguments, but provided {}", + resource_name, expected, provided + ), + ArgTypesInferenceError::TypeMisMatchError { expected, provided } => { + format!( + "Invalid arguments for resource constructor {}. Expected type {:?}, but provided {}", + resource_name, expected, provided + ) + } + })?; // Infer the types of resource method parameters let resource_method_name = function.function_name(); @@ -99,16 +117,50 @@ mod internal { registry_key, args, inferred_type, - ) + ).map_err(|e| match e { + ArgTypesInferenceError::InvalidFunctionCall => { + format!("Invalid resource method call {}. `{}` doesn't exist in resource `{}`", parsed_function_static, parsed_function_static.function.resource_method_name().unwrap(), resource_name) + } + ArgTypesInferenceError::ArgumentSizeMisMatch { + expected, + provided, + } => format!( + "Invalid call: {}. Expected {} arguments, but provided {}", + parsed_function_static, expected, provided + ), + ArgTypesInferenceError::TypeMisMatchError { expected, provided } => { + format!( + "Invalid arguments to resource method {}. Expected type {:?}, but provided {}", + parsed_function_static, expected, provided + ) + } + }) } else { let registry_key = RegistryKey::from_invocation_name(call_type); infer_types( - &FunctionNameInternal::Fqn(parsed_function_static), + &FunctionNameInternal::Fqn(parsed_function_static.clone()), function_type_registry, registry_key, args, inferred_type, - ) + ).map_err(|e| match e { + ArgTypesInferenceError::InvalidFunctionCall => { + format!("Invalid function call: {}", parsed_function_static.function.function_name()) + } + ArgTypesInferenceError::ArgumentSizeMisMatch { + expected, + provided, + } => format!( + "Invalid function call: {}. Expected {} arguments, but provided {}", + parsed_function_static, expected, provided + ), + ArgTypesInferenceError::TypeMisMatchError { expected, provided } => { + format!( + "Invalid argument types in function {}. Expected type {:?}, but provided {}", + parsed_function_static.function.function_name(), expected, provided + ) + } + }) } } @@ -128,7 +180,44 @@ mod internal { registry_key, args, inferred_type, - ) + ).map_err(|e| match e { + ArgTypesInferenceError::InvalidFunctionCall => { + format!("Invalid variant constructor call: {}", variant_name) + } + ArgTypesInferenceError::ArgumentSizeMisMatch { + expected, + provided, + } => format!( + "Invalid variant construction: {}. Expected {} arguments, but provided {}", + variant_name, expected, provided + ), + ArgTypesInferenceError::TypeMisMatchError { expected, provided } => { + format!( + "Invalid type for {} construction arguments. Expected type {:?}, but provided {}", + variant_name, expected, provided + ) + } + }) + } + } + } + enum ArgTypesInferenceError { + InvalidFunctionCall, + ArgumentSizeMisMatch { + expected: usize, + provided: usize, + }, + TypeMisMatchError { + expected: AnalysedType, + provided: String // If only partial information about type is available + }, + } + + impl ArgTypesInferenceError { + fn type_mismatch(expected: AnalysedType, provided: &'static str) -> ArgTypesInferenceError { + ArgTypesInferenceError::TypeMisMatchError { + expected, + provided: provided.to_string(), } } } @@ -139,7 +228,7 @@ mod internal { key: RegistryKey, args: &mut [Expr], inferred_type: &mut InferredType, - ) -> Result<(), String> { + ) -> Result<(), ArgTypesInferenceError> { if let Some(value) = function_type_registry.types.get(&key) { match value { RegistryValue::Value(_) => Ok(()), @@ -155,12 +244,10 @@ mod internal { Ok(()) } else { - Err(format!( - "Variant {} expects {} arguments, but {} were provided", - function_name, - parameter_types.len(), - args.len() - )) + Err(ArgTypesInferenceError::ArgumentSizeMisMatch { + expected: parameter_types.len(), + provided: args.len(), + }) } } RegistryValue::Function { @@ -190,20 +277,19 @@ mod internal { Ok(()) } else { - Err(format!( - "Function {} expects {} arguments, but {} were provided", - function_name, - parameter_types.len(), - args.len() - )) + Err(ArgTypesInferenceError::ArgumentSizeMisMatch { + expected: parameter_types.len(), + provided: args.len(), + }) } } } } else { - Err(format!("Unknown function/variant call {}", function_name)) + Err(ArgTypesInferenceError::InvalidFunctionCall) } } + #[derive(Clone)] enum FunctionNameInternal { ResourceConstructorName(String), ResourceMethodName(String), @@ -231,183 +317,56 @@ mod internal { } // A preliminary check of the arguments passed before typ inference - pub(crate) fn check_function_arguments( + fn check_function_arguments( expected: &AnalysedType, passed: &Expr, - ) -> Result<(), String> { + ) -> Result<(), ArgTypesInferenceError> { + // Valid expressions, whose evaluation/inference may give the right expected type + // We will leave it to the rest of the type inference process to check the validity of the expression let valid_possibilities = passed.is_identifier() || passed.is_select_field() || passed.is_select_index() || passed.is_select_field() || passed.is_match_expr() - || passed.is_if_else(); - - match expected { - AnalysedType::U32(_) => { - if valid_possibilities || passed.is_number() { - Ok(()) - } else { - Err(format!("Expected U32, but found {:?}", passed)) - } - } - - AnalysedType::U64(_) => { - if valid_possibilities || passed.is_number() { - Ok(()) - } else { - Err(format!("Expected U64, but found {:?}", passed)) - } - } - - AnalysedType::Variant(_) => { - if valid_possibilities || passed.is_number() { - Ok(()) - } else { - Err(format!("Expected Variant, but found {:?}", passed)) - } - } - - AnalysedType::Result(_) => { - if valid_possibilities || passed.is_result() { - Ok(()) - } else { - Err(format!("Expected Result, but found {:?}", passed)) - } - } - AnalysedType::Option(_) => { - if valid_possibilities || passed.is_option() { - Ok(()) - } else { - Err(format!("Expected Option, but found {:?}", passed)) - } - } - AnalysedType::Enum(_) => { - if valid_possibilities { - Ok(()) - } else { - Err(format!("Expected Enum, but found {:?}", passed)) - } - } - AnalysedType::Flags(_) => { - if valid_possibilities || passed.is_flags() { - Ok(()) - } else { - Err(format!("Expected Flags, but found {:?}", passed)) - } - } - AnalysedType::Record(_) => { - if valid_possibilities || passed.is_record() { - Ok(()) - } else { - Err(format!("Expected Record, but found {:?}", passed)) - } - } - AnalysedType::Tuple(_) => { - if valid_possibilities || passed.is_tuple() { - Ok(()) - } else { - Err(format!("Expected Tuple, but found {:?}", passed)) - } - } - AnalysedType::List(_) => { - if valid_possibilities || passed.is_list() { - Ok(()) - } else { - Err(format!("Expected List, but found {:?}", passed)) - } - } - AnalysedType::Str(_) => { - if valid_possibilities || passed.is_concat() || passed.is_literal() { - Ok(()) - } else { - Err(format!("Expected Str, but found {:?}", passed)) - } - } - // TODO? - AnalysedType::Chr(_) => { - if valid_possibilities || passed.is_literal() { - Ok(()) - } else { - Err(format!("Expected Chr, but found {:?}", passed)) - } - } - AnalysedType::F64(_) => { - if valid_possibilities || passed.is_number() { - Ok(()) - } else { - Err(format!("Expected F64, but found {:?}", passed)) - } - } - AnalysedType::F32(_) => { - if valid_possibilities || passed.is_number() { - Ok(()) - } else { - Err(format!("Expected F32, but found {:?}", passed)) - } - } - AnalysedType::S64(_) => { - if valid_possibilities || passed.is_number() { - Ok(()) - } else { - Err(format!("Expected S64, but found {:?}", passed)) - } - } - AnalysedType::S32(_) => { - if valid_possibilities || passed.is_number() { - Ok(()) - } else { - Err(format!("Expected S32, but found {:?}", passed)) - } - } - AnalysedType::U16(_) => { - if valid_possibilities || passed.is_number() { - Ok(()) - } else { - Err(format!("Expected U16, but found {:?}", passed)) - } - } - AnalysedType::S16(_) => { - if valid_possibilities || passed.is_number() { - Ok(()) - } else { - Err(format!("Expected S16, but found {:?}", passed)) - } - } - AnalysedType::U8(_) => { - if valid_possibilities || passed.is_number() { - Ok(()) - } else { - Err(format!("Expected U8, but found {:?}", passed)) - } - } - AnalysedType::S8(_) => { - if valid_possibilities || passed.is_number() { - Ok(()) - } else { - Err(format!("Expected S8, but found {:?}", passed)) - } - } - AnalysedType::Bool(_) => { - if valid_possibilities || passed.is_boolean() || passed.is_comparison() { - Ok(()) - } else { - Err(format!("Expected Bool, but found {:?}", passed)) - } - } - AnalysedType::Handle(_) => { - if valid_possibilities { - Ok(()) - } else { - Err(format!("Expected Handle, but found {:?}", passed)) - } - } + || passed.is_if_else() + || passed.is_function_call(); + + let is_valid = match expected { + AnalysedType::U32(_) => valid_possibilities || passed.is_number(), + AnalysedType::U64(_) => valid_possibilities || passed.is_number(), + AnalysedType::Variant(_) => valid_possibilities, + AnalysedType::Result(_) => valid_possibilities || passed.is_result(), + AnalysedType::Option(_) => valid_possibilities || passed.is_option(), + AnalysedType::Enum(_) => valid_possibilities, + AnalysedType::Flags(_) => valid_possibilities || passed.is_flags(), + AnalysedType::Record(_) => valid_possibilities || passed.is_record(), + AnalysedType::Tuple(_) => valid_possibilities || passed.is_tuple(), + AnalysedType::List(_) => valid_possibilities || passed.is_list(), + AnalysedType::Str(_) => valid_possibilities || passed.is_concat() || passed.is_literal(), + AnalysedType::Chr(_) => valid_possibilities || passed.is_literal(), + AnalysedType::F64(_) => valid_possibilities || passed.is_number(), + AnalysedType::F32(_) => valid_possibilities || passed.is_number(), + AnalysedType::S64(_) => valid_possibilities || passed.is_number(), + AnalysedType::S32(_) => valid_possibilities || passed.is_number(), + AnalysedType::U16(_) => valid_possibilities || passed.is_number(), + AnalysedType::S16(_) => valid_possibilities || passed.is_number(), + AnalysedType::U8(_) => valid_possibilities || passed.is_number(), + AnalysedType::S8(_) => valid_possibilities || passed.is_number(), + AnalysedType::Bool(_) => valid_possibilities || passed.is_boolean() || passed.is_comparison(), + AnalysedType::Handle(_) => valid_possibilities + }; + + if is_valid { + Ok(()) + } else { + Err(ArgTypesInferenceError::type_mismatch(expected.clone(), passed.expr_kind())) } } fn tag_argument_types( args: &mut [Expr], parameter_types: &[AnalysedType], - ) -> Result<(), String> { + ) -> Result<(), ArgTypesInferenceError> { for (arg, param_type) in args.iter_mut().zip(parameter_types) { check_function_arguments(param_type, arg)?; arg.add_infer_type_mut(param_type.clone().into()); From 10657b1eec1f6df5a5e89427e2ea587503345848 Mon Sep 17 00:00:00 2001 From: Afsal Thaj Date: Wed, 2 Oct 2024 17:48:50 +1000 Subject: [PATCH 06/15] Add more negative tests --- golem-rib/src/compiler/byte_code.rs | 103 +++++++++++++++++- golem-rib/src/function_name.rs | 38 +++---- .../call_arguments_inference.rs | 30 ++--- 3 files changed, 131 insertions(+), 40 deletions(-) diff --git a/golem-rib/src/compiler/byte_code.rs b/golem-rib/src/compiler/byte_code.rs index 5282dfddc..31dc47a27 100644 --- a/golem-rib/src/compiler/byte_code.rs +++ b/golem-rib/src/compiler/byte_code.rs @@ -496,8 +496,8 @@ mod internal { #[cfg(test)] mod compiler_tests { use super::*; - use crate::{compiler, ArmPattern, InferredType, MatchArm, Number, VariableId}; - use golem_wasm_ast::analysis::{AnalysedType, NameTypePair, TypeRecord, TypeStr, TypeU32}; + use crate::{ArmPattern, InferredType, MatchArm, Number, VariableId}; + use golem_wasm_ast::analysis::{AnalysedType, NameTypePair, TypeRecord, TypeStr}; use golem_wasm_rpc::protobuf::type_annotated_value::TypeAnnotatedValue; #[test] @@ -986,11 +986,12 @@ mod compiler_tests { #[cfg(test)] mod invalid_function_invoke_tests { + use golem_wasm_ast::analysis::{AnalysedType, TypeStr}; use crate::{compiler, Expr}; use crate::compiler::byte_code::compiler_tests::internal; #[test] - fn test_invalid_function_call() { + fn test_unknown_function() { let expr = r#" foo(request); "success" @@ -1001,12 +1002,30 @@ mod compiler_tests { assert_eq!( compiler_error, - "Unknown function call foo" + "Unknown function call: `foo`" ); } #[test] - fn test_invalid_resource_method_call() { + fn test_unknown_resource_constructor() { + let metadata = internal::metadata_with_resource_methods(); + let expr = r#" + let user_id = "user"; + golem:it/api.{cart(user_id).add-item}("apple"); + golem:it/api.{cart0(user_id).add-item}("apple"); + "success" + "#; + + let expr = Expr::from_text(expr).unwrap(); + let compiler_error = compiler::compile(&expr, &metadata).unwrap_err(); + assert_eq!( + compiler_error, + "Unknown resource constructor call: `golem:it/api.{cart0(user_id).add-item}`. Resource `cart0` doesn't exist" + ); + } + + #[test] + fn test_unknown_resource_method() { let metadata = internal::metadata_with_resource_methods(); let expr = r#" let user_id = "user"; @@ -1021,7 +1040,79 @@ mod compiler_tests { compiler_error, "Invalid resource method call golem:it/api.{cart(user_id).foo}. `foo` doesn't exist in resource `cart`" ); + } + + #[test] + fn test_invalid_arg_size_function() { + + let metadata = + internal::get_component_metadata("foo", vec![AnalysedType::Str(TypeStr)], AnalysedType::Str(TypeStr)); + + let expr = r#" + let user_id = "user"; + let result = foo(user_id, user_id); + result + "#; + + let expr = Expr::from_text(expr).unwrap(); + let compiler_error = compiler::compile(&expr, &metadata).unwrap_err(); + assert_eq!( + compiler_error, + "Incorrect number of arguments for function `foo`. Expected 1, but provided 2" + ); + } + + #[test] + fn test_invalid_arg_size_resource_constructor() { + let metadata = internal::metadata_with_resource_methods(); + let expr = r#" + let user_id = "user"; + golem:it/api.{cart(user_id, user_id).add-item}("apple"); + "success" + "#; + + let expr = Expr::from_text(expr).unwrap(); + let compiler_error = compiler::compile(&expr, &metadata).unwrap_err(); + assert_eq!( + compiler_error, + "Incorrect number of arguments for resource constructor `cart`. Expected 1, but provided 2" + ); + } + + #[test] + fn test_invalid_arg_size_resource_method() { + let metadata = internal::metadata_with_resource_methods(); + let expr = r#" + let user_id = "user"; + golem:it/api.{cart(user_id).add-item}("apple", "samsung"); + "success" + "#; + + let expr = Expr::from_text(expr).unwrap(); + let compiler_error = compiler::compile(&expr, &metadata).unwrap_err(); + assert_eq!( + compiler_error, + "Incorrect number of arguments in resource method `golem:it/api.{cart(user_id).add-item}`. Expected 1, but provided 2" + ); + } + + #[test] + fn test_invalid_arg_types_function() { + let metadata = + internal::get_component_metadata("foo", vec![AnalysedType::Str(TypeStr)], AnalysedType::Str(TypeStr)); + let expr = r#" + let user_id = 1u64; + let result = foo(user_id); + result + "#; + + let expr = Expr::from_text(expr).unwrap(); + let compiler_error = compiler::compile(&expr, &metadata).unwrap_err(); + assert_eq!( + compiler_error, + "Incorrect number of arguments for function `foo`. Expected 1, but provided 2" + ); } } @@ -1381,7 +1472,7 @@ mod compiler_tests { mod internal { use crate::RibInputTypeInfo; - use golem_wasm_ast::analysis::{AnalysedExport, AnalysedFunction, AnalysedFunctionParameter, AnalysedFunctionResult, AnalysedInstance, AnalysedResourceId, AnalysedResourceMode, AnalysedType, NameOptionTypePair, NameTypePair, TypeF32, TypeHandle, TypeList, TypeRecord, TypeStr, TypeU32, TypeVariant}; + use golem_wasm_ast::analysis::*; use std::collections::HashMap; pub(crate) fn metadata_with_resource_methods() -> Vec { diff --git a/golem-rib/src/function_name.rs b/golem-rib/src/function_name.rs index 5a2e49022..5f1bd2987 100644 --- a/golem-rib/src/function_name.rs +++ b/golem-rib/src/function_name.rs @@ -476,20 +476,20 @@ impl ParsedFunctionReference { Self::RawResourceStaticMethod { resource, method, .. } => format!("[static]{resource}.{method}"), - ParsedFunctionReference::IndexedResourceConstructor { resource, .. } => { + Self::IndexedResourceConstructor { resource, .. } => { format!("[constructor]{resource}") } - ParsedFunctionReference::IndexedResourceMethod { + Self::IndexedResourceMethod { resource, method, .. } => { format!("[method]{resource}.{method}") } - ParsedFunctionReference::IndexedResourceStaticMethod { + Self::IndexedResourceStaticMethod { resource, method, .. } => { format!("[static]{resource}.{method}") } - ParsedFunctionReference::IndexedResourceDrop { resource, .. } => { + Self::IndexedResourceDrop { resource, .. } => { format!("[drop]{resource}") } } @@ -498,30 +498,30 @@ impl ParsedFunctionReference { pub fn resource_method_name(&self) -> Option { match self { Self::Function { .. } => None, - Self::RawResourceConstructor { resource, .. } => None, - Self::RawResourceDrop { resource, .. } => None, - Self::RawResourceMethod { - method, .. - } => Some(method.clone()), - Self::RawResourceStaticMethod { - method, .. - } => Some(method.clone()), - ParsedFunctionReference::IndexedResourceConstructor { .. } => { + Self::RawResourceConstructor { .. } => None, + Self::RawResourceDrop { .. } => None, + Self::IndexedResourceConstructor { .. } => { None } - ParsedFunctionReference::IndexedResourceMethod { + Self::IndexedResourceDrop { .. } => { + None + } + Self::IndexedResourceMethod { method, .. } => { Some(method.clone()) } - ParsedFunctionReference::IndexedResourceStaticMethod { - method, .. + Self::IndexedResourceStaticMethod { + method, .. } => { Some(method.clone()) } - ParsedFunctionReference::IndexedResourceDrop { .. } => { - None - } + Self::RawResourceMethod { + method, .. + } => Some(method.clone()), + Self::RawResourceStaticMethod { + method, .. + } => Some(method.clone()), } } diff --git a/golem-rib/src/type_inference/call_arguments_inference.rs b/golem-rib/src/type_inference/call_arguments_inference.rs index 473d557db..79d2fd464 100644 --- a/golem-rib/src/type_inference/call_arguments_inference.rs +++ b/golem-rib/src/type_inference/call_arguments_inference.rs @@ -45,7 +45,7 @@ mod internal { Expr, FunctionTypeRegistry, InferredType, ParsedFunctionName, RegistryKey, RegistryValue, }; use golem_wasm_ast::analysis::AnalysedType; - use std::fmt::{Display, format}; + use std::fmt::{Display}; pub(crate) fn resolve_call_argument_types( call_type: &mut CallType, @@ -57,11 +57,11 @@ mod internal { CallType::Function(dynamic_parsed_function_name) => { let parsed_function_static = dynamic_parsed_function_name.clone().to_static(); let function = parsed_function_static.clone().function; - let resource_name = function.resource_name().ok_or("Resource name not found")?; if function.resource_name().is_some() { + let resource_name = function.resource_name().ok_or("Resource name not found")?; + let constructor_name = { - let raw_str = function.resource_name().ok_or("Resource name not found")?; - format!["[constructor]{}", raw_str] + format!["[constructor]{}", resource_name] }; let mut constructor_params: &mut Vec = &mut vec![]; @@ -86,14 +86,14 @@ mod internal { constructor_params, inferred_type, ).map_err(|e| match e { - ArgTypesInferenceError::InvalidFunctionCall => { - format!("Invalid resource constructor call: {}", resource_name) + ArgTypesInferenceError::UnknownFunction => { + format!("Unknown resource constructor call: `{}`. Resource `{}` doesn't exist" , parsed_function_static, resource_name) } ArgTypesInferenceError::ArgumentSizeMisMatch { expected, provided, } => format!( - "Invalid resource constructor call. {} expects {} arguments, but provided {}", + "Incorrect number of arguments for resource constructor `{}`. Expected {}, but provided {}", resource_name, expected, provided ), ArgTypesInferenceError::TypeMisMatchError { expected, provided } => { @@ -118,14 +118,14 @@ mod internal { args, inferred_type, ).map_err(|e| match e { - ArgTypesInferenceError::InvalidFunctionCall => { + ArgTypesInferenceError::UnknownFunction => { format!("Invalid resource method call {}. `{}` doesn't exist in resource `{}`", parsed_function_static, parsed_function_static.function.resource_method_name().unwrap(), resource_name) } ArgTypesInferenceError::ArgumentSizeMisMatch { expected, provided, } => format!( - "Invalid call: {}. Expected {} arguments, but provided {}", + "Incorrect number of arguments in resource method `{}`. Expected {}, but provided {}", parsed_function_static, expected, provided ), ArgTypesInferenceError::TypeMisMatchError { expected, provided } => { @@ -144,14 +144,14 @@ mod internal { args, inferred_type, ).map_err(|e| match e { - ArgTypesInferenceError::InvalidFunctionCall => { - format!("Invalid function call: {}", parsed_function_static.function.function_name()) + ArgTypesInferenceError::UnknownFunction => { + format!("Unknown function call: `{}`", parsed_function_static.function.function_name()) } ArgTypesInferenceError::ArgumentSizeMisMatch { expected, provided, } => format!( - "Invalid function call: {}. Expected {} arguments, but provided {}", + "Incorrect number of arguments for function `{}`. Expected {}, but provided {}", parsed_function_static, expected, provided ), ArgTypesInferenceError::TypeMisMatchError { expected, provided } => { @@ -181,7 +181,7 @@ mod internal { args, inferred_type, ).map_err(|e| match e { - ArgTypesInferenceError::InvalidFunctionCall => { + ArgTypesInferenceError::UnknownFunction => { format!("Invalid variant constructor call: {}", variant_name) } ArgTypesInferenceError::ArgumentSizeMisMatch { @@ -202,7 +202,7 @@ mod internal { } } enum ArgTypesInferenceError { - InvalidFunctionCall, + UnknownFunction, ArgumentSizeMisMatch { expected: usize, provided: usize, @@ -285,7 +285,7 @@ mod internal { } } } else { - Err(ArgTypesInferenceError::InvalidFunctionCall) + Err(ArgTypesInferenceError::UnknownFunction) } } From d514652582741908230ef9f5faec88ae37c9cef8 Mon Sep 17 00:00:00 2001 From: Afsal Thaj Date: Wed, 2 Oct 2024 18:15:47 +1000 Subject: [PATCH 07/15] Use get kind --- golem-rib/src/compiler/byte_code.rs | 3 +- golem-rib/src/expr.rs | 40 ------ .../call_arguments_inference.rs | 48 ++----- golem-rib/src/type_inference/kind.rs | 133 ++++++++++++++++++ golem-rib/src/type_inference/mod.rs | 1 + 5 files changed, 145 insertions(+), 80 deletions(-) create mode 100644 golem-rib/src/type_inference/kind.rs diff --git a/golem-rib/src/compiler/byte_code.rs b/golem-rib/src/compiler/byte_code.rs index 31dc47a27..7c8d3baa7 100644 --- a/golem-rib/src/compiler/byte_code.rs +++ b/golem-rib/src/compiler/byte_code.rs @@ -1102,8 +1102,7 @@ mod compiler_tests { internal::get_component_metadata("foo", vec![AnalysedType::Str(TypeStr)], AnalysedType::Str(TypeStr)); let expr = r#" - let user_id = 1u64; - let result = foo(user_id); + let result = foo(1u64); result "#; diff --git a/golem-rib/src/expr.rs b/golem-rib/src/expr.rs index 11d11cc06..787e5e127 100644 --- a/golem-rib/src/expr.rs +++ b/golem-rib/src/expr.rs @@ -383,46 +383,6 @@ impl Expr { Expr::Sequence(expressions, inferred_type) } - // Useful in various scenarios such as error messages, where we need to specify the - // kind of expression even before identifying the types fully - pub fn expr_kind(&self) -> &'static str { - match self { - Expr::Let(_, _, _, _) => "let binding", - Expr::SelectField(_, _, _) => "select field", - Expr::SelectIndex(_, _, _) => "select index", - Expr::Sequence(_, _) => "list", - Expr::Record(_, _) => "record", - Expr::Tuple(_, _) => "tuple", - Expr::Literal(_, _) => "literal", - Expr::Number(_, _, _) => "number", - Expr::Flags(_, _) => "flags", - Expr::Identifier(_, _) => "identifer", - Expr::Boolean(_, _) => "boolean", - Expr::Concat(_, _) => "string", - Expr::Multiple(_, _) => "multline code block", - Expr::Not(_, _) => "not", - Expr::GreaterThan(_, _, _) => "boolean", - Expr::And(_, _, _) => "boolean", - Expr::GreaterThanOrEqualTo(_, _, _) => "boolean", - Expr::LessThanOrEqualTo(_, _, _) => "boolean", - Expr::EqualTo(_, _, _) => "boolean", - Expr::LessThan(_, _, _) => "boolean", - Expr::Cond(_, _, _, _) => "if else", - Expr::PatternMatch(_, _, _) => "pattern match", - Expr::Option(_, _) => "option", - Expr::Result(expr, _) => { - match expr { - Ok(_) => "result::ok", - Err(_) => "result::err", - } - }, - Expr::Call(_, _, _) => "function call", - Expr::Unwrap(_, _) => "unwrap", - Expr::Throw(_, _) => "throw", - Expr::GetTag(_, _) => "get tag" - } - } - pub fn inferred_type(&self) -> InferredType { match self { Expr::Let(_, _, _, inferred_type) diff --git a/golem-rib/src/type_inference/call_arguments_inference.rs b/golem-rib/src/type_inference/call_arguments_inference.rs index 79d2fd464..77a5e1117 100644 --- a/golem-rib/src/type_inference/call_arguments_inference.rs +++ b/golem-rib/src/type_inference/call_arguments_inference.rs @@ -46,6 +46,7 @@ mod internal { }; use golem_wasm_ast::analysis::AnalysedType; use std::fmt::{Display}; + use crate::type_inference::kind::{GetTypeKind, TypeKind}; pub(crate) fn resolve_call_argument_types( call_type: &mut CallType, @@ -209,15 +210,15 @@ mod internal { }, TypeMisMatchError { expected: AnalysedType, - provided: String // If only partial information about type is available + provided: TypeKind }, } impl ArgTypesInferenceError { - fn type_mismatch(expected: AnalysedType, provided: &'static str) -> ArgTypesInferenceError { + fn type_mismatch(expected: AnalysedType, provided: TypeKind) -> ArgTypesInferenceError { ArgTypesInferenceError::TypeMisMatchError { expected, - provided: provided.to_string(), + provided } } } @@ -319,47 +320,18 @@ mod internal { // A preliminary check of the arguments passed before typ inference fn check_function_arguments( expected: &AnalysedType, - passed: &Expr, + provided: &Expr, ) -> Result<(), ArgTypesInferenceError> { - // Valid expressions, whose evaluation/inference may give the right expected type - // We will leave it to the rest of the type inference process to check the validity of the expression - let valid_possibilities = passed.is_identifier() - || passed.is_select_field() - || passed.is_select_index() - || passed.is_select_field() - || passed.is_match_expr() - || passed.is_if_else() - || passed.is_function_call(); - - let is_valid = match expected { - AnalysedType::U32(_) => valid_possibilities || passed.is_number(), - AnalysedType::U64(_) => valid_possibilities || passed.is_number(), - AnalysedType::Variant(_) => valid_possibilities, - AnalysedType::Result(_) => valid_possibilities || passed.is_result(), - AnalysedType::Option(_) => valid_possibilities || passed.is_option(), - AnalysedType::Enum(_) => valid_possibilities, - AnalysedType::Flags(_) => valid_possibilities || passed.is_flags(), - AnalysedType::Record(_) => valid_possibilities || passed.is_record(), - AnalysedType::Tuple(_) => valid_possibilities || passed.is_tuple(), - AnalysedType::List(_) => valid_possibilities || passed.is_list(), - AnalysedType::Str(_) => valid_possibilities || passed.is_concat() || passed.is_literal(), - AnalysedType::Chr(_) => valid_possibilities || passed.is_literal(), - AnalysedType::F64(_) => valid_possibilities || passed.is_number(), - AnalysedType::F32(_) => valid_possibilities || passed.is_number(), - AnalysedType::S64(_) => valid_possibilities || passed.is_number(), - AnalysedType::S32(_) => valid_possibilities || passed.is_number(), - AnalysedType::U16(_) => valid_possibilities || passed.is_number(), - AnalysedType::S16(_) => valid_possibilities || passed.is_number(), - AnalysedType::U8(_) => valid_possibilities || passed.is_number(), - AnalysedType::S8(_) => valid_possibilities || passed.is_number(), - AnalysedType::Bool(_) => valid_possibilities || passed.is_boolean() || passed.is_comparison(), - AnalysedType::Handle(_) => valid_possibilities + let is_valid = if provided.inferred_type().is_unknown() { + true + } else { + provided.inferred_type().get_kind() == expected.get_kind() }; if is_valid { Ok(()) } else { - Err(ArgTypesInferenceError::type_mismatch(expected.clone(), passed.expr_kind())) + Err(ArgTypesInferenceError::type_mismatch(expected.clone(), provided.inferred_type().get_kind())) } } diff --git a/golem-rib/src/type_inference/kind.rs b/golem-rib/src/type_inference/kind.rs new file mode 100644 index 000000000..2e42db4d1 --- /dev/null +++ b/golem-rib/src/type_inference/kind.rs @@ -0,0 +1,133 @@ +use std::fmt::Display; +use golem_wasm_ast::analysis::AnalysedType; +use crate::{Expr, InferredType}; + +pub trait GetTypeKind { + fn get_kind(&self) -> TypeKind; + +} + +#[derive(PartialEq)] +pub enum TypeKind { + Record, + Tuple, + Flag, + Str, + Number, + List, + Boolean, + FunctionCall, + Option, + Enum, + Char, + Result, + Resource, + Variant, + Unknown, +} + +impl Display for TypeKind { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + TypeKind::Record => write!(f, "Record"), + TypeKind::Tuple => write!(f, "Tuple"), + TypeKind::Flag => write!(f, "Flag"), + TypeKind::Str => write!(f, "Str"), + TypeKind::Number => write!(f, "Number"), + TypeKind::List => write!(f, "List"), + TypeKind::Boolean => write!(f, "Boolean"), + TypeKind::FunctionCall => write!(f, "FunctionCall"), + TypeKind::Option => write!(f, "Option"), + TypeKind::Enum => write!(f, "Enum"), + TypeKind::Char => write!(f, "Char"), + TypeKind::Result => write!(f, "Result"), + TypeKind::Resource => write!(f, "Resource"), + TypeKind::Variant => write!(f, "Variant"), + TypeKind::Unknown => write!(f, "Unknown"), + } + } +} + +impl GetTypeKind for AnalysedType { + fn get_kind(&self) -> TypeKind { + match self { + AnalysedType::Record(_) => TypeKind::Record, + AnalysedType::Tuple(_) => TypeKind::Tuple, + AnalysedType::Flags(_) => TypeKind::Flag, + AnalysedType::Str(_) => TypeKind::Str, + AnalysedType::S8(_) => TypeKind::Number, + AnalysedType::U8(_) => TypeKind::Number, + AnalysedType::S16(_) => TypeKind::Number, + AnalysedType::U16(_) => TypeKind::Number, + AnalysedType::S32(_) => TypeKind::Number, + AnalysedType::U32(_) => TypeKind::Number, + AnalysedType::S64(_) => TypeKind::Number, + AnalysedType::U64(_) => TypeKind::Number, + AnalysedType::F32(_) => TypeKind::Number, + AnalysedType::F64(_) => TypeKind::Number, + AnalysedType::Chr(_) => TypeKind::Char, + AnalysedType::List(_) => TypeKind::List, + AnalysedType::Bool(_) => TypeKind::Boolean, + AnalysedType::Option(_) => TypeKind::Option, + AnalysedType::Enum(_) => TypeKind::Enum, + AnalysedType::Result(_) => TypeKind::Result, + AnalysedType::Handle(_) => TypeKind::Resource, + AnalysedType::Variant(_) => TypeKind::Variant, + } + } +} + +impl GetTypeKind for InferredType { + fn get_kind(&self) -> TypeKind { + match self { + InferredType::Bool => TypeKind::Boolean, + InferredType::S8 => TypeKind::Number, + InferredType::U8 => TypeKind::Number, + InferredType::S16 => TypeKind::Number, + InferredType::U16 => TypeKind::Number, + InferredType::S32 => TypeKind::Number, + InferredType::U32 => TypeKind::Number, + InferredType::S64 => TypeKind::Number, + InferredType::U64 => TypeKind::Number, + InferredType::F32 => TypeKind::Number, + InferredType::F64 => TypeKind::Number, + InferredType::Chr => TypeKind::Char, + InferredType::Str => TypeKind::Str, + InferredType::List(_) => TypeKind::List, + InferredType::Tuple(_) => TypeKind::Tuple, + InferredType::Record(_) => TypeKind::Record, + InferredType::Flags(_) => TypeKind::Flag, + InferredType::Enum(_) => TypeKind::Enum, + InferredType::Option(_) => TypeKind::Option, + InferredType::Result { .. } => TypeKind::Result, + InferredType::Variant(_) => TypeKind::Variant, + InferredType::Resource { .. } => TypeKind::Resource, + InferredType::OneOf(possibilities) => { + if let Some(first) = possibilities.first() { + let first = first.get_kind(); + if possibilities.iter().all(|p| p.get_kind() == first) { + first + } else { + TypeKind::Unknown + } + } else { + TypeKind::Unknown + } + } + InferredType::AllOf(possibilities) => { + if let Some(first) = possibilities.first() { + let first = first.get_kind(); + if possibilities.iter().all(|p| p.get_kind() == first) { + first + } else { + TypeKind::Unknown + } + } else { + TypeKind::Unknown + } + } + InferredType::Unknown => TypeKind::Unknown, + InferredType::Sequence(_) => TypeKind::Unknown + } + } +} \ No newline at end of file diff --git a/golem-rib/src/type_inference/mod.rs b/golem-rib/src/type_inference/mod.rs index d0d2aa7a2..6fbebc09d 100644 --- a/golem-rib/src/type_inference/mod.rs +++ b/golem-rib/src/type_inference/mod.rs @@ -46,6 +46,7 @@ mod enum_resolution; mod global_input_inference; mod inference_fix_point; mod type_binding; +pub(crate) mod kind; #[cfg(test)] mod type_inference_tests { From 9d9e99c5c4ebb917926b900d03e213cad2cdcab2 Mon Sep 17 00:00:00 2001 From: Afsal Thaj Date: Wed, 2 Oct 2024 20:57:52 +1000 Subject: [PATCH 08/15] Fix clippy --- golem-rib/src/compiler/byte_code.rs | 71 +++++++++++---- golem-rib/src/function_name.rs | 32 ++----- .../call_arguments_inference.rs | 43 +++++---- golem-rib/src/type_inference/kind.rs | 89 +++++++++---------- golem-rib/src/type_inference/mod.rs | 2 +- .../src/http/http_request.rs | 6 +- golem-worker-service-base/src/lib.rs | 2 +- .../compiled_golem_worker_binding.rs | 10 +-- .../worker_binding/worker_binding_resolver.rs | 26 +++--- .../src/worker_service_rib_compiler/mod.rs | 16 +--- .../src/worker_service_rib_interpreter/mod.rs | 4 +- 11 files changed, 150 insertions(+), 151 deletions(-) diff --git a/golem-rib/src/compiler/byte_code.rs b/golem-rib/src/compiler/byte_code.rs index 7c8d3baa7..2ca7e499e 100644 --- a/golem-rib/src/compiler/byte_code.rs +++ b/golem-rib/src/compiler/byte_code.rs @@ -986,9 +986,9 @@ mod compiler_tests { #[cfg(test)] mod invalid_function_invoke_tests { - use golem_wasm_ast::analysis::{AnalysedType, TypeStr}; - use crate::{compiler, Expr}; use crate::compiler::byte_code::compiler_tests::internal; + use crate::{compiler, Expr}; + use golem_wasm_ast::analysis::{AnalysedType, TypeStr}; #[test] fn test_unknown_function() { @@ -1000,10 +1000,7 @@ mod compiler_tests { let expr = Expr::from_text(expr).unwrap(); let compiler_error = compiler::compile(&expr, &vec![]).unwrap_err(); - assert_eq!( - compiler_error, - "Unknown function call: `foo`" - ); + assert_eq!(compiler_error, "Unknown function call: `foo`"); } #[test] @@ -1044,9 +1041,11 @@ mod compiler_tests { #[test] fn test_invalid_arg_size_function() { - - let metadata = - internal::get_component_metadata("foo", vec![AnalysedType::Str(TypeStr)], AnalysedType::Str(TypeStr)); + let metadata = internal::get_component_metadata( + "foo", + vec![AnalysedType::Str(TypeStr)], + AnalysedType::Str(TypeStr), + ); let expr = r#" let user_id = "user"; @@ -1098,8 +1097,11 @@ mod compiler_tests { #[test] fn test_invalid_arg_types_function() { - let metadata = - internal::get_component_metadata("foo", vec![AnalysedType::Str(TypeStr)], AnalysedType::Str(TypeStr)); + let metadata = internal::get_component_metadata( + "foo", + vec![AnalysedType::Str(TypeStr)], + AnalysedType::Str(TypeStr), + ); let expr = r#" let result = foo(1u64); @@ -1110,7 +1112,40 @@ mod compiler_tests { let compiler_error = compiler::compile(&expr, &metadata).unwrap_err(); assert_eq!( compiler_error, - "Incorrect number of arguments for function `foo`. Expected 1, but provided 2" + "Invalid argument type in function `foo`. Expected type `str`, but provided argument `1u64` is a `number`" + ); + } + + #[test] + fn test_invalid_arg_types_resource_method() { + let metadata = internal::metadata_with_resource_methods(); + let expr = r#" + let user_id = "user"; + golem:it/api.{cart(user_id).add-item}("apple"); + "success" + "#; + + let expr = Expr::from_text(expr).unwrap(); + let compiler_error = compiler::compile(&expr, &metadata).unwrap_err(); + assert_eq!( + compiler_error, + "Invalid arguments type in resource method `golem:it/api.{cart(user_id).add-item}`. Expected type `record`, but provided argument `\"apple\"` is a `str`" + ); + } + + #[test] + fn test_invalid_arg_types_resource_constructor() { + let metadata = internal::metadata_with_resource_methods(); + let expr = r#" + golem:it/api.{cart({foo : "bar"}).add-item}("apple"); + "success" + "#; + + let expr = Expr::from_text(expr).unwrap(); + let compiler_error = compiler::compile(&expr, &metadata).unwrap_err(); + assert_eq!( + compiler_error, + "Invalid argument type in resource constructor `cart`. Expected type `str`, but provided argument `{foo: \"bar\"}` is a `record`" ); } } @@ -1505,17 +1540,15 @@ mod compiler_tests { AnalysedFunctionParameter { name: "item".to_string(), typ: AnalysedType::Record(TypeRecord { - fields: vec![ - NameTypePair { - name: "name".to_string(), - typ: AnalysedType::Str(TypeStr), - }, - ], + fields: vec![NameTypePair { + name: "name".to_string(), + typ: AnalysedType::Str(TypeStr), + }], }), }, ], results: vec![], - } + }, ], }); diff --git a/golem-rib/src/function_name.rs b/golem-rib/src/function_name.rs index 5f1bd2987..662e765bf 100644 --- a/golem-rib/src/function_name.rs +++ b/golem-rib/src/function_name.rs @@ -497,31 +497,15 @@ impl ParsedFunctionReference { pub fn resource_method_name(&self) -> Option { match self { - Self::Function { .. } => None, - Self::RawResourceConstructor { .. } => None, + Self::Function { .. } => None, + Self::RawResourceConstructor { .. } => None, Self::RawResourceDrop { .. } => None, - Self::IndexedResourceConstructor { .. } => { - None - } - Self::IndexedResourceDrop { .. } => { - None - } - Self::IndexedResourceMethod { - method, .. - } => { - Some(method.clone()) - } - Self::IndexedResourceStaticMethod { - method, .. - } => { - Some(method.clone()) - } - Self::RawResourceMethod { - method, .. - } => Some(method.clone()), - Self::RawResourceStaticMethod { - method, .. - } => Some(method.clone()), + Self::IndexedResourceConstructor { .. } => None, + Self::IndexedResourceDrop { .. } => None, + Self::IndexedResourceMethod { method, .. } => Some(method.clone()), + Self::IndexedResourceStaticMethod { method, .. } => Some(method.clone()), + Self::RawResourceMethod { method, .. } => Some(method.clone()), + Self::RawResourceStaticMethod { method, .. } => Some(method.clone()), } } diff --git a/golem-rib/src/type_inference/call_arguments_inference.rs b/golem-rib/src/type_inference/call_arguments_inference.rs index 77a5e1117..56538b73b 100644 --- a/golem-rib/src/type_inference/call_arguments_inference.rs +++ b/golem-rib/src/type_inference/call_arguments_inference.rs @@ -41,12 +41,12 @@ pub fn infer_call_arguments_type( mod internal { use crate::call_type::CallType; + use crate::type_inference::kind::GetTypeKind; use crate::{ Expr, FunctionTypeRegistry, InferredType, ParsedFunctionName, RegistryKey, RegistryValue, }; use golem_wasm_ast::analysis::AnalysedType; - use std::fmt::{Display}; - use crate::type_inference::kind::{GetTypeKind, TypeKind}; + use std::fmt::Display; pub(crate) fn resolve_call_argument_types( call_type: &mut CallType, @@ -59,11 +59,10 @@ mod internal { let parsed_function_static = dynamic_parsed_function_name.clone().to_static(); let function = parsed_function_static.clone().function; if function.resource_name().is_some() { - let resource_name = function.resource_name().ok_or("Resource name not found")?; + let resource_name = + function.resource_name().ok_or("Resource name not found")?; - let constructor_name = { - format!["[constructor]{}", resource_name] - }; + let constructor_name = { format!["[constructor]{}", resource_name] }; let mut constructor_params: &mut Vec = &mut vec![]; @@ -99,8 +98,8 @@ mod internal { ), ArgTypesInferenceError::TypeMisMatchError { expected, provided } => { format!( - "Invalid arguments for resource constructor {}. Expected type {:?}, but provided {}", - resource_name, expected, provided + "Invalid argument type in resource constructor `{}`. Expected type `{}`, but provided argument `{}` is a `{}`", + resource_name, expected.get_type_kind(), provided, provided.inferred_type().get_type_kind() ) } })?; @@ -131,8 +130,8 @@ mod internal { ), ArgTypesInferenceError::TypeMisMatchError { expected, provided } => { format!( - "Invalid arguments to resource method {}. Expected type {:?}, but provided {}", - parsed_function_static, expected, provided + "Invalid arguments type in resource method `{}`. Expected type `{}`, but provided argument `{}` is a `{}`", + parsed_function_static, expected.get_type_kind(), provided, provided.inferred_type().get_type_kind() ) } }) @@ -157,8 +156,8 @@ mod internal { ), ArgTypesInferenceError::TypeMisMatchError { expected, provided } => { format!( - "Invalid argument types in function {}. Expected type {:?}, but provided {}", - parsed_function_static.function.function_name(), expected, provided + "Invalid argument type in function `{}`. Expected type `{}`, but provided argument `{}` is a `{}`", + parsed_function_static.function.function_name(), expected.get_type_kind(), provided, provided.inferred_type().get_type_kind() ) } }) @@ -194,8 +193,8 @@ mod internal { ), ArgTypesInferenceError::TypeMisMatchError { expected, provided } => { format!( - "Invalid type for {} construction arguments. Expected type {:?}, but provided {}", - variant_name, expected, provided + "Invalid argument type in variant `{}`. Expected type `{}`, but provided argument `{}` is a `{}`", + variant_name, expected.get_type_kind(), provided, provided.inferred_type().get_type_kind() ) } }) @@ -210,16 +209,13 @@ mod internal { }, TypeMisMatchError { expected: AnalysedType, - provided: TypeKind + provided: Expr, }, } impl ArgTypesInferenceError { - fn type_mismatch(expected: AnalysedType, provided: TypeKind) -> ArgTypesInferenceError { - ArgTypesInferenceError::TypeMisMatchError { - expected, - provided - } + fn type_mismatch(expected: AnalysedType, provided: Expr) -> ArgTypesInferenceError { + ArgTypesInferenceError::TypeMisMatchError { expected, provided } } } @@ -325,13 +321,16 @@ mod internal { let is_valid = if provided.inferred_type().is_unknown() { true } else { - provided.inferred_type().get_kind() == expected.get_kind() + provided.inferred_type().get_type_kind() == expected.get_type_kind() }; if is_valid { Ok(()) } else { - Err(ArgTypesInferenceError::type_mismatch(expected.clone(), provided.inferred_type().get_kind())) + Err(ArgTypesInferenceError::type_mismatch( + expected.clone(), + provided.clone(), + )) } } diff --git a/golem-rib/src/type_inference/kind.rs b/golem-rib/src/type_inference/kind.rs index 2e42db4d1..4c4386f02 100644 --- a/golem-rib/src/type_inference/kind.rs +++ b/golem-rib/src/type_inference/kind.rs @@ -1,10 +1,9 @@ -use std::fmt::Display; +use crate::InferredType; use golem_wasm_ast::analysis::AnalysedType; -use crate::{Expr, InferredType}; +use std::fmt::Display; pub trait GetTypeKind { - fn get_kind(&self) -> TypeKind; - + fn get_type_kind(&self) -> TypeKind; } #[derive(PartialEq)] @@ -16,7 +15,6 @@ pub enum TypeKind { Number, List, Boolean, - FunctionCall, Option, Enum, Char, @@ -29,27 +27,26 @@ pub enum TypeKind { impl Display for TypeKind { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { - TypeKind::Record => write!(f, "Record"), - TypeKind::Tuple => write!(f, "Tuple"), - TypeKind::Flag => write!(f, "Flag"), - TypeKind::Str => write!(f, "Str"), - TypeKind::Number => write!(f, "Number"), - TypeKind::List => write!(f, "List"), - TypeKind::Boolean => write!(f, "Boolean"), - TypeKind::FunctionCall => write!(f, "FunctionCall"), - TypeKind::Option => write!(f, "Option"), - TypeKind::Enum => write!(f, "Enum"), - TypeKind::Char => write!(f, "Char"), - TypeKind::Result => write!(f, "Result"), - TypeKind::Resource => write!(f, "Resource"), - TypeKind::Variant => write!(f, "Variant"), - TypeKind::Unknown => write!(f, "Unknown"), + TypeKind::Record => write!(f, "record"), + TypeKind::Tuple => write!(f, "tuple"), + TypeKind::Flag => write!(f, "flag"), + TypeKind::Str => write!(f, "str"), + TypeKind::Number => write!(f, "number"), + TypeKind::List => write!(f, "list"), + TypeKind::Boolean => write!(f, "boolean"), + TypeKind::Option => write!(f, "option"), + TypeKind::Enum => write!(f, "enum"), + TypeKind::Char => write!(f, "chr"), + TypeKind::Result => write!(f, "result"), + TypeKind::Resource => write!(f, "resource"), + TypeKind::Variant => write!(f, "variant"), + TypeKind::Unknown => write!(f, "unknown"), } } } impl GetTypeKind for AnalysedType { - fn get_kind(&self) -> TypeKind { + fn get_type_kind(&self) -> TypeKind { match self { AnalysedType::Record(_) => TypeKind::Record, AnalysedType::Tuple(_) => TypeKind::Tuple, @@ -78,7 +75,7 @@ impl GetTypeKind for AnalysedType { } impl GetTypeKind for InferredType { - fn get_kind(&self) -> TypeKind { + fn get_type_kind(&self) -> TypeKind { match self { InferredType::Bool => TypeKind::Boolean, InferredType::S8 => TypeKind::Number, @@ -102,32 +99,28 @@ impl GetTypeKind for InferredType { InferredType::Result { .. } => TypeKind::Result, InferredType::Variant(_) => TypeKind::Variant, InferredType::Resource { .. } => TypeKind::Resource, - InferredType::OneOf(possibilities) => { - if let Some(first) = possibilities.first() { - let first = first.get_kind(); - if possibilities.iter().all(|p| p.get_kind() == first) { - first - } else { - TypeKind::Unknown - } - } else { - TypeKind::Unknown - } - } - InferredType::AllOf(possibilities) => { - if let Some(first) = possibilities.first() { - let first = first.get_kind(); - if possibilities.iter().all(|p| p.get_kind() == first) { - first - } else { - TypeKind::Unknown - } - } else { - TypeKind::Unknown - } - } + InferredType::OneOf(possibilities) => internal::get_type_kind(possibilities), + InferredType::AllOf(possibilities) => internal::get_type_kind(possibilities), InferredType::Unknown => TypeKind::Unknown, - InferredType::Sequence(_) => TypeKind::Unknown + InferredType::Sequence(_) => TypeKind::Unknown, + } + } +} + +mod internal { + use crate::type_inference::kind::{GetTypeKind, TypeKind}; + use crate::InferredType; + + pub(crate) fn get_type_kind(possibilities: &[InferredType]) -> TypeKind { + if let Some(first) = possibilities.first() { + let first = first.get_type_kind(); + if possibilities.iter().all(|p| p.get_type_kind() == first) { + first + } else { + TypeKind::Unknown + } + } else { + TypeKind::Unknown } } -} \ No newline at end of file +} diff --git a/golem-rib/src/type_inference/mod.rs b/golem-rib/src/type_inference/mod.rs index 6fbebc09d..36ad43937 100644 --- a/golem-rib/src/type_inference/mod.rs +++ b/golem-rib/src/type_inference/mod.rs @@ -45,8 +45,8 @@ mod variant_resolution; mod enum_resolution; mod global_input_inference; mod inference_fix_point; -mod type_binding; pub(crate) mod kind; +mod type_binding; #[cfg(test)] mod type_inference_tests { diff --git a/golem-worker-service-base/src/http/http_request.rs b/golem-worker-service-base/src/http/http_request.rs index d898c8a8b..32ad75cbf 100644 --- a/golem-worker-service-base/src/http/http_request.rs +++ b/golem-worker-service-base/src/http/http_request.rs @@ -249,9 +249,9 @@ mod tests { } fn get_test_evaluator() -> Arc { - Arc::new(DefaultRibInterpreter::from_worker_request_executor(Arc::new( - TestWorkerRequestExecutor {}, - ))) + Arc::new(DefaultRibInterpreter::from_worker_request_executor( + Arc::new(TestWorkerRequestExecutor {}), + )) } #[derive(Debug)] diff --git a/golem-worker-service-base/src/lib.rs b/golem-worker-service-base/src/lib.rs index a04eb8a58..0e58fe7ec 100644 --- a/golem-worker-service-base/src/lib.rs +++ b/golem-worker-service-base/src/lib.rs @@ -13,8 +13,8 @@ pub mod repo; pub mod service; mod worker_binding; pub mod worker_bridge_execution; -pub mod worker_service_rib_interpreter; mod worker_service_rib_compiler; +pub mod worker_service_rib_interpreter; const VERSION: &str = golem_version!(); diff --git a/golem-worker-service-base/src/worker_binding/compiled_golem_worker_binding.rs b/golem-worker-service-base/src/worker_binding/compiled_golem_worker_binding.rs index 80a6fcd83..ac05de1ba 100644 --- a/golem-worker-service-base/src/worker_binding/compiled_golem_worker_binding.rs +++ b/golem-worker-service-base/src/worker_binding/compiled_golem_worker_binding.rs @@ -1,9 +1,9 @@ use crate::worker_binding::{GolemWorkerBinding, ResponseMapping}; +use crate::worker_service_rib_compiler::{DefaultRibCompiler, WorkerServiceRibCompiler}; use bincode::{Decode, Encode}; use golem_service_base::model::VersionedComponentId; use golem_wasm_ast::analysis::AnalysedExport; use rib::{Expr, RibByteCode, RibInputTypeInfo}; -use crate::worker_service_rib_compiler::{DefaultRibCompiler, WorkerServiceRibCompiler}; #[derive(Debug, Clone, PartialEq, Encode, Decode)] pub struct CompiledGolemWorkerBinding { @@ -16,7 +16,7 @@ pub struct CompiledGolemWorkerBinding { impl CompiledGolemWorkerBinding { pub fn from_golem_worker_binding( golem_worker_binding: &GolemWorkerBinding, - export_metadata: &Vec, + export_metadata: &[AnalysedExport], ) -> Result { let worker_name_compiled = WorkerNameCompiled::from_worker_name( &golem_worker_binding.worker_name, @@ -53,7 +53,7 @@ pub struct WorkerNameCompiled { impl WorkerNameCompiled { pub fn from_worker_name( worker_name: &Expr, - exports: &Vec, + exports: &[AnalysedExport], ) -> Result { let worker_name_compiled = DefaultRibCompiler::compile(worker_name, exports)?; @@ -75,7 +75,7 @@ pub struct IdempotencyKeyCompiled { impl IdempotencyKeyCompiled { pub fn from_idempotency_key( idempotency_key: &Expr, - exports: &Vec, + exports: &[AnalysedExport], ) -> Result { let idempotency_key_compiled = DefaultRibCompiler::compile(idempotency_key, exports)?; @@ -97,7 +97,7 @@ pub struct ResponseMappingCompiled { impl ResponseMappingCompiled { pub fn from_response_mapping( response_mapping: &ResponseMapping, - exports: &Vec, + exports: &[AnalysedExport], ) -> Result { let response_compiled = DefaultRibCompiler::compile(&response_mapping.0, exports)?; diff --git a/golem-worker-service-base/src/worker_binding/worker_binding_resolver.rs b/golem-worker-service-base/src/worker_binding/worker_binding_resolver.rs index a1962bdf7..bbd14564c 100644 --- a/golem-worker-service-base/src/worker_binding/worker_binding_resolver.rs +++ b/golem-worker-service-base/src/worker_binding/worker_binding_resolver.rs @@ -2,8 +2,8 @@ use crate::api_definition::http::{CompiledHttpApiDefinition, VarInfo}; use crate::http::http_request::router; use crate::http::router::RouterPattern; use crate::http::InputHttpRequest; +use crate::worker_service_rib_interpreter::EvaluationError; use crate::worker_service_rib_interpreter::WorkerServiceRibInterpreter; -use crate::worker_service_rib_interpreter::{EvaluationError}; use async_trait::async_trait; use golem_common::model::IdempotencyKey; use golem_service_base::model::VersionedComponentId; @@ -172,24 +172,24 @@ impl RequestToWorkerBindingResolver for InputHttpRequ // To evaluate worker-name, most probably let worker_name: String = rib::interpret_pure( &binding.worker_name_compiled.compiled_worker_name, - &resolve_rib_input.value + &resolve_rib_input.value, ) - .await - .map_err(|err| format!("Failed to evaluate worker name expression. {}", err))? - .get_literal() - .ok_or("Worker name is not a String".to_string())? - .as_string(); + .await + .map_err(|err| format!("Failed to evaluate worker name expression. {}", err))? + .get_literal() + .ok_or("Worker name is not a String".to_string())? + .as_string(); let component_id = &binding.component_id; let idempotency_key = if let Some(idempotency_key_compiled) = &binding.idempotency_key_compiled { - let idempotency_key_value = rib::interpret_pure( - &idempotency_key_compiled.compiled_idempotency_key, - &resolve_rib_input.value, - ) - .await - .map_err(|err| err.to_string())?; + let idempotency_key_value = rib::interpret_pure( + &idempotency_key_compiled.compiled_idempotency_key, + &resolve_rib_input.value, + ) + .await + .map_err(|err| err.to_string())?; let idempotency_key = idempotency_key_value .get_literal() diff --git a/golem-worker-service-base/src/worker_service_rib_compiler/mod.rs b/golem-worker-service-base/src/worker_service_rib_compiler/mod.rs index 569419656..14ca2738b 100644 --- a/golem-worker-service-base/src/worker_service_rib_compiler/mod.rs +++ b/golem-worker-service-base/src/worker_service_rib_compiler/mod.rs @@ -4,25 +4,17 @@ use rib::{CompilerOutput, Expr}; // A wrapper service over original Rib Compiler concerning // the details of the worker bridge. pub trait WorkerServiceRibCompiler { - - fn compile( - rib: &Expr, - export_metadata: &Vec, - ) -> Result; + fn compile(rib: &Expr, export_metadata: &[AnalysedExport]) -> Result; } - pub struct DefaultRibCompiler; impl WorkerServiceRibCompiler for DefaultRibCompiler { - fn compile( - rib: &Expr, - export_metadata: &Vec, - ) -> Result { + fn compile(rib: &Expr, export_metadata: &[AnalysedExport]) -> Result { rib::compile_with_limited_globals( rib, - export_metadata, + &export_metadata.to_vec(), Some(vec!["request".to_string()]), ) } -} \ No newline at end of file +} diff --git a/golem-worker-service-base/src/worker_service_rib_interpreter/mod.rs b/golem-worker-service-base/src/worker_service_rib_interpreter/mod.rs index a3ed0a1ce..3ef7cafe2 100644 --- a/golem-worker-service-base/src/worker_service_rib_interpreter/mod.rs +++ b/golem-worker-service-base/src/worker_service_rib_interpreter/mod.rs @@ -10,13 +10,12 @@ use golem_common::model::{ComponentId, IdempotencyKey}; use crate::worker_binding::RibInputValue; use rib::{RibByteCode, RibFunctionInvoke, RibInterpreterResult}; -use crate::worker_bridge_execution::{ WorkerRequest, WorkerRequestExecutor}; +use crate::worker_bridge_execution::{WorkerRequest, WorkerRequestExecutor}; // A wrapper service over original RibInterpreter concerning // the details of the worker service. #[async_trait] pub trait WorkerServiceRibInterpreter { - // Evaluate a Rib byte against a specific worker. // RibByteCode may have actual function calls in a worker. async fn evaluate( @@ -105,4 +104,3 @@ impl WorkerServiceRibInterpreter for DefaultRibInterpreter { .map_err(EvaluationError) } } - From 0044a1e328d51a504d55703dc23e3dc1cc596928 Mon Sep 17 00:00:00 2001 From: Afsal Thaj Date: Wed, 2 Oct 2024 21:14:43 +1000 Subject: [PATCH 09/15] Fix error messages --- golem-rib/src/compiler/byte_code.rs | 8 ++++---- .../src/type_inference/call_arguments_inference.rs | 10 +++++----- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/golem-rib/src/compiler/byte_code.rs b/golem-rib/src/compiler/byte_code.rs index 2ca7e499e..8a4cf3b6d 100644 --- a/golem-rib/src/compiler/byte_code.rs +++ b/golem-rib/src/compiler/byte_code.rs @@ -1035,7 +1035,7 @@ mod compiler_tests { let compiler_error = compiler::compile(&expr, &metadata).unwrap_err(); assert_eq!( compiler_error, - "Invalid resource method call golem:it/api.{cart(user_id).foo}. `foo` doesn't exist in resource `cart`" + "Unknown resource method call golem:it/api.{cart(user_id).foo}. `foo` doesn't exist in resource `cart`" ); } @@ -1112,7 +1112,7 @@ mod compiler_tests { let compiler_error = compiler::compile(&expr, &metadata).unwrap_err(); assert_eq!( compiler_error, - "Invalid argument type in function `foo`. Expected type `str`, but provided argument `1u64` is a `number`" + "Invalid type for the argument in function `foo`. Expected type `str`, but provided argument `1u64` is a `number`" ); } @@ -1129,7 +1129,7 @@ mod compiler_tests { let compiler_error = compiler::compile(&expr, &metadata).unwrap_err(); assert_eq!( compiler_error, - "Invalid arguments type in resource method `golem:it/api.{cart(user_id).add-item}`. Expected type `record`, but provided argument `\"apple\"` is a `str`" + "Invalid type for the argument in resource method `golem:it/api.{cart(user_id).add-item}`. Expected type `record`, but provided argument `\"apple\"` is a `str`" ); } @@ -1145,7 +1145,7 @@ mod compiler_tests { let compiler_error = compiler::compile(&expr, &metadata).unwrap_err(); assert_eq!( compiler_error, - "Invalid argument type in resource constructor `cart`. Expected type `str`, but provided argument `{foo: \"bar\"}` is a `record`" + "Invalid type for the argument in resource constructor `cart`. Expected type `str`, but provided argument `{foo: \"bar\"}` is a `record`" ); } } diff --git a/golem-rib/src/type_inference/call_arguments_inference.rs b/golem-rib/src/type_inference/call_arguments_inference.rs index 56538b73b..f9f2052f5 100644 --- a/golem-rib/src/type_inference/call_arguments_inference.rs +++ b/golem-rib/src/type_inference/call_arguments_inference.rs @@ -98,7 +98,7 @@ mod internal { ), ArgTypesInferenceError::TypeMisMatchError { expected, provided } => { format!( - "Invalid argument type in resource constructor `{}`. Expected type `{}`, but provided argument `{}` is a `{}`", + "Invalid type for the argument in resource constructor `{}`. Expected type `{}`, but provided argument `{}` is a `{}`", resource_name, expected.get_type_kind(), provided, provided.inferred_type().get_type_kind() ) } @@ -119,7 +119,7 @@ mod internal { inferred_type, ).map_err(|e| match e { ArgTypesInferenceError::UnknownFunction => { - format!("Invalid resource method call {}. `{}` doesn't exist in resource `{}`", parsed_function_static, parsed_function_static.function.resource_method_name().unwrap(), resource_name) + format!("Unknown resource method call {}. `{}` doesn't exist in resource `{}`", parsed_function_static, parsed_function_static.function.resource_method_name().unwrap(), resource_name) } ArgTypesInferenceError::ArgumentSizeMisMatch { expected, @@ -130,7 +130,7 @@ mod internal { ), ArgTypesInferenceError::TypeMisMatchError { expected, provided } => { format!( - "Invalid arguments type in resource method `{}`. Expected type `{}`, but provided argument `{}` is a `{}`", + "Invalid type for the argument in resource method `{}`. Expected type `{}`, but provided argument `{}` is a `{}`", parsed_function_static, expected.get_type_kind(), provided, provided.inferred_type().get_type_kind() ) } @@ -156,7 +156,7 @@ mod internal { ), ArgTypesInferenceError::TypeMisMatchError { expected, provided } => { format!( - "Invalid argument type in function `{}`. Expected type `{}`, but provided argument `{}` is a `{}`", + "Invalid type for the argument in function `{}`. Expected type `{}`, but provided argument `{}` is a `{}`", parsed_function_static.function.function_name(), expected.get_type_kind(), provided, provided.inferred_type().get_type_kind() ) } @@ -193,7 +193,7 @@ mod internal { ), ArgTypesInferenceError::TypeMisMatchError { expected, provided } => { format!( - "Invalid argument type in variant `{}`. Expected type `{}`, but provided argument `{}` is a `{}`", + "Invalid type for the argument in variant `{}`. Expected type `{}`, but provided argument `{}` is a `{}`", variant_name, expected.get_type_kind(), provided, provided.inferred_type().get_type_kind() ) } From 0b92194c76535090903052a71b469d497ca1ceb3 Mon Sep 17 00:00:00 2001 From: Afsal Thaj Date: Thu, 3 Oct 2024 11:02:03 +1000 Subject: [PATCH 10/15] Renamings and fix comments --- .../src/type_inference/rib_input_type.rs | 2 ++ .../src/api/custom_http_request_api.rs | 25 ++++++++------ .../src/api/register_api_definition_api.rs | 2 +- .../http/http_api_definition.rs | 5 +++ .../compiled_golem_worker_binding.rs | 8 ++--- .../rib_input_value_resolver.rs | 4 +++ .../worker_binding/worker_binding_resolver.rs | 34 ++++++++++--------- .../src/worker_service_rib_interpreter/mod.rs | 2 +- 8 files changed, 49 insertions(+), 33 deletions(-) diff --git a/golem-rib/src/type_inference/rib_input_type.rs b/golem-rib/src/type_inference/rib_input_type.rs index 6b144e097..0b4a23ea9 100644 --- a/golem-rib/src/type_inference/rib_input_type.rs +++ b/golem-rib/src/type_inference/rib_input_type.rs @@ -20,6 +20,8 @@ use poem_openapi::Object; use serde::{Deserialize, Serialize}; use std::collections::{HashMap, VecDeque}; +// RibInputTypeInfo refers to the required global inputs to a RibScript +// with its type information. Example: `request` variable which should be of the type `Record`. #[derive(Debug, Clone, PartialEq, Serialize, Deserialize, Encode, Decode, Object)] pub struct RibInputTypeInfo { pub types: HashMap, diff --git a/golem-worker-service-base/src/api/custom_http_request_api.rs b/golem-worker-service-base/src/api/custom_http_request_api.rs index 4cddc492f..23ebec6ea 100644 --- a/golem-worker-service-base/src/api/custom_http_request_api.rs +++ b/golem-worker-service-base/src/api/custom_http_request_api.rs @@ -19,7 +19,7 @@ use crate::worker_bridge_execution::WorkerRequestExecutor; // This is a common API projects can make use of, similar to healthcheck service #[derive(Clone)] pub struct CustomHttpRequestApi { - pub evaluator: Arc, + pub worker_service_rib_interpreter: Arc, pub api_definition_lookup_service: Arc + Sync + Send>, } @@ -36,7 +36,7 @@ impl CustomHttpRequestApi { )); Self { - evaluator, + worker_service_rib_interpreter: evaluator, api_definition_lookup_service, } } @@ -71,7 +71,7 @@ impl CustomHttpRequestApi { } }; - let api_request = InputHttpRequest { + let input_http_request = InputHttpRequest { input_path: ApiInputPath { base_path: uri.path().to_string(), query_path: uri.query().map(|x| x.to_string()), @@ -83,25 +83,28 @@ impl CustomHttpRequestApi { let possible_api_definitions = match self .api_definition_lookup_service - .get(api_request.clone()) + .get(input_http_request.clone()) .await { - Ok(api_definition) => api_definition, - Err(err) => { - error!("API request host: {} - error: {}", host, err); + Ok(api_defs) => api_defs, + Err(api_defs_lookup_error) => { + error!( + "API request host: {} - error: {}", + host, api_defs_lookup_error + ); return Response::builder() .status(StatusCode::INTERNAL_SERVER_ERROR) .body(Body::from_string("Internal error".to_string())); } }; - match api_request + match input_http_request .resolve_worker_binding(possible_api_definitions) .await { - Ok(resolved_worker_request) => { - resolved_worker_request - .interpret_response_mapping::(&self.evaluator) + Ok(resolved_worker_binding) => { + resolved_worker_binding + .interpret_response_mapping(&self.worker_service_rib_interpreter) .await } diff --git a/golem-worker-service-base/src/api/register_api_definition_api.rs b/golem-worker-service-base/src/api/register_api_definition_api.rs index d47f9aa55..aa9d6344f 100644 --- a/golem-worker-service-base/src/api/register_api_definition_api.rs +++ b/golem-worker-service-base/src/api/register_api_definition_api.rs @@ -158,7 +158,7 @@ impl From for GolemWorkerBindingWithTypeInfo { .response_rib_expr .to_string(), response_mapping_input: Some(worker_binding.response_compiled.rib_input), - worker_name_input: Some(worker_binding.worker_name_compiled.rib_input), + worker_name_input: Some(worker_binding.worker_name_compiled.rib_input_type_info), idempotency_key_input: value .idempotency_key_compiled .map(|idempotency_key_compiled| idempotency_key_compiled.rib_input), diff --git a/golem-worker-service-base/src/api_definition/http/http_api_definition.rs b/golem-worker-service-base/src/api_definition/http/http_api_definition.rs index 59dda81e5..b374f005c 100644 --- a/golem-worker-service-base/src/api_definition/http/http_api_definition.rs +++ b/golem-worker-service-base/src/api_definition/http/http_api_definition.rs @@ -80,6 +80,11 @@ impl From for HttpApiDefinition { } } +// The Rib Expressions that exists in various parts of HttpApiDefinition (mainly in Routes) +// are compiled to form CompiledHttpApiDefinition. +// The Compilation happens during API definition registration, +// and is persisted, so that custom http requests are served by looking up +// CompiledHttpApiDefinition #[derive(Debug, Clone, PartialEq)] pub struct CompiledHttpApiDefinition { pub id: ApiDefinitionId, diff --git a/golem-worker-service-base/src/worker_binding/compiled_golem_worker_binding.rs b/golem-worker-service-base/src/worker_binding/compiled_golem_worker_binding.rs index ac05de1ba..85e6c111e 100644 --- a/golem-worker-service-base/src/worker_binding/compiled_golem_worker_binding.rs +++ b/golem-worker-service-base/src/worker_binding/compiled_golem_worker_binding.rs @@ -47,7 +47,7 @@ impl CompiledGolemWorkerBinding { pub struct WorkerNameCompiled { pub worker_name: Expr, pub compiled_worker_name: RibByteCode, - pub rib_input: RibInputTypeInfo, + pub rib_input_type_info: RibInputTypeInfo, } impl WorkerNameCompiled { @@ -60,7 +60,7 @@ impl WorkerNameCompiled { Ok(WorkerNameCompiled { worker_name: worker_name.clone(), compiled_worker_name: worker_name_compiled.byte_code, - rib_input: worker_name_compiled.global_input_type_info, + rib_input_type_info: worker_name_compiled.global_input_type_info, }) } } @@ -153,7 +153,7 @@ impl TryFrom .ok_or("Missing worker name".to_string()) .and_then(Expr::try_from)?, compiled_worker_name: worker_name_compiled, - rib_input: worker_name_input, + rib_input_type_info: worker_name_input, }; let idempotency_key_compiled = match (idempotency_key_compiled, idempotency_key_input) { @@ -197,7 +197,7 @@ impl TryFrom let worker_name = Some(value.worker_name_compiled.worker_name.into()); let compiled_worker_name_expr = Some(value.worker_name_compiled.compiled_worker_name.into()); - let worker_name_rib_input = Some(value.worker_name_compiled.rib_input.into()); + let worker_name_rib_input = Some(value.worker_name_compiled.rib_input_type_info.into()); let (idempotency_key, compiled_idempotency_key_expr, idempotency_key_rib_input) = match value.idempotency_key_compiled { Some(x) => ( diff --git a/golem-worker-service-base/src/worker_binding/rib_input_value_resolver.rs b/golem-worker-service-base/src/worker_binding/rib_input_value_resolver.rs index c61dc9091..2cf02a47d 100644 --- a/golem-worker-service-base/src/worker_binding/rib_input_value_resolver.rs +++ b/golem-worker-service-base/src/worker_binding/rib_input_value_resolver.rs @@ -5,6 +5,10 @@ use rib::RibInputTypeInfo; use std::collections::HashMap; use std::fmt::Display; +// `RibInputValueResolver` is responsible +// for converting to RibInputValue which is in the right shape +// to act as input for Rib Script. Example: HttpRequestDetails +// can be converted to RibInputValue pub trait RibInputValueResolver { fn resolve_rib_input_value( &self, diff --git a/golem-worker-service-base/src/worker_binding/worker_binding_resolver.rs b/golem-worker-service-base/src/worker_binding/worker_binding_resolver.rs index bbd14564c..f9d85b7e8 100644 --- a/golem-worker-service-base/src/worker_binding/worker_binding_resolver.rs +++ b/golem-worker-service-base/src/worker_binding/worker_binding_resolver.rs @@ -17,12 +17,9 @@ use crate::worker_binding::rib_input_value_resolver::RibInputValueResolver; use crate::worker_binding::{RequestDetails, ResponseMappingCompiled, RibInputTypeMismatch}; use crate::worker_bridge_execution::to_response::ToResponse; -// Every request (http or others) can have an instance of this resolver -// to resolve to a single worker-binding with additional-info which is required for worker_service_rib_interpreter -// TODO; It will be better if worker binding resolver -// able to deal with only one API definition -// as the first stage resolution can take place (based on host, input request (route resolution) -// up the stage +// Every type of request (example: InputHttpRequest (which corresponds to a Route)) can have an instance of this resolver, +// to resolve a single worker-binding is then executed with the help of worker_service_rib_interpreter, which internally +// calls the worker function. #[async_trait] pub trait RequestToWorkerBindingResolver { async fn resolve_worker_binding( @@ -127,15 +124,15 @@ impl ResolvedWorkerBindingFromRequest { impl RequestToWorkerBindingResolver for InputHttpRequest { async fn resolve_worker_binding( &self, - api_definition: Vec, + compiled_api_definitions: Vec, ) -> Result { - let routes = api_definition + let compiled_routes = compiled_api_definitions .iter() .flat_map(|x| x.routes.clone()) .collect::>(); let api_request = self; - let router = router::build(routes); + let router = router::build(compiled_routes); let path: Vec<&str> = RouterPattern::split(&api_request.input_path.base_path).collect(); let request_query_variables = self.input_path.query_components().unwrap_or_default(); let request_body = &self.req_body; @@ -156,7 +153,7 @@ impl RequestToWorkerBindingResolver for InputHttpRequ .collect() }; - let request_details = RequestDetails::from( + let http_request_details = RequestDetails::from( &zipped_path_params, &request_query_variables, query_params, @@ -165,9 +162,14 @@ impl RequestToWorkerBindingResolver for InputHttpRequ ) .map_err(|err| format!("Failed to fetch input request details {}", err.join(", ")))?; - let resolve_rib_input = request_details - .resolve_rib_input_value(&binding.worker_name_compiled.rib_input) - .map_err(|err| format!("Failed to resolve rib input value {}", err))?; + let resolve_rib_input = http_request_details + .resolve_rib_input_value(&binding.worker_name_compiled.rib_input_type_info) + .map_err(|err| { + format!( + "Failed to resolve rib input value from http request details {}", + err + ) + })?; // To evaluate worker-name, most probably let worker_name: String = rib::interpret_pure( @@ -175,9 +177,9 @@ impl RequestToWorkerBindingResolver for InputHttpRequ &resolve_rib_input.value, ) .await - .map_err(|err| format!("Failed to evaluate worker name expression. {}", err))? + .map_err(|err| format!("Failed to evaluate worker name rib expression. {}", err))? .get_literal() - .ok_or("Worker name is not a String".to_string())? + .ok_or("Worker name is not a Rib expression that resolves to String".to_string())? .as_string(); let component_id = &binding.component_id; @@ -212,7 +214,7 @@ impl RequestToWorkerBindingResolver for InputHttpRequ let resolved_binding = ResolvedWorkerBindingFromRequest { worker_detail, - request_details, + request_details: http_request_details, compiled_response_mapping: binding.response_compiled.clone(), }; diff --git a/golem-worker-service-base/src/worker_service_rib_interpreter/mod.rs b/golem-worker-service-base/src/worker_service_rib_interpreter/mod.rs index 3ef7cafe2..0a735c240 100644 --- a/golem-worker-service-base/src/worker_service_rib_interpreter/mod.rs +++ b/golem-worker-service-base/src/worker_service_rib_interpreter/mod.rs @@ -17,7 +17,7 @@ use crate::worker_bridge_execution::{WorkerRequest, WorkerRequestExecutor}; #[async_trait] pub trait WorkerServiceRibInterpreter { // Evaluate a Rib byte against a specific worker. - // RibByteCode may have actual function calls in a worker. + // RibByteCode may have actual function calls. async fn evaluate( &self, worker_name: &str, From a492d7e06a77c37738ee0fbbcb3fce8d2a262fb9 Mon Sep 17 00:00:00 2001 From: Afsal Thaj Date: Thu, 3 Oct 2024 11:02:09 +1000 Subject: [PATCH 11/15] Renamings and fix comments --- golem-rib/src/compiler/desugar.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/golem-rib/src/compiler/desugar.rs b/golem-rib/src/compiler/desugar.rs index e2c2a0ac5..cab54a592 100644 --- a/golem-rib/src/compiler/desugar.rs +++ b/golem-rib/src/compiler/desugar.rs @@ -261,19 +261,19 @@ mod internal { ) -> Option { match pred_expr_inferred_type { InferredType::Record(field_and_types) => { - // Resolution body is a list of expressions which grows (may be with some let bindings) + // Resolution body is a list of expressions which grows (maybe with some let bindings) // as we recursively iterate over the bind patterns // where bind patterns are {name: x, age: _, address : _ } in the case of `match record { {name: x, age: _, address : _ } ) =>` - // These will exist prior to the original resolution of a successful tuple match. + // These will exist prior to the original resolution of a successful record match. let mut resolution_body = vec![]; // The conditions keep growing as we recursively iterate over the bind patterns - // and there are multiple conditions (if condition) for each element in the tuple + // and there are multiple conditions (if condition) for each element in the record. let mut conditions = vec![]; - // We assume pred-expr can be queried by field using Expr::select_field and we pick each element in the bind pattern + // We assume pred-expr can be queried by field using Expr::select_field, and we pick each element in the bind pattern // to get the corresponding expr in pred-expr and keep recursively iterating until the record is completed. - // However there is no resolution body for each of this iteration, so we use an empty expression + // However, there is no resolution body for each of this iteration, so we use an empty expression // and finally push the original resolution body once we fully build the conditions. for (field, arm_pattern) in bind_patterns.iter() { let new_pred = Expr::select_field(pred_expr.clone(), field); From 3883c20470301b05209951959628d77d4c87f5b6 Mon Sep 17 00:00:00 2001 From: Afsal Thaj Date: Thu, 3 Oct 2024 13:39:12 +1000 Subject: [PATCH 12/15] Avoid repetition in error messages --- golem-rib/src/compiler/byte_code.rs | 75 ++++- golem-rib/src/function_name.rs | 14 +- .../call_arguments_inference.rs | 306 ++++++++++++------ 3 files changed, 280 insertions(+), 115 deletions(-) diff --git a/golem-rib/src/compiler/byte_code.rs b/golem-rib/src/compiler/byte_code.rs index 8a4cf3b6d..71e6ed990 100644 --- a/golem-rib/src/compiler/byte_code.rs +++ b/golem-rib/src/compiler/byte_code.rs @@ -1035,7 +1035,7 @@ mod compiler_tests { let compiler_error = compiler::compile(&expr, &metadata).unwrap_err(); assert_eq!( compiler_error, - "Unknown resource method call golem:it/api.{cart(user_id).foo}. `foo` doesn't exist in resource `cart`" + "Unknown resource method call `golem:it/api.{cart(user_id).foo}`. `foo` doesn't exist in resource `cart`" ); } @@ -1095,6 +1095,24 @@ mod compiler_tests { ); } + #[test] + fn test_invalid_arg_size_variants() { + let metadata = internal::metadata_with_variants(); + + let expr = r#" + let regiser_user_action = register-user(1, "foo"); + let result = golem:it/api.{foo}(regiser_user_action); + result + "#; + + let expr = Expr::from_text(expr).unwrap(); + let compiler_error = compiler::compile(&expr, &metadata).unwrap_err(); + assert_eq!( + compiler_error, + "Invalid number of arguments in variant `register-user`. Expected 1, but provided 2" + ); + } + #[test] fn test_invalid_arg_types_function() { let metadata = internal::get_component_metadata( @@ -1148,6 +1166,24 @@ mod compiler_tests { "Invalid type for the argument in resource constructor `cart`. Expected type `str`, but provided argument `{foo: \"bar\"}` is a `record`" ); } + + #[test] + fn test_invalid_arg_types_variants() { + let metadata = internal::metadata_with_variants(); + + let expr = r#" + let regiser_user_action = register-user("foo"); + let result = golem:it/api.{foo}(regiser_user_action); + result + "#; + + let expr = Expr::from_text(expr).unwrap(); + let compiler_error = compiler::compile(&expr, &metadata).unwrap_err(); + assert_eq!( + compiler_error, + "Invalid type for the argument in variant constructor `register-user`. Expected type `number`, but provided argument `\"foo\"` is a `str`" + ); + } } #[cfg(test)] @@ -1509,6 +1545,43 @@ mod compiler_tests { use golem_wasm_ast::analysis::*; use std::collections::HashMap; + pub(crate) fn metadata_with_variants() -> Vec { + let instance = AnalysedExport::Instance(AnalysedInstance { + name: "golem:it/api".to_string(), + functions: vec![AnalysedFunction { + name: "foo".to_string(), + parameters: vec![AnalysedFunctionParameter { + name: "param1".to_string(), + typ: AnalysedType::Variant(TypeVariant { + cases: vec![ + NameOptionTypePair { + name: "register-user".to_string(), + typ: Some(AnalysedType::U64(TypeU64)), + }, + NameOptionTypePair { + name: "process-user".to_string(), + typ: Some(AnalysedType::Str(TypeStr)), + }, + NameOptionTypePair { + name: "validate".to_string(), + typ: None, + }, + ], + }), + }], + results: vec![AnalysedFunctionResult { + name: None, + typ: AnalysedType::Handle(TypeHandle { + resource_id: AnalysedResourceId(0), + mode: AnalysedResourceMode::Owned, + }), + }], + }], + }); + + vec![instance] + } + pub(crate) fn metadata_with_resource_methods() -> Vec { let instance = AnalysedExport::Instance(AnalysedInstance { name: "golem:it/api".to_string(), diff --git a/golem-rib/src/function_name.rs b/golem-rib/src/function_name.rs index 662e765bf..2586d7a6a 100644 --- a/golem-rib/src/function_name.rs +++ b/golem-rib/src/function_name.rs @@ -497,15 +497,11 @@ impl ParsedFunctionReference { pub fn resource_method_name(&self) -> Option { match self { - Self::Function { .. } => None, - Self::RawResourceConstructor { .. } => None, - Self::RawResourceDrop { .. } => None, - Self::IndexedResourceConstructor { .. } => None, - Self::IndexedResourceDrop { .. } => None, - Self::IndexedResourceMethod { method, .. } => Some(method.clone()), - Self::IndexedResourceStaticMethod { method, .. } => Some(method.clone()), - Self::RawResourceMethod { method, .. } => Some(method.clone()), - Self::RawResourceStaticMethod { method, .. } => Some(method.clone()), + Self::IndexedResourceStaticMethod { method, .. } + | Self::RawResourceMethod { method, .. } + | Self::RawResourceStaticMethod { method, .. } + | Self::IndexedResourceMethod { method, .. } => Some(method.clone()), + _ => None, } } diff --git a/golem-rib/src/type_inference/call_arguments_inference.rs b/golem-rib/src/type_inference/call_arguments_inference.rs index f9f2052f5..af221d8ee 100644 --- a/golem-rib/src/type_inference/call_arguments_inference.rs +++ b/golem-rib/src/type_inference/call_arguments_inference.rs @@ -80,29 +80,21 @@ mod internal { // Infer the types of constructor parameter expressions infer_types( - &FunctionNameInternal::ResourceConstructorName(constructor_name), + &FunctionTypeInternal::ResourceConstructorName { + fqn: parsed_function_static.to_string(), + resource_constructor_name_pretty: parsed_function_static + .function + .resource_name() + .cloned() + .unwrap_or_default(), + resource_constructor_name: constructor_name, + }, function_type_registry, registry_key, constructor_params, inferred_type, - ).map_err(|e| match e { - ArgTypesInferenceError::UnknownFunction => { - format!("Unknown resource constructor call: `{}`. Resource `{}` doesn't exist" , parsed_function_static, resource_name) - } - ArgTypesInferenceError::ArgumentSizeMisMatch { - expected, - provided, - } => format!( - "Incorrect number of arguments for resource constructor `{}`. Expected {}, but provided {}", - resource_name, expected, provided - ), - ArgTypesInferenceError::TypeMisMatchError { expected, provided } => { - format!( - "Invalid type for the argument in resource constructor `{}`. Expected type `{}`, but provided argument `{}` is a `{}`", - resource_name, expected.get_type_kind(), provided, provided.inferred_type().get_type_kind() - ) - } - })?; + ) + .map_err(|e| e.to_string())?; // Infer the types of resource method parameters let resource_method_name = function.function_name(); @@ -112,55 +104,35 @@ mod internal { ); infer_types( - &FunctionNameInternal::ResourceMethodName(resource_method_name), + &FunctionTypeInternal::ResourceMethodName { + fqn: parsed_function_static.to_string(), + resource_constructor_name_pretty: parsed_function_static + .function + .resource_name() + .cloned() + .unwrap_or_default(), + resource_method_name_pretty: parsed_function_static + .function + .resource_method_name() + .unwrap_or_default(), + resource_method_name, + }, function_type_registry, registry_key, args, inferred_type, - ).map_err(|e| match e { - ArgTypesInferenceError::UnknownFunction => { - format!("Unknown resource method call {}. `{}` doesn't exist in resource `{}`", parsed_function_static, parsed_function_static.function.resource_method_name().unwrap(), resource_name) - } - ArgTypesInferenceError::ArgumentSizeMisMatch { - expected, - provided, - } => format!( - "Incorrect number of arguments in resource method `{}`. Expected {}, but provided {}", - parsed_function_static, expected, provided - ), - ArgTypesInferenceError::TypeMisMatchError { expected, provided } => { - format!( - "Invalid type for the argument in resource method `{}`. Expected type `{}`, but provided argument `{}` is a `{}`", - parsed_function_static, expected.get_type_kind(), provided, provided.inferred_type().get_type_kind() - ) - } - }) + ) + .map_err(|e| e.to_string()) } else { let registry_key = RegistryKey::from_invocation_name(call_type); infer_types( - &FunctionNameInternal::Fqn(parsed_function_static.clone()), + &FunctionTypeInternal::Fqn(parsed_function_static.to_string()), function_type_registry, registry_key, args, inferred_type, - ).map_err(|e| match e { - ArgTypesInferenceError::UnknownFunction => { - format!("Unknown function call: `{}`", parsed_function_static.function.function_name()) - } - ArgTypesInferenceError::ArgumentSizeMisMatch { - expected, - provided, - } => format!( - "Incorrect number of arguments for function `{}`. Expected {}, but provided {}", - parsed_function_static, expected, provided - ), - ArgTypesInferenceError::TypeMisMatchError { expected, provided } => { - format!( - "Invalid type for the argument in function `{}`. Expected type `{}`, but provided argument `{}` is a `{}`", - parsed_function_static.function.function_name(), expected.get_type_kind(), provided, provided.inferred_type().get_type_kind() - ) - } - }) + ) + .map_err(|e| e.to_string()) } } @@ -175,57 +147,159 @@ mod internal { CallType::VariantConstructor(variant_name) => { let registry_key = RegistryKey::FunctionName(variant_name.clone()); infer_types( - &FunctionNameInternal::VariantName(variant_name.clone()), + &FunctionTypeInternal::VariantName(variant_name.clone()), function_type_registry, registry_key, args, inferred_type, - ).map_err(|e| match e { - ArgTypesInferenceError::UnknownFunction => { - format!("Invalid variant constructor call: {}", variant_name) - } - ArgTypesInferenceError::ArgumentSizeMisMatch { - expected, - provided, - } => format!( - "Invalid variant construction: {}. Expected {} arguments, but provided {}", - variant_name, expected, provided - ), - ArgTypesInferenceError::TypeMisMatchError { expected, provided } => { - format!( - "Invalid type for the argument in variant `{}`. Expected type `{}`, but provided argument `{}` is a `{}`", - variant_name, expected.get_type_kind(), provided, provided.inferred_type().get_type_kind() - ) - } - }) + ) + .map_err(|e| e.to_string()) } } } - enum ArgTypesInferenceError { - UnknownFunction, + + // An internal error type for all possibilities of errors + // when inferring the type of arguments + enum FunctionArgsTypeInferenceError { + UnknownFunction(FunctionTypeInternal), ArgumentSizeMisMatch { + function_type_internal: FunctionTypeInternal, expected: usize, provided: usize, }, TypeMisMatchError { + function_type_internal: FunctionTypeInternal, expected: AnalysedType, provided: Expr, }, } - impl ArgTypesInferenceError { - fn type_mismatch(expected: AnalysedType, provided: Expr) -> ArgTypesInferenceError { - ArgTypesInferenceError::TypeMisMatchError { expected, provided } + impl FunctionArgsTypeInferenceError { + fn type_mismatch( + function_type_internal: FunctionTypeInternal, + expected: AnalysedType, + provided: Expr, + ) -> FunctionArgsTypeInferenceError { + FunctionArgsTypeInferenceError::TypeMisMatchError { + function_type_internal, + expected, + provided, + } + } + } + + impl Display for FunctionArgsTypeInferenceError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + fn unknown_function_type( + function_type_internal: &FunctionTypeInternal, + ) -> &'static str { + match function_type_internal { + FunctionTypeInternal::ResourceConstructorName { .. } => "resource constructor", + FunctionTypeInternal::ResourceMethodName { .. } => "resource method", + FunctionTypeInternal::Fqn(_) => "function", + FunctionTypeInternal::VariantName(_) => "variant constructor", + } + } + match self { + FunctionArgsTypeInferenceError::UnknownFunction(FunctionTypeInternal::Fqn( + parsed_function_name, + )) => { + write!(f, "Unknown function call: `{}`", parsed_function_name) + } + FunctionArgsTypeInferenceError::UnknownFunction( + FunctionTypeInternal::ResourceMethodName { + fqn, + resource_constructor_name_pretty: resource_name_human, + resource_method_name_pretty: resource_method_name_human, + .. + }, + ) => { + write!( + f, + "Unknown resource method call `{}`. `{}` doesn't exist in resource `{}`", + fqn, resource_method_name_human, resource_name_human + ) + } + FunctionArgsTypeInferenceError::UnknownFunction( + FunctionTypeInternal::ResourceConstructorName { + fqn, + resource_constructor_name_pretty: resource_constructor_name_human, + .. + }, + ) => { + write!( + f, + "Unknown resource constructor call: `{}`. Resource `{}` doesn't exist", + fqn, resource_constructor_name_human + ) + } + + FunctionArgsTypeInferenceError::UnknownFunction( + FunctionTypeInternal::VariantName(variant_name), + ) => { + write!(f, "Invalid variant constructor call: {}", variant_name) + } + + FunctionArgsTypeInferenceError::TypeMisMatchError { + function_type_internal, + expected, + provided, + } => match function_type_internal { + FunctionTypeInternal::ResourceConstructorName { + fqn, + resource_constructor_name_pretty: resource_constructor_name_human, + .. + } => { + write!(f,"Invalid type for the argument in resource constructor `{}`. Expected type `{}`, but provided argument `{}` is a `{}`", resource_constructor_name_human, expected.get_type_kind(), provided, provided.inferred_type().get_type_kind()) + } + FunctionTypeInternal::ResourceMethodName { + fqn, + .. + } => { + write!(f,"Invalid type for the argument in resource method `{}`. Expected type `{}`, but provided argument `{}` is a `{}`", fqn, expected.get_type_kind(), provided, provided.inferred_type().get_type_kind()) + } + FunctionTypeInternal::Fqn(fqn) => { + write!(f,"Invalid type for the argument in function `{}`. Expected type `{}`, but provided argument `{}` is a `{}`", fqn, expected.get_type_kind(), provided, provided.inferred_type().get_type_kind()) + } + FunctionTypeInternal::VariantName(str) => { + write!(f,"Invalid type for the argument in variant constructor `{}`. Expected type `{}`, but provided argument `{}` is a `{}`", str, expected.get_type_kind(), provided, provided.inferred_type().get_type_kind()) + } + }, + FunctionArgsTypeInferenceError::ArgumentSizeMisMatch { + function_type_internal, + expected, + provided, + } => match function_type_internal { + FunctionTypeInternal::ResourceConstructorName { + resource_constructor_name_pretty, + .. + } => { + write!(f, "Incorrect number of arguments for resource constructor `{}`. Expected {}, but provided {}", resource_constructor_name_pretty, expected, provided) + } + FunctionTypeInternal::ResourceMethodName { + fqn, + .. + } => { + write!(f, "Incorrect number of arguments in resource method `{}`. Expected {}, but provided {}", fqn, expected, provided) + } + FunctionTypeInternal::Fqn(fqn) => { + write!(f, "Incorrect number of arguments for function `{}`. Expected {}, but provided {}", fqn, expected, provided) + } + FunctionTypeInternal::VariantName(str) => { + write!(f, "Invalid number of arguments in variant `{}`. Expected {}, but provided {}", str, expected, provided) + } + }, + } } } fn infer_types( - function_name: &FunctionNameInternal, + function_name: &FunctionTypeInternal, function_type_registry: &FunctionTypeRegistry, key: RegistryKey, args: &mut [Expr], inferred_type: &mut InferredType, - ) -> Result<(), ArgTypesInferenceError> { + ) -> Result<(), FunctionArgsTypeInferenceError> { if let Some(value) = function_type_registry.types.get(&key) { match value { RegistryValue::Value(_) => Ok(()), @@ -236,12 +310,13 @@ mod internal { let parameter_types = parameter_types.clone(); if parameter_types.len() == args.len() { - tag_argument_types(args, ¶meter_types)?; + tag_argument_types(function_name, args, ¶meter_types)?; *inferred_type = InferredType::from_variant_cases(variant_type); Ok(()) } else { - Err(ArgTypesInferenceError::ArgumentSizeMisMatch { + Err(FunctionArgsTypeInferenceError::ArgumentSizeMisMatch { + function_type_internal: function_name.clone(), expected: parameter_types.len(), provided: args.len(), }) @@ -253,14 +328,14 @@ mod internal { } => { let mut parameter_types = parameter_types.clone(); - if let FunctionNameInternal::ResourceMethodName(_) = function_name { + if let FunctionTypeInternal::ResourceMethodName { .. } = function_name { if let Some(AnalysedType::Handle(_)) = parameter_types.first() { parameter_types.remove(0); } } if parameter_types.len() == args.len() { - tag_argument_types(args, ¶meter_types)?; + tag_argument_types(function_name, args, ¶meter_types)?; *inferred_type = { if return_types.len() == 1 { @@ -274,7 +349,8 @@ mod internal { Ok(()) } else { - Err(ArgTypesInferenceError::ArgumentSizeMisMatch { + Err(FunctionArgsTypeInferenceError::ArgumentSizeMisMatch { + function_type_internal: function_name.clone(), expected: parameter_types.len(), provided: args.len(), }) @@ -282,31 +358,48 @@ mod internal { } } } else { - Err(ArgTypesInferenceError::UnknownFunction) + Err(FunctionArgsTypeInferenceError::UnknownFunction( + function_name.clone(), + )) } } #[derive(Clone)] - enum FunctionNameInternal { - ResourceConstructorName(String), - ResourceMethodName(String), - Fqn(ParsedFunctionName), + enum FunctionTypeInternal { + ResourceConstructorName { + fqn: String, + resource_constructor_name_pretty: String, + resource_constructor_name: String, + }, + ResourceMethodName { + fqn: String, + resource_constructor_name_pretty: String, + resource_method_name_pretty: String, + resource_method_name: String, + }, + Fqn(String), VariantName(String), } - impl Display for FunctionNameInternal { + impl Display for FunctionTypeInternal { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { - FunctionNameInternal::ResourceConstructorName(name) => { - write!(f, "{}", name) + FunctionTypeInternal::ResourceConstructorName { + resource_constructor_name, + .. + } => { + write!(f, "{}", resource_constructor_name) } - FunctionNameInternal::ResourceMethodName(name) => { - write!(f, "{}", name) + FunctionTypeInternal::ResourceMethodName { + resource_method_name, + .. + } => { + write!(f, "{}", resource_method_name) } - FunctionNameInternal::Fqn(name) => { - write!(f, "{}", name) + FunctionTypeInternal::Fqn(fqn) => { + write!(f, "{}", fqn) } - FunctionNameInternal::VariantName(name) => { + FunctionTypeInternal::VariantName(name) => { write!(f, "{}", name) } } @@ -315,9 +408,10 @@ mod internal { // A preliminary check of the arguments passed before typ inference fn check_function_arguments( + function_name: &FunctionTypeInternal, expected: &AnalysedType, provided: &Expr, - ) -> Result<(), ArgTypesInferenceError> { + ) -> Result<(), FunctionArgsTypeInferenceError> { let is_valid = if provided.inferred_type().is_unknown() { true } else { @@ -327,7 +421,8 @@ mod internal { if is_valid { Ok(()) } else { - Err(ArgTypesInferenceError::type_mismatch( + Err(FunctionArgsTypeInferenceError::type_mismatch( + function_name.clone(), expected.clone(), provided.clone(), )) @@ -335,11 +430,12 @@ mod internal { } fn tag_argument_types( + function_name: &FunctionTypeInternal, args: &mut [Expr], parameter_types: &[AnalysedType], - ) -> Result<(), ArgTypesInferenceError> { + ) -> Result<(), FunctionArgsTypeInferenceError> { for (arg, param_type) in args.iter_mut().zip(parameter_types) { - check_function_arguments(param_type, arg)?; + check_function_arguments(function_name, param_type, arg)?; arg.add_infer_type_mut(param_type.clone().into()); } From 5ad10a066d7b73ac29239163ad53458a68ac7d3a Mon Sep 17 00:00:00 2001 From: Afsal Thaj Date: Thu, 3 Oct 2024 13:40:19 +1000 Subject: [PATCH 13/15] Make the error type str --- .../call_arguments_inference.rs | 25 +++---------------- 1 file changed, 3 insertions(+), 22 deletions(-) diff --git a/golem-rib/src/type_inference/call_arguments_inference.rs b/golem-rib/src/type_inference/call_arguments_inference.rs index af221d8ee..855d4dca4 100644 --- a/golem-rib/src/type_inference/call_arguments_inference.rs +++ b/golem-rib/src/type_inference/call_arguments_inference.rs @@ -42,9 +42,7 @@ pub fn infer_call_arguments_type( mod internal { use crate::call_type::CallType; use crate::type_inference::kind::GetTypeKind; - use crate::{ - Expr, FunctionTypeRegistry, InferredType, ParsedFunctionName, RegistryKey, RegistryValue, - }; + use crate::{Expr, FunctionTypeRegistry, InferredType, RegistryKey, RegistryValue}; use golem_wasm_ast::analysis::AnalysedType; use std::fmt::Display; @@ -190,16 +188,6 @@ mod internal { impl Display for FunctionArgsTypeInferenceError { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - fn unknown_function_type( - function_type_internal: &FunctionTypeInternal, - ) -> &'static str { - match function_type_internal { - FunctionTypeInternal::ResourceConstructorName { .. } => "resource constructor", - FunctionTypeInternal::ResourceMethodName { .. } => "resource method", - FunctionTypeInternal::Fqn(_) => "function", - FunctionTypeInternal::VariantName(_) => "variant constructor", - } - } match self { FunctionArgsTypeInferenceError::UnknownFunction(FunctionTypeInternal::Fqn( parsed_function_name, @@ -246,16 +234,12 @@ mod internal { provided, } => match function_type_internal { FunctionTypeInternal::ResourceConstructorName { - fqn, resource_constructor_name_pretty: resource_constructor_name_human, .. } => { write!(f,"Invalid type for the argument in resource constructor `{}`. Expected type `{}`, but provided argument `{}` is a `{}`", resource_constructor_name_human, expected.get_type_kind(), provided, provided.inferred_type().get_type_kind()) } - FunctionTypeInternal::ResourceMethodName { - fqn, - .. - } => { + FunctionTypeInternal::ResourceMethodName { fqn, .. } => { write!(f,"Invalid type for the argument in resource method `{}`. Expected type `{}`, but provided argument `{}` is a `{}`", fqn, expected.get_type_kind(), provided, provided.inferred_type().get_type_kind()) } FunctionTypeInternal::Fqn(fqn) => { @@ -276,10 +260,7 @@ mod internal { } => { write!(f, "Incorrect number of arguments for resource constructor `{}`. Expected {}, but provided {}", resource_constructor_name_pretty, expected, provided) } - FunctionTypeInternal::ResourceMethodName { - fqn, - .. - } => { + FunctionTypeInternal::ResourceMethodName { fqn, .. } => { write!(f, "Incorrect number of arguments in resource method `{}`. Expected {}, but provided {}", fqn, expected, provided) } FunctionTypeInternal::Fqn(fqn) => { From 595cd7a590022ae42f05d425e2b67b88e222b9a4 Mon Sep 17 00:00:00 2001 From: Afsal Thaj Date: Thu, 3 Oct 2024 13:45:16 +1000 Subject: [PATCH 14/15] Add comments --- golem-rib/src/interpreter/mod.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/golem-rib/src/interpreter/mod.rs b/golem-rib/src/interpreter/mod.rs index cf78699c1..ab44f8e5c 100644 --- a/golem-rib/src/interpreter/mod.rs +++ b/golem-rib/src/interpreter/mod.rs @@ -36,6 +36,9 @@ pub async fn interpret( interpreter.run(rib.clone()).await } +// This function can be used for those the Rib Scripts +// where there are no side effecting function calls. +// It is recommended to use `interpret` over `interpret_pure` if you are unsure. pub async fn interpret_pure( rib: &RibByteCode, rib_input: &HashMap, From db77bd6cae8e93a470672c4a9aa91500e9a3b4f1 Mon Sep 17 00:00:00 2001 From: Afsal Thaj Date: Thu, 3 Oct 2024 13:50:03 +1000 Subject: [PATCH 15/15] Avoid underscore patterns --- golem-rib/src/type_registry.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/golem-rib/src/type_registry.rs b/golem-rib/src/type_registry.rs index aba79726e..feee91b9a 100644 --- a/golem-rib/src/type_registry.rs +++ b/golem-rib/src/type_registry.rs @@ -251,7 +251,10 @@ mod internal { AnalysedType::Option(type_option) => { update_registry(type_option.inner.as_ref(), registry); } - AnalysedType::Result(_) => {} + AnalysedType::Result(TypeResult { + ok: None, + err: None, + }) => {} AnalysedType::Flags(_) => {} AnalysedType::Str(_) => {} AnalysedType::Chr(_) => {}