From 0f2f13b6927f8402a4328521f7a35948ac190568 Mon Sep 17 00:00:00 2001
From: David Wilson <daviwil@microsoft.com>
Date: Mon, 25 Sep 2023 18:06:08 +0300
Subject: [PATCH] `tsp format` should return non-zero exit code on failures
 (#2451)

This PR fixes #2323 by returning a non-zero exit code upon any failure
when `tsp format` is executed at the command line. I'm also changing the
return type of `formatTypeSpecFiles` to return a list of file paths that
failed, not sure whether that explicitly qualifies as a breaking change.
If needed, I can create a new function to expose and wrap it with the
original function without changing its signature.
---
 ...fix-format-exit-code_2023-09-20-15-44.json | 10 +++++++
 .../compiler/src/core/cli/actions/format.ts   |  7 ++++-
 packages/compiler/src/core/formatter-fs.ts    | 26 ++++++++++++++++---
 packages/compiler/src/core/messages.ts        | 10 +++++++
 4 files changed, 49 insertions(+), 4 deletions(-)
 create mode 100644 common/changes/@typespec/compiler/fix-format-exit-code_2023-09-20-15-44.json

diff --git a/common/changes/@typespec/compiler/fix-format-exit-code_2023-09-20-15-44.json b/common/changes/@typespec/compiler/fix-format-exit-code_2023-09-20-15-44.json
new file mode 100644
index 0000000000..93a0a7768e
--- /dev/null
+++ b/common/changes/@typespec/compiler/fix-format-exit-code_2023-09-20-15-44.json
@@ -0,0 +1,10 @@
+{
+  "changes": [
+    {
+      "packageName": "@typespec/compiler",
+      "comment": "`tsp format` now returns a non-zero exit code when it fails to format a file",
+      "type": "none"
+    }
+  ],
+  "packageName": "@typespec/compiler"
+}
\ No newline at end of file
diff --git a/packages/compiler/src/core/cli/actions/format.ts b/packages/compiler/src/core/cli/actions/format.ts
index d1bc11f327..c2d6dbabd3 100644
--- a/packages/compiler/src/core/cli/actions/format.ts
+++ b/packages/compiler/src/core/cli/actions/format.ts
@@ -1,4 +1,5 @@
 import { findUnformattedTypeSpecFiles, formatTypeSpecFiles } from "../../formatter-fs.js";
+import { logDiagnostics } from "../../index.js";
 import { CliCompilerHost } from "../types.js";
 
 export interface FormatArgs {
@@ -23,9 +24,13 @@ export async function formatAction(host: CliCompilerHost, args: FormatArgs) {
       process.exit(1);
     }
   } else {
-    await formatTypeSpecFiles(args["include"], {
+    const [_, diagnostics] = await formatTypeSpecFiles(args["include"], {
       exclude: args["exclude"],
       debug: args.debug,
     });
+    if (diagnostics.length > 0) {
+      logDiagnostics(diagnostics, host.logSink);
+      process.exit(1);
+    }
   }
 }
diff --git a/packages/compiler/src/core/formatter-fs.ts b/packages/compiler/src/core/formatter-fs.ts
index 07c58aa797..30a674f0bb 100644
--- a/packages/compiler/src/core/formatter-fs.ts
+++ b/packages/compiler/src/core/formatter-fs.ts
@@ -3,6 +3,8 @@ import { globby } from "globby";
 import { resolveConfig } from "prettier";
 import { PrettierParserError } from "../formatter/parser.js";
 import { checkFormatTypeSpec, formatTypeSpec } from "./formatter.js";
+import { Diagnostic, NoTarget } from "./index.js";
+import { createDiagnostic } from "./messages.js";
 import { normalizePath } from "./path-utils.js";
 
 export interface TypeSpecFormatOptions {
@@ -10,28 +12,46 @@ export interface TypeSpecFormatOptions {
   debug?: boolean;
 }
 
+export interface TypeSpecFormatResult {
+  /**
+   * The list of files which were formatted successfully, the paths of which are either relative or absolute based on the original file path patterns.
+   */
+  formattedFiles: string[];
+}
+
 /**
  * Format all the TypeSpec files.
  * @param patterns List of wildcard pattern searching for TypeSpec files.
+ * @returns list of files which failed to format.
  */
 export async function formatTypeSpecFiles(
   patterns: string[],
   { exclude, debug }: TypeSpecFormatOptions
-) {
+): Promise<[TypeSpecFormatResult, readonly Diagnostic[]]> {
   const files = await findFiles(patterns, exclude);
+  const diagnostics: Diagnostic[] = [];
+  const formattedFiles: string[] = [];
   for (const file of files) {
     try {
       await formatTypeSpecFile(file);
+      formattedFiles.push(file);
     } catch (e) {
       if (e instanceof PrettierParserError) {
         const details = debug ? e.message : "";
-        // eslint-disable-next-line no-console
-        console.error(`File '${file}' failed to format. ${details}`);
+        diagnostics.push(
+          createDiagnostic({
+            code: "format-failed",
+            format: { file, details },
+            target: NoTarget,
+          })
+        );
       } else {
         throw e;
       }
     }
   }
+
+  return [{ formattedFiles }, diagnostics];
 }
 
 /**
diff --git a/packages/compiler/src/core/messages.ts b/packages/compiler/src/core/messages.ts
index f4840bf739..1603e7797d 100644
--- a/packages/compiler/src/core/messages.ts
+++ b/packages/compiler/src/core/messages.ts
@@ -652,6 +652,16 @@ const diagnostics = {
     },
   },
 
+  /**
+   * Formatter
+   */
+  "format-failed": {
+    severity: "error",
+    messages: {
+      default: paramMessage`File '${"file"}' failed to format. ${"details"}`,
+    },
+  },
+
   /**
    * Decorator
    */