feat: Enable codespan formatting for errors via feature flags (#5204)

* feat: Enable codespan formatting for errors via feature flags

This improved error formatting was added back in #4270 but so far we have only used it for

* feat: Pass feature flags to flux_ast_get_error so we can get pretty errors

* feat: Enable prettyError by default on the command line

* chore: Update tests using Compile and Parse to pass a Context

* chore: Fix cffi test

* chore: make generate

cmd/flux/cmd/test.go

@@ -144,8 +144,12 @@ func runFluxTests(out io.Writer, setup TestSetupFunc, flags TestFlags) (bool, er
 	return runner.Finish(), nil
 }
 
+var defaultCmdFeatureFlags = executetest.TestFlagger{
+	"prettyError": true,
+}
+
 func WithFeatureFlags(ctx context.Context, features string) (context.Context, error) {
-	flagger := executetest.TestFlagger{}
+	flagger := defaultCmdFeatureFlags
 	if len(features) != 0 {
 		if err := json.Unmarshal([]byte(features), &flagger); err != nil {
 			return nil, errors.Newf(codes.Invalid, "Unable to unmarshal features as json: %s", err)

cmd/flux/fmt.go

@@ -1,11 +1,13 @@
 package main
 
 import (
+	"context"
 	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
 
+	fluxcmd "github.com/influxdata/flux/cmd/flux/cmd"
 	"github.com/influxdata/flux/libflux/go/libflux"
 	"github.com/spf13/cobra"
 )
@@ -16,17 +18,25 @@ var fmtFlags struct {
 }
 
 func formatFile(cmd *cobra.Command, args []string) error {
+
+	ctx := context.Background()
+
+	ctx, err := fluxcmd.WithFeatureFlags(ctx, flags.Features)
+	if err != nil {
+		return err
+	}
+
 	script := args[0]
 	var bad []string
-	err := filepath.Walk(script,
+	err = filepath.Walk(script,
 		func(path string, info os.FileInfo, err error) error {
 			if err != nil {
 				return err
 			}
 			if info.IsDir() || filepath.Ext(info.Name()) != ".flux" {
 				return nil
 			}
-			ok, err := format(path)
+			ok, err := format(ctx, path)
 			if err != nil {
 				return err
 			}
@@ -50,15 +60,15 @@ func formatFile(cmd *cobra.Command, args []string) error {
 	return nil
 }
 
-func format(script string) (bool, error) {
+func format(ctx context.Context, script string) (bool, error) {
 	fromFile, err := os.ReadFile(script)
 	if err != nil {
 		return false, err
 	}
 	curFileStr := string(fromFile)
 	ast := libflux.ParseString(curFileStr)
 	defer ast.Free()
-	if err := ast.GetError(); err != nil {
+	if err := ast.GetError(libflux.NewOptions(ctx)); err != nil {
 		return false, fmt.Errorf("parse error: %s, %s", script, err)
 
 	}

internal/feature/flags.yml

@@ -105,3 +105,9 @@
   key: strictNullLogicalOps
   default: false
   contact: Owen Nelson
+
+- name: Pretty Error
+  description: Enables formatting with codespan for errors
+  key: prettyError
+  default: false
+  contact: Markus Westerlind

internal/spec/build.go

@@ -195,7 +195,7 @@ func FromTableObject(ctx context.Context, to *flux.TableObject, now time.Time) (
 // This function is used in tests that compare flux.Specs (e.g. in planner tests).
 func FromScript(ctx context.Context, runtime flux.Runtime, now time.Time, script string) (*operation.Spec, error) {
 	s, _ := opentracing.StartSpanFromContext(ctx, "parse")
-	astPkg, err := runtime.Parse(script)
+	astPkg, err := runtime.Parse(ctx, script)
 	if err != nil {
 		return nil, err
 	}

lang/compiler.go

@@ -15,6 +15,7 @@ import (
 	"github.com/influxdata/flux/internal/operation"
 	"github.com/influxdata/flux/internal/spec"
 	"github.com/influxdata/flux/interpreter"
+	"github.com/influxdata/flux/libflux/go/libflux"
 	"github.com/influxdata/flux/memory"
 	"github.com/influxdata/flux/metadata"
 	"github.com/influxdata/flux/plan"
@@ -92,8 +93,8 @@ func applyOptions(opts ...CompileOption) *compileOptions {
 
 // Compile evaluates a Flux script producing a flux.Program.
 // now parameter must be non-zero, that is the default now time should be set before compiling.
-func Compile(q string, runtime flux.Runtime, now time.Time, opts ...CompileOption) (*AstProgram, error) {
-	astPkg, err := runtime.Parse(q)
+func Compile(ctx context.Context, q string, runtime flux.Runtime, now time.Time, opts ...CompileOption) (*AstProgram, error) {
+	astPkg, err := runtime.Parse(ctx, q)
 	if err != nil {
 		return nil, err
 	}
@@ -183,9 +184,9 @@ func (c FluxCompiler) Compile(ctx context.Context, runtime flux.Runtime) (flux.P
 		if err != nil {
 			return nil, errors.Wrap(err, codes.Inherit, "extern json parse error")
 		}
-		return Compile(query, runtime, c.Now, WithExtern(hdl))
+		return Compile(ctx, query, runtime, c.Now, WithExtern(hdl))
 	}
-	return Compile(query, runtime, c.Now)
+	return Compile(ctx, query, runtime, c.Now)
 }
 
 func (c FluxCompiler) CompilerType() flux.CompilerType {
@@ -208,7 +209,7 @@ func (c ASTCompiler) Compile(ctx context.Context, runtime flux.Runtime) (flux.Pr
 	if err != nil {
 		return nil, err
 	}
-	if err := hdl.GetError(); err != nil {
+	if err := hdl.GetError(libflux.NewOptions(ctx)); err != nil {
 		return nil, err
 	}
 

lang/compiler_test.go

@@ -209,15 +209,15 @@ func TestFluxCompiler(t *testing.T) {
 }
 
 func TestCompilationError(t *testing.T) {
-	program, err := lang.Compile(`illegal query`, runtime.Default, time.Unix(0, 0))
+	ctx, deps := dependency.Inject(context.Background(), executetest.NewTestExecuteDependencies())
+	defer deps.Finish()
+
+	program, err := lang.Compile(ctx, `illegal query`, runtime.Default, time.Unix(0, 0))
 	if err != nil {
 		// This shouldn't happen, has the script should be evaluated at program Start.
 		t.Fatal(err)
 	}
 
-	ctx, deps := dependency.Inject(context.Background(), executetest.NewTestExecuteDependencies())
-	defer deps.Finish()
-
 	_, err = program.Start(ctx, &memory.ResourceAllocator{})
 	if err == nil {
 		t.Fatal("compilation error expected, got none")
@@ -346,10 +346,13 @@ csv.from(csv: "foo,bar") |> range(start: 2017-10-10T00:00:00Z)
 	rt := runtime.Default
 	for _, tc := range testcases {
 		t.Run(tc.name, func(t *testing.T) {
+			ctx, deps := dependency.Inject(context.Background(), executetest.NewTestExecuteDependencies())
+			defer deps.Finish()
+
 			var c lang.ASTCompiler
 			{
 				if tc.script != "" {
-					astPkg, err := rt.Parse(tc.script)
+					astPkg, err := rt.Parse(ctx, tc.script)
 					if err != nil {
 						t.Fatalf("failed to parse script: %v", err)
 					}
@@ -407,8 +410,6 @@ csv.from(csv: "foo,bar") |> range(start: 2017-10-10T00:00:00Z)
 			if err != nil {
 				t.Fatalf("failed to compile AST: %v", err)
 			}
-			ctx, deps := dependency.Inject(context.Background(), executetest.NewTestExecuteDependencies())
-			defer deps.Finish()
 
 			// we need to start the program to get compile errors derived from AST evaluation
 			if _, err := program.Start(ctx, &memory.ResourceAllocator{}); err != nil {
@@ -487,6 +488,10 @@ csv.from(csv: "
 }
 
 func TestCompileOptions(t *testing.T) {
+	// start program in order to evaluate planner options
+	ctx, deps := dependency.Inject(context.Background(), executetest.NewTestExecuteDependencies())
+	defer deps.Finish()
+
 	src := `import "csv"
 			csv.from(csv: "foo,bar")
 				|> range(start: 2017-10-10T00:00:00Z)
@@ -496,15 +501,11 @@ func TestCompileOptions(t *testing.T) {
 
 	opt := lang.WithLogPlanOpts(plan.OnlyLogicalRules(removeCount{}))
 
-	program, err := lang.Compile(src, runtime.Default, now, opt)
+	program, err := lang.Compile(ctx, src, runtime.Default, now, opt)
 	if err != nil {
 		t.Fatalf("failed to compile script: %v", err)
 	}
 
-	// start program in order to evaluate planner options
-	ctx, deps := dependency.Inject(context.Background(), executetest.NewTestExecuteDependencies())
-	defer deps.Finish()
-
 	if _, err := program.Start(ctx, &memory.ResourceAllocator{}); err != nil {
 		t.Fatalf("failed to start program: %v", err)
 	}
@@ -783,14 +784,18 @@ option planner.disableLogicalRules = ["removeCountRule"]`},
 			if len(tc.files) == 0 {
 				t.Fatal("the test should have at least one file")
 			}
-			astPkg, err := runtime.Parse(tc.files[0])
+
+			ctx, deps := dependency.Inject(context.Background(), executetest.NewTestExecuteDependencies())
+			defer deps.Finish()
+
+			astPkg, err := runtime.Parse(ctx, tc.files[0])
 			if err != nil {
 				t.Fatal(err)
 			}
 
 			if len(tc.files) > 1 {
 				for _, file := range tc.files[1:] {
-					otherPkg, err := runtime.Parse(file)
+					otherPkg, err := runtime.Parse(ctx, file)
 					if err != nil {
 						t.Fatal(err)
 					}
@@ -801,8 +806,6 @@ option planner.disableLogicalRules = ["removeCountRule"]`},
 			}
 
 			program := lang.CompileAST(astPkg, runtime.Default, nowFn())
-			ctx, deps := dependency.Inject(context.Background(), executetest.NewTestExecuteDependencies())
-			defer deps.Finish()
 
 			if q, err := program.Start(ctx, &memory.ResourceAllocator{}); err != nil {
 				if tc.wantErr == "" {

lang/execopts_test.go

@@ -19,7 +19,12 @@ func TestExecutionOptions(t *testing.T) {
 		option now = () => ( 2020-10-15T00:00:00Z )
 	`
 
-	h, err := parser.ParseToHandle([]byte(src))
+	// Prepare a context with execution dependencies.
+	ctx := context.TODO()
+	deps := execute.DefaultExecutionDependencies()
+	ctx = deps.Inject(ctx)
+
+	h, err := parser.ParseToHandle(ctx, []byte(src))
 	if err != nil {
 		t.Fatalf("failed to parse test case: %v", err)
 	}
@@ -39,11 +44,6 @@ func TestExecutionOptions(t *testing.T) {
 		pkg.Range(scope.Set)
 	}
 
-	// Prepare a context with execution dependencies.
-	ctx := context.TODO()
-	deps := execute.DefaultExecutionDependencies()
-	ctx = deps.Inject(ctx)
-
 	// Pass lang.ExecutionOptions as the options configurator. It is
 	// responsible for installing the configured options into the execution
 	// dependencies. The goal of this test is to verify the option

lang/query_test.go

@@ -19,11 +19,12 @@ import (
 )
 
 func runQuery(ctx context.Context, script string) (flux.Query, func(), error) {
-	program, err := lang.Compile(script, runtime.Default, time.Unix(0, 0))
+	ctx, deps := dependency.Inject(ctx, executetest.NewTestExecuteDependencies())
+
+	program, err := lang.Compile(ctx, script, runtime.Default, time.Unix(0, 0))
 	if err != nil {
 		return nil, nil, err
 	}
-	ctx, deps := dependency.Inject(ctx, executetest.NewTestExecuteDependencies())
 	q, err := program.Start(ctx, memory.DefaultAllocator)
 	if err != nil {
 		deps.Finish()

libflux/c/main.c

@@ -25,7 +25,7 @@ void test_ast() {
     struct flux_ast_pkg_t *ast_pkg_foo = flux_parse("test", "package foo\nx = 1 + 1");
     assert(ast_pkg_foo !=  NULL);
 
-    struct flux_error_t* err = flux_ast_get_error(ast_pkg_foo);
+    struct flux_error_t* err = flux_ast_get_error(ast_pkg_foo, "");
     assert(err == NULL);
 
     printf("Marshaling to JSON\n");
@@ -43,7 +43,7 @@ void test_ast() {
     struct flux_ast_pkg_t *ast_pkg_foo = flux_parse("test", "x = 1 + / 1");
     assert(ast_pkg_foo !=  NULL);
 
-    struct flux_error_t* err = flux_ast_get_error(ast_pkg_foo);
+    struct flux_error_t* err = flux_ast_get_error(ast_pkg_foo, "");
     assert(err != NULL);
     const char* err_str = flux_error_str(err);
     printf("  error: %s\n", err_str);
@@ -56,7 +56,7 @@ void test_ast() {
     struct flux_ast_pkg_t *ast_pkg_foo = flux_parse("test", "package foo\nx=1+1");
     assert(ast_pkg_foo != NULL);
 
-    struct flux_error_t* err = flux_ast_get_error(ast_pkg_foo);
+    struct flux_error_t* err = flux_ast_get_error(ast_pkg_foo, "");
     assert(err == NULL);
 
     struct flux_buffer_t buf;

libflux/flux-core/src/ast/check/mod.rs

@@ -10,7 +10,11 @@ use crate::{
 
 /// Inspects an AST node and returns a list of found AST errors plus
 /// any errors existed before `ast.check()` is performed.
-pub fn check(node: walk::Node) -> Result<(), Errors<Error>> {
+pub fn check<'a>(node: impl Into<walk::Node<'a>>) -> Result<(), Errors<Error>> {
+    check_(node.into())
+}
+
+fn check_(node: walk::Node) -> Result<(), Errors<Error>> {
     const MAX_DEPTH: u32 = 180;
 
     #[derive(Default)]