From 1db34002d1d95151f74ce01626bb08de83c70acc Mon Sep 17 00:00:00 2001 From: Markus Westerlind Date: Wed, 9 Nov 2022 12:10:39 +0100 Subject: [PATCH] refactor: Remove flux_analyze (#5335) * refactor: Remove flux_analyze It can be implemented in terms of `flux_analyze_with` * chore: make generate --- libflux/c/main.c | 89 +---------------------------- libflux/flux/src/cffi.rs | 55 +++++------------- libflux/go/libflux/analyze.go | 44 ++++++-------- libflux/go/libflux/buildinfo.gen.go | 4 +- libflux/include/influxdata/flux.h | 11 ---- 5 files changed, 36 insertions(+), 167 deletions(-) diff --git a/libflux/c/main.c b/libflux/c/main.c index 2a1fd66412..7ada60d501 100644 --- a/libflux/c/main.c +++ b/libflux/c/main.c @@ -11,7 +11,6 @@ void test_env_stdlib(); int main(int argc, char* argv[]) { test_ast(); - test_semantic(); test_stateful_analyzer(); test_env_stdlib(); return 0; @@ -68,91 +67,6 @@ void test_ast() { } } -void test_semantic() { - printf("Testing semantic graph functions...\n"); - - { - printf("Parsing to AST\n"); - struct flux_ast_pkg_t *ast_pkg_foo = flux_parse("test", "package foo\nx = 1 + 1"); - assert(ast_pkg_foo != NULL); - - printf("Analyzing (expect success)\n"); - struct flux_semantic_pkg_t* sem_pkg = NULL; - struct flux_error_t* err = flux_analyze(ast_pkg_foo, "", &sem_pkg); - assert(err == NULL); - - printf("Marshaling to FlatBuffer\n"); - struct flux_buffer_t buf; - err = flux_semantic_marshal_fb(sem_pkg, &buf); - assert(err == NULL); - printf(" FlatBuffer is length %ld\n", buf.len); - flux_free_bytes(buf.data); - - flux_free_semantic_pkg(sem_pkg); - } - - { - printf("Parsing to AST\n"); - struct flux_ast_pkg_t *ast_pkg_foo = flux_parse("test", "package foo\nx = 1 + 1.0"); - assert(ast_pkg_foo != NULL); - - printf("Analyzing (expect failure)\n"); - struct flux_semantic_pkg_t* sem_pkg = NULL; - struct flux_error_t* err = flux_analyze(ast_pkg_foo, "", &sem_pkg); - assert(err != NULL); - assert(sem_pkg != NULL); - const char* err_str = flux_error_str(err); - printf(" error: %s\n", err_str); - flux_free_error(err); - flux_free_semantic_pkg(sem_pkg); - } - - { - printf("Parsing to AST\n"); - struct flux_ast_pkg_t *ast_pkg_foo = flux_parse("test", "package foo\nx = 1 + 1"); - assert(ast_pkg_foo != NULL); - - struct flux_semantic_pkg_t* sem_pkg = NULL; - struct flux_error_t* err = flux_analyze(ast_pkg_foo, "", &sem_pkg); - assert(err == NULL); - assert(sem_pkg != NULL); - - printf("Find variable type v (expect success)\n"); - struct flux_buffer_t buf; - err = flux_find_var_type(sem_pkg, "v", &buf); - // Note that we do not call flux_free_ast_pkg(ast_pkg_foo); here because we will - // consume the AST package during the conversion from the AST package to the semantic package. - assert(err == NULL); - printf(" FlatBuffer is length %ld\n", buf.len); - flux_free_bytes(buf.data); - flux_free_semantic_pkg(sem_pkg); - } - - { - printf("Parsing to AST\n"); - struct flux_ast_pkg_t *ast_pkg_foo = flux_parse("test", "package foo\nx = 1 + 1.0"); - assert(ast_pkg_foo != NULL); - - struct flux_semantic_pkg_t* sem_pkg = NULL; - struct flux_error_t* err = flux_analyze(ast_pkg_foo, "", &sem_pkg); - assert(err != NULL); - assert(sem_pkg != NULL); - const char* err_str = flux_error_str(err); - printf(" error: %s\n", err_str); - flux_free_error(err); - - printf("Find variable type v (expect failure)\n"); - struct flux_buffer_t buf; - err = flux_find_var_type(sem_pkg, "v", &buf); - assert(err == NULL); - - flux_free_bytes(buf.data); - flux_free_semantic_pkg(sem_pkg); - } - - printf("\n"); -} - void test_stateful_analyzer() { printf("Testing semantic analyzer...\n"); @@ -186,7 +100,8 @@ void test_stateful_analyzer() { sem_pkg = NULL; err = flux_analyze_with(analyzer, NULL, ast_pkg, &sem_pkg); assert(err != NULL); - assert(sem_pkg == NULL); + assert(sem_pkg != NULL); + flux_free_semantic_pkg(sem_pkg); const char* err_str = flux_error_str(err); printf(" error: %s\n", err_str); flux_error_print(err); diff --git a/libflux/flux/src/cffi.rs b/libflux/flux/src/cffi.rs index 3339bbca5e..ce06c9943f 100644 --- a/libflux/flux/src/cffi.rs +++ b/libflux/flux/src/cffi.rs @@ -344,40 +344,6 @@ pub unsafe extern "C" fn flux_merge_ast_pkgs( .unwrap_or_else(|err| Some(err.into())) } -/// flux_analyze is a C-compatible wrapper around the analyze() function below -/// -/// Note that Box is used to indicate we are receiving/returning a C pointer and also -/// transferring ownership. -/// -/// # Safety -/// -/// This function is unsafe because it dereferences a raw pointer. -#[no_mangle] -#[allow(clippy::boxed_local)] -pub unsafe extern "C" fn flux_analyze( - ast_pkg: Box, - options: *const c_char, - out_sem_pkg: *mut Option>, -) -> Option> { - catch_unwind(|| { - let options = match Options::from_c_str(options) { - Ok(x) => x, - Err(err) => return Some(err.into()), - }; - match analyze(&ast_pkg, options) { - Ok(sem_pkg) => { - *out_sem_pkg = Some(Box::new(sem_pkg)); - None - } - Err(salvage) => { - *out_sem_pkg = salvage.value.map(Box::new); - Some(salvage.error.into()) - } - } - }) - .unwrap_or_else(|err| Some(err.into())) -} - /// flux_find_var_type() is a C-compatible wrapper around the find_var_type() function below. /// Note that Box is used to indicate we are receiving/returning a C pointer and also /// transferring ownership. @@ -455,7 +421,10 @@ pub struct StatefulAnalyzer { } impl StatefulAnalyzer { - fn analyze(&mut self, ast_pkg: &ast::Package) -> Result { + fn analyze( + &mut self, + ast_pkg: &ast::Package, + ) -> SalvageResult { let Options { features } = self.options.clone(); let env = Environment::from(&self.env); @@ -471,7 +440,7 @@ impl StatefulAnalyzer { let (mut env, sem_pkg) = match result { Ok(r) => r, Err(e) => { - return Err(e.error.into()); + return Err(e.err_into().map(|(_, x)| x)); } }; @@ -550,19 +519,21 @@ pub unsafe extern "C" fn flux_analyze_with( Some(std::str::from_utf8(CStr::from_ptr(csrc).to_bytes()).unwrap()) }; - let sem_pkg = Box::new(match analyzer.analyze(ast_pkg) { - Ok(sem_pkg) => sem_pkg, + match analyzer.analyze(ast_pkg) { + Ok(sem_pkg) => { + *out_sem_pkg = Some(Box::new(sem_pkg)); + } Err(mut err) => { + *out_sem_pkg = err.value.map(Box::new); if let Some(src) = src { - if let Error::Semantic(err) = &mut err { + if let Error::Semantic(err) = &mut err.error { err.source = Some(src.into()); } } - return Some(err.into()); + return Some(err.error.into()); } - }); + } - *out_sem_pkg = Some(sem_pkg); None }) .unwrap_or_else(|err| Some(err.into())) diff --git a/libflux/go/libflux/analyze.go b/libflux/go/libflux/analyze.go index 26d0c51df4..747b8d5c9b 100644 --- a/libflux/go/libflux/analyze.go +++ b/libflux/go/libflux/analyze.go @@ -120,7 +120,6 @@ func Analyze(astPkg *ASTPkg) (*SemanticPkg, error) { } func AnalyzeWithOptions(astPkg *ASTPkg, options Options) (*SemanticPkg, error) { - var semPkg *C.struct_flux_semantic_pkg_t defer func() { // This is necessary because the ASTPkg returned from the libflux API calls has its finalizer // set with the Go runtime. But this API will consume the AST package during @@ -136,25 +135,13 @@ func AnalyzeWithOptions(astPkg *ASTPkg, options Options) (*SemanticPkg, error) { cOptions := C.CString(stringOptions) defer C.free(unsafe.Pointer(cOptions)) - // Analyze may return a semantic package even on errors - fluxErr := C.flux_analyze(astPkg.ptr, cOptions, &semPkg) - - runtime.KeepAlive(astPkg) - - var p *SemanticPkg - if semPkg != nil { - p = &SemanticPkg{ptr: semPkg} - runtime.SetFinalizer(p, free) - } + analyzer, err := NewAnalyzerWithOptions(options) + semPkg, fluxErr := analyzer.Analyze("", astPkg) if fluxErr != nil { - defer C.flux_free_error(fluxErr) - cstr := C.flux_error_str(fluxErr) - str := C.GoString(cstr) - err = errors.New(codes.Invalid, str) + err = fluxErr.GoError() } - - return p, err + return semPkg, err } func AnalyzeString(script string) (*SemanticPkg, error) { @@ -233,7 +220,7 @@ func (p *Analyzer) Analyze(src string, astPkg *ASTPkg) (*SemanticPkg, *FluxError csrc := C.CString(src) defer C.free(unsafe.Pointer(csrc)) - var semPkg *C.struct_flux_semantic_pkg_t + var cSemPkg *C.struct_flux_semantic_pkg_t defer func() { // This is necessary because the ASTPkg returned from the libflux API calls has its finalizer // set with the Go runtime. But this API will consume the AST package during @@ -242,16 +229,23 @@ func (p *Analyzer) Analyze(src string, astPkg *ASTPkg) (*SemanticPkg, *FluxError astPkg.ptr = nil }() - if err := C.flux_analyze_with(p.ptr, csrc, astPkg.ptr, &semPkg); err != nil { - err := &FluxError{ptr: err} + fluxErr := C.flux_analyze_with(p.ptr, csrc, astPkg.ptr, &cSemPkg) + + runtime.KeepAlive(astPkg) + + var semPkg *SemanticPkg + if cSemPkg != nil { + semPkg = &SemanticPkg{ptr: cSemPkg} + runtime.SetFinalizer(semPkg, free) + } + + var err *FluxError + if fluxErr != nil { + err = &FluxError{ptr: fluxErr} runtime.SetFinalizer(err, free) - return nil, err } - runtime.KeepAlive(p) - pkg := &SemanticPkg{ptr: semPkg} - runtime.SetFinalizer(pkg, free) - return pkg, nil + return semPkg, err } // Free frees the memory allocated by Rust for the semantic graph. diff --git a/libflux/go/libflux/buildinfo.gen.go b/libflux/go/libflux/buildinfo.gen.go index 8bbf4220cb..a60c43f670 100644 --- a/libflux/go/libflux/buildinfo.gen.go +++ b/libflux/go/libflux/buildinfo.gen.go @@ -62,13 +62,13 @@ var sourceHashes = map[string]string{ "libflux/flux/Cargo.toml": "308c541b31f6ef25094ed4a4c2fddaf38244b0632b93c1f44b2ee4b4209d479c", "libflux/flux/FLUXDOC.md": "92e6dd8043bd87b4924e09aa28fb5346630aee1214de28ea2c8fc0687cad0785", "libflux/flux/build.rs": "31dcb1e825555e56b4d959244c4ea630b1d32ccddc1f8615620e0c23552d914f", - "libflux/flux/src/cffi.rs": "de2fc6176cc106b41826f37b2538f8617bc9485eafe098fa2e8008e0ddddb4e3", + "libflux/flux/src/cffi.rs": "27c789e117b7c0d298217444129b5d999edd6e6e6f2c7d51e5f2e698b85712fe", "libflux/flux/src/lib.rs": "af2f62a02ec08ee476121e035c6587e31c1b6d5d4912ccace1e2e9ae019ad9c1", "libflux/flux/templates/base.html": "a818747b9621828bb96b94291c60922db54052bbe35d5e354f8e589d2a4ebd02", "libflux/flux/templates/home.html": "f9927514dd42ca7271b4817ad1ca33ec79c03a77a783581b4dcafabd246ebf3f", "libflux/flux/templates/package.html": "635e1d638ad1a430f3894ff0a458572ca3c4dc6b9cec2c57fe771443849e1402", "libflux/flux/templates/value.html": "2af699af2535f83635acbc75c92d2ee941c2335350fc7e82ceb2dccf479360bf", - "libflux/include/influxdata/flux.h": "35f4bfcf252329f6f941c75096488d7b5a902f05a09a41db76649c3632d85fb7", + "libflux/include/influxdata/flux.h": "c97e8a157a7867f5fd14053b45f3668e3ba26df38cae786f7631e8d13ec2eeb8", "stdlib/array/array.flux": "d0737bb4ee1cea99eb82d1fd6150f5686edd58d1c36a676593e623933dbcc424", "stdlib/array/array_test.flux": "c4281ef1128ba6164c16f1dcb1b051d2231fe39617f23a070ae1942b5ddb70d5", "stdlib/array/from_test.flux": "688a3593d27aed7110d016c0b7634808dee533311d4349669a1104bc50a9f3e7", diff --git a/libflux/include/influxdata/flux.h b/libflux/include/influxdata/flux.h index e98786495b..5d10210ac9 100644 --- a/libflux/include/influxdata/flux.h +++ b/libflux/include/influxdata/flux.h @@ -105,17 +105,6 @@ void flux_free_stateful_analyzer(struct flux_stateful_analyzer_t *); // a semantic graph for that snippet. struct flux_error_t *flux_analyze_with(struct flux_stateful_analyzer_t *, const char * src, struct flux_ast_pkg_t *, struct flux_semantic_pkg_t **); -// flux_analyze analyzes the given AST and will populate the second pointer argument with -// a pointer to the resulting semantic graph. -// It is the caller's responsibility to free the resulting semantic graph with a call to flux_free_semantic_pkg(). -// Note that the caller should free the pointer to the semantic graph, not the pointer to the pointer -// to the semantic graph. It is the former that references memory allocated by Rust. -// If analysis fails, the second pointer argument wil be pointed at 0, and an error will be returned. -// Any non-null error must be freed by calling flux_free_error. -// Regardless of whether an error is returned, this function will consume and free its -// flux_ast_pkg_t* argument, so it should not be reused after calling this function. -struct flux_error_t *flux_analyze(struct flux_ast_pkg_t *, const char * options, struct flux_semantic_pkg_t **); - // Find out the type of a variable referenced in the given Flux AST and return a MonoType flat buffer for it. // The Flux AST should not contain any definition for the referenced variable. // A type variable for the designated variable will be automatically injected into the type environment