Repository: denoland/deno
Pull request: fix(cli): handle edge cases around `export`s in doc tests and default export

This commit fixes issues with the pseudo test file generation logic, namely:

- `export`s declared in snippets
- auto import insertion for `default export`

## Case 1: `export`s declared in snippets

In the previous implementation, `export`s declared in snippets were moved to the top level of the module in the generated pseudo test file. This is required because `export` must be at the top level.

This becomes a problem if such a `export` has a body, containing a reference to a local variable. Suppose we extract this snippet from JSDoc:

```ts
const logger = createLogger("my-awesome-module");

export function sum(a: number, b: number): number {
  logger.debug("sum called");
  return a + b;
}
```

This gets converted into the following invalid code (note that `export function sum` is moved to the top level, but its body references `logger` variable which can't be referenced from here):

```ts
export function sum(a: number, b: number): number {
  logger.debug("sum called");
  return a + b;
}

Deno.test("./base.ts$1-7.ts", async () => {
  const logger = createLogger("my-awesome-module");
});
```

To resolve this issue, this commit adds a logic to remove the `export` keyword, allowing the exported items to stay in the `Deno.test` block scope, like so:

```ts
Deno.test("./base.ts$1-7.ts", async () => {
  const logger = createLogger("my-awesome-module");

  function sum(a: number, b: number): number {
    logger.debug("sum called");
    return a + b;
  }
});
```

## Case 2: default export

Previously `default export foo` was not captured by the export collector, so auto import insertion didn't work for this case. To put it concretely, the following code snippet didn't work when run with `deno test --doc` because `import foo from "file:///path/to/mod.ts"` didn't get inserted automatically:

```ts
/**
 * ```ts
 * console.log(foo);
 * ```
 * 
 * @module
 */

const foo = 42;
export default foo;
```

This commit fixes this issue and the above example works fine.

---

Fixes #25718


Changed files:
filename: cli/util/extract.rs
patch: @@ -358,7 +358,18 @@ impl Visit for ExportCollector {
           self.default_export = Some(ident.sym.clone());
         }
       }
-      ast::DefaultDecl::TsInterfaceDecl(_) => {}
+      ast::DefaultDecl::TsInterfaceDecl(iface_decl) => {
+        self.default_export = Some(iface_decl.id.sym.clone());
+      }
+    }
+  }
+
+  fn visit_export_default_expr(
+    &mut self,
+    export_default_expr: &ast::ExportDefaultExpr,
+  ) {
+    if let ast::Expr::Ident(ident) = &*export_default_expr.expr {
+      self.default_export = Some(ident.sym.clone());
     }
   }
 
