Bug: KCL formatter removes 'fn' from closures: (#4718)

# Problem

Before this PR, our formatter reformats
```
squares_out = reduce(arr, 0, fn (i, squares)  {
  return 1
})
```
to 
```
squares_out = reduce(arr, 0, (i, squares) {
  return 1
})
```
i.e. it removes the `fn` keyword from the closure. This keyword is required, so, our formatter turned working code into invalid code.

# Cause

When this closure parameter is formatted, the ExprContext is ::Decl, so `Expr::recast` skips adding the `fn` keyword. The reason it's ::Decl is because the `squares_out = ` declaration sets it, and no subsequent call sets the context to something else.

# Solution

When recasting a call expression, set the context for every argument to `ExprContext::Other`.
This commit is contained in:
Adam Chalmers
2024-12-09 19:13:49 -06:00
committed by GitHub
parent 59a6333aad
commit 6aa588f09f
4 changed files with 37 additions and 12 deletions

View File

@ -45,7 +45,7 @@ circles = map([1..3], drawCircle)
```js ```js
r = 10 // radius r = 10 // radius
// Call `map`, using an anonymous function instead of a named one. // Call `map`, using an anonymous function instead of a named one.
circles = map([1..3], (id) { circles = map([1..3], fn(id) {
return startSketchOn("XY") return startSketchOn("XY")
|> circle({ center = [id * 2 * r, 0], radius = r }, %) |> circle({ center = [id * 2 * r, 0], radius = r }, %)
}) })

View File

@ -61,7 +61,7 @@ assertEqual(sum([1, 2, 3]), 6, 0.00001, "1 + 2 + 3 summed is 6")
// an anonymous `add` function as its parameter, instead of declaring a // an anonymous `add` function as its parameter, instead of declaring a
// named function outside. // named function outside.
arr = [1, 2, 3] arr = [1, 2, 3]
sum = reduce(arr, 0, (i, result_so_far) { sum = reduce(arr, 0, fn(i, result_so_far) {
return i + result_so_far return i + result_so_far
}) })
@ -84,7 +84,7 @@ fn decagon(radius) {
// Use a `reduce` to draw the remaining decagon sides. // Use a `reduce` to draw the remaining decagon sides.
// For each number in the array 1..10, run the given function, // For each number in the array 1..10, run the given function,
// which takes a partially-sketched decagon and adds one more edge to it. // which takes a partially-sketched decagon and adds one more edge to it.
fullDecagon = reduce([1..10], startOfDecagonSketch, (i, partialDecagon) { fullDecagon = reduce([1..10], startOfDecagonSketch, fn(i, partialDecagon) {
// Draw one edge of the decagon. // Draw one edge of the decagon.
x = cos(stepAngle * i) * radius x = cos(stepAngle * i) * radius
y = sin(stepAngle * i) * radius y = sin(stepAngle * i) * radius

View File

@ -100436,7 +100436,7 @@
"deprecated": false, "deprecated": false,
"examples": [ "examples": [
"r = 10 // radius\nfn drawCircle(id) {\n return startSketchOn(\"XY\")\n |> circle({ center = [id * 2 * r, 0], radius = r }, %)\n}\n\n// Call `drawCircle`, passing in each element of the array.\n// The outputs from each `drawCircle` form a new array,\n// which is the return value from `map`.\ncircles = map([1..3], drawCircle)", "r = 10 // radius\nfn drawCircle(id) {\n return startSketchOn(\"XY\")\n |> circle({ center = [id * 2 * r, 0], radius = r }, %)\n}\n\n// Call `drawCircle`, passing in each element of the array.\n// The outputs from each `drawCircle` form a new array,\n// which is the return value from `map`.\ncircles = map([1..3], drawCircle)",
"r = 10 // radius\n// Call `map`, using an anonymous function instead of a named one.\ncircles = map([1..3], (id) {\n return startSketchOn(\"XY\")\n |> circle({ center = [id * 2 * r, 0], radius = r }, %)\n})" "r = 10 // radius\n// Call `map`, using an anonymous function instead of a named one.\ncircles = map([1..3], fn(id) {\n return startSketchOn(\"XY\")\n |> circle({ center = [id * 2 * r, 0], radius = r }, %)\n})"
] ]
}, },
{ {
@ -146129,8 +146129,8 @@
"deprecated": false, "deprecated": false,
"examples": [ "examples": [
"// This function adds two numbers.\nfn add(a, b) {\n return a + b\n}\n\n// This function adds an array of numbers.\n// It uses the `reduce` function, to call the `add` function on every\n// element of the `arr` parameter. The starting value is 0.\nfn sum(arr) {\n return reduce(arr, 0, add)\n}\n\n/* The above is basically like this pseudo-code:\nfn sum(arr):\n let sumSoFar = 0\n for i in arr:\n sumSoFar = add(sumSoFar, i)\n return sumSoFar */\n\n\n// We use `assertEqual` to check that our `sum` function gives the\n// expected result. It's good to check your work!\nassertEqual(sum([1, 2, 3]), 6, 0.00001, \"1 + 2 + 3 summed is 6\")", "// This function adds two numbers.\nfn add(a, b) {\n return a + b\n}\n\n// This function adds an array of numbers.\n// It uses the `reduce` function, to call the `add` function on every\n// element of the `arr` parameter. The starting value is 0.\nfn sum(arr) {\n return reduce(arr, 0, add)\n}\n\n/* The above is basically like this pseudo-code:\nfn sum(arr):\n let sumSoFar = 0\n for i in arr:\n sumSoFar = add(sumSoFar, i)\n return sumSoFar */\n\n\n// We use `assertEqual` to check that our `sum` function gives the\n// expected result. It's good to check your work!\nassertEqual(sum([1, 2, 3]), 6, 0.00001, \"1 + 2 + 3 summed is 6\")",
"// This example works just like the previous example above, but it uses\n// an anonymous `add` function as its parameter, instead of declaring a\n// named function outside.\narr = [1, 2, 3]\nsum = reduce(arr, 0, (i, result_so_far) {\n return i + result_so_far\n})\n\n// We use `assertEqual` to check that our `sum` function gives the\n// expected result. It's good to check your work!\nassertEqual(sum, 6, 0.00001, \"1 + 2 + 3 summed is 6\")", "// This example works just like the previous example above, but it uses\n// an anonymous `add` function as its parameter, instead of declaring a\n// named function outside.\narr = [1, 2, 3]\nsum = reduce(arr, 0, fn(i, result_so_far) {\n return i + result_so_far\n})\n\n// We use `assertEqual` to check that our `sum` function gives the\n// expected result. It's good to check your work!\nassertEqual(sum, 6, 0.00001, \"1 + 2 + 3 summed is 6\")",
"// Declare a function that sketches a decagon.\nfn decagon(radius) {\n // Each side of the decagon is turned this many degrees from the previous angle.\n stepAngle = 1 / 10 * tau()\n\n // Start the decagon sketch at this point.\n startOfDecagonSketch = startSketchAt([cos(0) * radius, sin(0) * radius])\n\n // Use a `reduce` to draw the remaining decagon sides.\n // For each number in the array 1..10, run the given function,\n // which takes a partially-sketched decagon and adds one more edge to it.\n fullDecagon = reduce([1..10], startOfDecagonSketch, (i, partialDecagon) {\n // Draw one edge of the decagon.\n x = cos(stepAngle * i) * radius\n y = sin(stepAngle * i) * radius\n return lineTo([x, y], partialDecagon)\n })\n\n return fullDecagon\n}\n\n/* The `decagon` above is basically like this pseudo-code:\nfn decagon(radius):\n let stepAngle = (1/10) * tau()\n let startOfDecagonSketch = startSketchAt([(cos(0)*radius), (sin(0) * radius)])\n\n // Here's the reduce part.\n let partialDecagon = startOfDecagonSketch\n for i in [1..10]:\n let x = cos(stepAngle * i) * radius\n let y = sin(stepAngle * i) * radius\n partialDecagon = lineTo([x, y], partialDecagon)\n fullDecagon = partialDecagon // it's now full\n return fullDecagon */\n\n\n// Use the `decagon` function declared above, to sketch a decagon with radius 5.\ndecagon(5.0)\n |> close(%)" "// Declare a function that sketches a decagon.\nfn decagon(radius) {\n // Each side of the decagon is turned this many degrees from the previous angle.\n stepAngle = 1 / 10 * tau()\n\n // Start the decagon sketch at this point.\n startOfDecagonSketch = startSketchAt([cos(0) * radius, sin(0) * radius])\n\n // Use a `reduce` to draw the remaining decagon sides.\n // For each number in the array 1..10, run the given function,\n // which takes a partially-sketched decagon and adds one more edge to it.\n fullDecagon = reduce([1..10], startOfDecagonSketch, fn(i, partialDecagon) {\n // Draw one edge of the decagon.\n x = cos(stepAngle * i) * radius\n y = sin(stepAngle * i) * radius\n return lineTo([x, y], partialDecagon)\n })\n\n return fullDecagon\n}\n\n/* The `decagon` above is basically like this pseudo-code:\nfn decagon(radius):\n let stepAngle = (1/10) * tau()\n let startOfDecagonSketch = startSketchAt([(cos(0)*radius), (sin(0) * radius)])\n\n // Here's the reduce part.\n let partialDecagon = startOfDecagonSketch\n for i in [1..10]:\n let x = cos(stepAngle * i) * radius\n let y = sin(stepAngle * i) * radius\n partialDecagon = lineTo([x, y], partialDecagon)\n fullDecagon = partialDecagon // it's now full\n return fullDecagon */\n\n\n// Use the `decagon` function declared above, to sketch a decagon with radius 5.\ndecagon(5.0)\n |> close(%)"
] ]
}, },
{ {

View File

@ -166,7 +166,14 @@ pub(crate) enum ExprContext {
} }
impl Expr { impl Expr {
pub(crate) fn recast(&self, options: &FormatOptions, indentation_level: usize, ctxt: ExprContext) -> String { pub(crate) fn recast(&self, options: &FormatOptions, indentation_level: usize, mut ctxt: ExprContext) -> String {
let is_decl = matches!(ctxt, ExprContext::Decl);
if is_decl {
// Just because this expression is being bound to a variable, doesn't mean that every child
// expression is being bound. So, reset the expression context if necessary.
// This will still preserve the "::Pipe" context though.
ctxt = ExprContext::Other;
}
match &self { match &self {
Expr::BinaryExpression(bin_exp) => bin_exp.recast(options), Expr::BinaryExpression(bin_exp) => bin_exp.recast(options),
Expr::ArrayExpression(array_exp) => array_exp.recast(options, indentation_level, ctxt), Expr::ArrayExpression(array_exp) => array_exp.recast(options, indentation_level, ctxt),
@ -175,11 +182,7 @@ impl Expr {
Expr::MemberExpression(mem_exp) => mem_exp.recast(), Expr::MemberExpression(mem_exp) => mem_exp.recast(),
Expr::Literal(literal) => literal.recast(), Expr::Literal(literal) => literal.recast(),
Expr::FunctionExpression(func_exp) => { Expr::FunctionExpression(func_exp) => {
let mut result = if ctxt == ExprContext::Decl { let mut result = if is_decl { String::new() } else { "fn".to_owned() };
String::new()
} else {
"fn".to_owned()
};
result += &func_exp.recast(options, indentation_level); result += &func_exp.recast(options, indentation_level);
result result
} }
@ -2170,6 +2173,28 @@ sketch002 = startSketchOn({
assert_eq!(actual, expected); assert_eq!(actual, expected);
} }
#[test]
fn unparse_fn_unnamed() {
let input = r#"squares_out = reduce(arr, 0, fn(i, squares) {
return 1
})
"#;
let ast = crate::parsing::top_level_parse(input).unwrap();
let actual = ast.recast(&FormatOptions::new(), 0);
assert_eq!(actual, input);
}
#[test]
fn unparse_fn_named() {
let input = r#"fn f(x) {
return 1
}
"#;
let ast = crate::parsing::top_level_parse(input).unwrap();
let actual = ast.recast(&FormatOptions::new(), 0);
assert_eq!(actual, input);
}
#[test] #[test]
fn recast_objects_with_comments() { fn recast_objects_with_comments() {
use winnow::Parser; use winnow::Parser;