@@ -472,7 +483,7 @@ fn extract_sym_from_pat(pat: &ast::Pat) -> Vec<Atom> {
 /// });
 /// ```
 ///
-/// # Edge case - duplicate identifier
+/// # Edge case 1 - duplicate identifier
 ///
 /// If a given file imports, say, `doSomething` from an external module while
 /// the base file exports `doSomething` as well, the generated pseudo test file
@@ -491,6 +502,52 @@ fn extract_sym_from_pat(pat: &ast::Pat) -> Vec<Atom> {
 ///   assertEquals(doSomething(1), 2);
 /// });
 /// ```
+///
+/// # Edge case 2 - exports can't be put inside `Deno.test` blocks
+///
+/// All exports like `export const foo = 42` must be at the top level of the
+/// module, making it impossible to wrap exports in `Deno.test` blocks. For
+/// example, when the following code snippet is provided:
+///
+/// ```ts
+/// const logger = createLogger("my-awesome-module");
+///
+/// export function sum(a: number, b: number): number {
+///   logger.debug("sum called");
+///   return a + b;
+/// }
+/// ```
+///
+/// If we applied the naive transformation to this, the generated pseudo test
+/// file would look like:
+///
+/// ```ts
+/// Deno.test("./base.ts$1-7.ts", async () => {
+///   const logger = createLogger("my-awesome-module");
+///
+///   export function sum(a: number, b: number): number {
+///     logger.debug("sum called");
+///     return a + b;
+///   }
+/// });
+/// ```
+///
+/// But obviously this violates the rule because `export function sum` is not
+/// at the top level of the module.
+///
+/// To address this issue, the `export` keyword is removed so that the item can
+/// stay in the `Deno.test` block's scope:
+///
+/// ```ts
+/// Deno.test("./base.ts$1-7.ts", async () => {
+///   const logger = createLogger("my-awesome-module");
+///
+///   function sum(a: number, b: number): number {
+///     logger.debug("sum called");
+///     return a + b;
+///   }
+/// });
+/// ```
 fn generate_pseudo_file(
   file: File,
   base_file_specifier: &ModuleSpecifier,
@@ -553,9 +610,51 @@ impl<'a> VisitMut for Transform<'a> {
 
         for item in &module.body {
           match item {
-            ast::ModuleItem::ModuleDecl(decl) => {
-              module_decls.push(decl.clone());
-            }
+            ast::ModuleItem::ModuleDecl(decl) => match self.wrap_kind {
+              WrapKind::NoWrap => {
+                module_decls.push(decl.clone());
+              }
+              // We remove `export` keywords so that they can be put inside
+              // `Deno.test` block scope.
+              WrapKind::DenoTest => match decl {
+                ast::ModuleDecl::ExportDecl(export_decl) => {
+                  stmts.push(ast::Stmt::Decl(export_decl.decl.clone()));
+                }
+                ast::ModuleDecl::ExportDefaultDecl(export_default_decl) => {
+                  let stmt = match &export_default_decl.decl {
+                    ast::DefaultDecl::Class(class) => {
+                      let expr = ast::Expr::Class(class.clone());
+                      ast::Stmt::Expr(ast::ExprStmt {
+                        span: DUMMY_SP,
+                        expr: Box::new(expr),
+                      })
+                    }
+                    ast::DefaultDecl::Fn(func) => {
+                      let expr = ast::Expr::Fn(func.clone());
+                      ast::Stmt::Expr(ast::ExprStmt {
+                        span: DUMMY_SP,
+                        expr: Box::new(expr),
+                      })
+                    }
+                    ast::DefaultDecl::TsInterfaceDecl(ts_interface_decl) => {
+                      ast::Stmt::Decl(ast::Decl::TsInterface(
+                        ts_interface_decl.clone(),
+                      ))
+                    }
+                  };
+                  stmts.push(stmt);
+                }
+                ast::ModuleDecl::ExportDefaultExpr(export_default_expr) => {
+                  stmts.push(ast::Stmt::Expr(ast::ExprStmt {
+                    span: DUMMY_SP,
+                    expr: export_default_expr.expr.clone(),
+                  }));
+                }
+                _ => {
+                  module_decls.push(decl.clone());
+                }
+              },
+            },
             ast::ModuleItem::Stmt(stmt) => {
               stmts.push(stmt.clone());
             }
@@ -880,6 +979,32 @@ Deno.test("file:///main.ts$13-16.ts", async ()=>{
           },
         ],
       },
+      Test {
+        input: Input {
+          source: r#"
+/**
+ * ```ts
+ * foo();
+ * ```
+ */
+export function foo() {}
+
+export const ONE = 1;
+const TWO = 2;
+export default TWO;
+"#,
+          specifier: "file:///main.ts",
+        },
+        expected: vec![Expected {
+          source: r#"import TWO, { ONE, foo } from "file:///main.ts";
+Deno.test("file:///main.ts$3-6.ts", async ()=>{
+    foo();
+});
+"#,
+          specifier: "file:///main.ts$3-6.ts",
+          media_type: MediaType::TypeScript,
+        }],
+      },
       // Avoid duplicate imports
       Test {
         input: Input {
@@ -945,30 +1070,43 @@ Deno.test("file:///main.ts$3-7.ts", async ()=>{
           media_type: MediaType::TypeScript,
         }],
       },
-      // example code has an exported item `foo` - because `export` must be at
-      // the top level, `foo` is "hoisted" to the top level instead of being
-      // wrapped in `Deno.test`.
+      // https://github.com/denoland/deno/issues/25718
+      // A case where the example code has an exported item which references
+      // a variable from one upper scope.
+      // Naive application of `Deno.test` wrap would cause a reference error
+      // because the variable would go inside the `Deno.test` block while the
+      // exported item would be moved to the top level. To suppress the auto
+      // move of the exported item to the top level, the `export` keyword is
+      // removed so that the item stays in the same scope as the variable.
       Test {
         input: Input {
           source: r#"
 /**
  * ```ts
- * doSomething();
- * export const foo = 42;
+ * import { getLogger } from "@std/log";
+ *
+ * const logger = getLogger("my-awesome-module");
+ *
+ * export function foo() {
+ *   logger.debug("hello");
+ * }
  * ```
+ *
+ * @module
  */
-export function doSomething() {}
 "#,
           specifier: "file:///main.ts",
         },
         expected: vec![Expected {
-          source: r#"export const foo = 42;
-import { doSomething } from "file:///main.ts";
-Deno.test("file:///main.ts$3-7.ts", async ()=>{
-    doSomething();
+          source: r#"import { getLogger } from "@std/log";
+Deno.test("file:///main.ts$3-12.ts", async ()=>{
+    const logger = getLogger("my-awesome-module");
+    function foo() {
+        logger.debug("hello");
+    }
 });
 "#,
-          specifier: "file:///main.ts$3-7.ts",
+          specifier: "file:///main.ts$3-12.ts",
           media_type: MediaType::TypeScript,
         }],
       },
@@ -1103,26 +1241,23 @@ assertEquals(add(1, 2), 3);
           media_type: MediaType::TypeScript,
         }],
       },
-      // duplication of imported identifier and local identifier is fine, since
-      // we wrap the snippet in a block.
-      // This would be a problem if the local one is declared with `var`, as
-      // `var` is not block scoped but function scoped. For now we don't handle
-      // this case assuming that `var` is not used in modern code.
+      // If the snippet has a local variable with the same name as an exported
+      // item, the local variable takes precedence.
       Test {
         input: Input {
           source: r#"
-        /**
-         * ```ts
-         * const foo = createFoo();
-         * foo();
-         * ```
-         */
-        export function createFoo() {
-          return () => "created foo";
-        }
+/**
+ * ```ts
+ * const foo = createFoo();
+ * foo();
+ * ```
+ */
+export function createFoo() {
+  return () => "created foo";
+}
 
-        export const foo = () => "foo";
-        "#,
+export const foo = () => "foo";
+"#,
           specifier: "file:///main.ts",
         },
         expected: vec![Expected {
@@ -1134,6 +1269,38 @@ foo();
           media_type: MediaType::TypeScript,
         }],
       },
+      // Unlike `extract_doc_tests`, `extract_snippet_files` does not remove
+      // the `export` keyword from the exported items.
+      Test {
+        input: Input {
+          source: r#"
+/**
+ * ```ts
+ * import { getLogger } from "@std/log";
+ *
+ * const logger = getLogger("my-awesome-module");
+ *
+ * export function foo() {
+ *   logger.debug("hello");
+ * }
+ * ```
+ *
+ * @module
+ */
+"#,
+          specifier: "file:///main.ts",
+        },
+        expected: vec![Expected {
+          source: r#"import { getLogger } from "@std/log";
+export function foo() {
+    logger.debug("hello");
+}
+const logger = getLogger("my-awesome-module");
+"#,
+          specifier: "file:///main.ts$3-12.ts",
+          media_type: MediaType::TypeScript,
+        }],
+      },
       Test {
         input: Input {
           source: r#"
@@ -1311,6 +1478,21 @@ assertEquals(add(1, 2), 3);
         named_expected: atom_set!(),
         default_expected: Some("foo".into()),
       },
+      Test {
+        input: r#"export default class Foo {}"#,
+        named_expected: atom_set!(),
+        default_expected: Some("Foo".into()),
+      },
+      Test {
+        input: r#"export default interface Foo {}"#,
+        named_expected: atom_set!(),
+        default_expected: Some("Foo".into()),
+      },
+      Test {
+        input: r#"const foo = 42; export default foo;"#,
+        named_expected: atom_set!(),
+        default_expected: Some("foo".into()),
+      },
       Test {
         input: r#"export { foo, bar as barAlias };"#,
         named_expected: atom_set!("foo", "barAlias"),