Report errors on node id when possible (#2296)

fix #2084

When a diagnostic target a type/node instead of reporting the error on
the whole type spamming the IDE or terminal with a large warning/error
we only highlight that type id instead.

This is the behavior we can see in typescript/eslint. When the
error/warning is meant to target an interface/function, etc. it will
only highlight the name when able.

common/changes/@typespec/compiler/show-node-id-diagnostic_2023-08-14-18-50.json

@@ -0,0 +1,10 @@
+{
+  "changes": [
+    {
+      "packageName": "@typespec/compiler",
+      "comment": "Diagnostics reported on nodes with an `id` will see the diagnostic highlight the id instead of the whole node. For example it will show the error on the model name instead of highlighting the entire model.",
+      "type": "none"
+    }
+  ],
+  "packageName": "@typespec/compiler"
+}

common/changes/@typespec/protobuf/show-node-id-diagnostic_2023-08-14-19-16.json

@@ -0,0 +1,10 @@
+{
+  "changes": [
+    {
+      "packageName": "@typespec/protobuf",
+      "comment": "",
+      "type": "none"
+    }
+  ],
+  "packageName": "@typespec/protobuf"
+}

packages/compiler/src/core/diagnostics.ts

@@ -106,7 +106,7 @@ export function logDiagnostics(diagnostics: readonly Diagnostic[], logger: LogSi
       level: diagnostic.severity,
       message: diagnostic.message,
       code: diagnostic.code,
-      sourceLocation: getSourceLocation(diagnostic.target),
+      sourceLocation: getSourceLocation(diagnostic.target, { locateId: true }),
     });
   }
 }
@@ -117,7 +117,7 @@ export function formatDiagnostic(diagnostic: Diagnostic) {
       code: diagnostic.code,
       level: diagnostic.severity,
       message: diagnostic.message,
-      sourceLocation: getSourceLocation(diagnostic.target),
+      sourceLocation: getSourceLocation(diagnostic.target, { locateId: true }),
     },
     { pretty: false }
   );
@@ -159,13 +159,28 @@ export function createSourceFile(text: string, path: string): SourceFile {
   }
 }
 
-export function getSourceLocation(target: DiagnosticTarget): SourceLocation;
+export interface SourceLocationOptions {
-export function getSourceLocation(target: typeof NoTarget | undefined): undefined;
+  /**
+   * If trying to resolve the location of a type with an ID, show the location of the ID node instead of the entire type.
+   * This makes sure that the location range is not too large and hard to read.
+   */
+  locateId?: boolean;
+}
 export function getSourceLocation(
-  target: DiagnosticTarget | typeof NoTarget | undefined
+  target: DiagnosticTarget,
+  options?: SourceLocationOptions
+): SourceLocation;
+export function getSourceLocation(
+  target: typeof NoTarget | undefined,
+  options?: SourceLocationOptions
+): undefined;
+export function getSourceLocation(
+  target: DiagnosticTarget | typeof NoTarget | undefined,
+  options?: SourceLocationOptions
 ): SourceLocation | undefined;
 export function getSourceLocation(
-  target: DiagnosticTarget | typeof NoTarget | undefined
+  target: DiagnosticTarget | typeof NoTarget | undefined,
+  options: SourceLocationOptions = {}
 ): SourceLocation | undefined {
   if (target === NoTarget || target === undefined) {
     return undefined;
@@ -185,16 +200,16 @@ export function getSourceLocation(
       return createSyntheticSourceLocation();
     }
 
-    return getSourceLocationOfNode(target.declarations[0]);
+    return getSourceLocationOfNode(target.declarations[0], options);
   } else if (typeof target.kind === "number") {
     // node
-    return getSourceLocationOfNode(target as Node);
+    return getSourceLocationOfNode(target as Node, options);
   } else {
     // type
     const targetNode = (target as Type).node;
 
     if (targetNode) {
-      return getSourceLocationOfNode(targetNode);
+      return getSourceLocationOfNode(targetNode, options);
     }
 
     return createSyntheticSourceLocation();
@@ -210,7 +225,7 @@ function createSyntheticSourceLocation(loc = "<unknown location>") {
   };
 }
 
-function getSourceLocationOfNode(node: Node): SourceLocation {
+function getSourceLocationOfNode(node: Node, options: SourceLocationOptions): SourceLocation {
   let root = node;
 
   while (root.parent !== undefined) {
@@ -225,6 +240,10 @@ function getSourceLocationOfNode(node: Node): SourceLocation {
     );
   }
 
+  if (options.locateId && "id" in node && node.id !== undefined) {
+    node = node.id;
+  }
+
   return {
     file: root.file,
     pos: node.pos,

packages/compiler/src/core/logger/logger.ts

@@ -38,6 +38,6 @@ function processLog(log: LogInfo): ProcessedLog {
     level: log.level,
     code: log.code,
     message: log.message,
-    sourceLocation: getSourceLocation(log.target),
+    sourceLocation: getSourceLocation(log.target, { locateId: true }),
   };
 }

packages/compiler/src/server/serverlib.ts

@@ -577,7 +577,7 @@ export function createServer(host: ServerHost): Server {
     for (const each of program.diagnostics) {
       let document: TextDocument | undefined;
 
-      const location = getSourceLocation(each.target);
+      const location = getSourceLocation(each.target, { locateId: true });
       if (location?.file) {
         document = (location.file as ServerSourceFile).document;
       } else {

packages/compiler/test/core/diagnostics.test.ts

@@ -0,0 +1,45 @@
+import { strictEqual } from "assert";
+import { SourceLocationOptions, getSourceLocation } from "../../src/index.js";
+import { createTestRunner } from "../../src/testing/test-host.js";
+import { extractSquiggles } from "../../src/testing/test-server-host.js";
+import { BasicTestRunner } from "../../src/testing/types.js";
+
+describe("compiler: diagnostics", () => {
+  let runner: BasicTestRunner;
+
+  beforeEach(async () => {
+    runner = await createTestRunner();
+  });
+
+  async function expectLocationMatch(code: string, options: SourceLocationOptions = {}) {
+    const { pos, end, source } = extractSquiggles(code);
+    const { target } = await runner.compile(source);
+    const location = getSourceLocation(target, options);
+    strictEqual(location.pos, pos);
+    strictEqual(location.end, end);
+  }
+
+  describe("getSourceLocation", () => {
+    it("report whole model by default", () =>
+      expectLocationMatch(`
+      ~~~@doc("This is documentation")
+      @test("target")
+      model Foo {
+        name: string;
+      }~~~
+    
+    `));
+    it("report report only model id if `locateId: true`", () =>
+      expectLocationMatch(
+        `
+      @doc("This is documentation")
+      @test("target")
+      model ~~~Foo~~~ {
+        name: string;
+      }
+    
+    `,
+        { locateId: true }
+      ));
+  });
+});

packages/protobuf/test/scenarios/enum-nonintegral/diagnostics.txt

@@ -1,4 +1,4 @@
-/test/main.tsp:18:1 - error @typespec/protobuf/unconvertible-enum: enums must explicitly assign exactly one integer to each member to be used in a Protobuf message
+/test/main.tsp:18:6 - error @typespec/protobuf/unconvertible-enum: enums must explicitly assign exactly one integer to each member to be used in a Protobuf message
 /test/main.tsp:19:3 - error @typespec/protobuf/unconvertible-enum: the first variant of an enum must be set to zero to be used in a Protobuf message
-/test/main.tsp:23:1 - error @typespec/protobuf/unconvertible-enum: enums must explicitly assign exactly one integer to each member to be used in a Protobuf message
+/test/main.tsp:23:6 - error @typespec/protobuf/unconvertible-enum: enums must explicitly assign exactly one integer to each member to be used in a Protobuf message
 /test/main.tsp:24:3 - error @typespec/protobuf/unconvertible-enum: the first variant of an enum must be set to zero to be used in a Protobuf message

packages/protobuf/test/scenarios/simple-error/diagnostics.txt

@@ -1,6 +1,6 @@
-/test/main.tsp:12:5 - error @typespec/protobuf/field-index: field index 0 is invalid (must be an integer greater than zero)
+/test/main.tsp:12:15 - error @typespec/protobuf/field-index: field index 0 is invalid (must be an integer greater than zero)
-/test/main.tsp:13:5 - error @typespec/protobuf/field-index: field index 536870912 is out of bounds (must be less than 536870912)
+/test/main.tsp:13:23 - error @typespec/protobuf/field-index: field index 536870912 is out of bounds (must be less than 536870912)
-/test/main.tsp:14:5 - error @typespec/protobuf/field-index: field index 19000 falls within the implementation-reserved range of 19000-19999 inclusive
+/test/main.tsp:14:19 - error @typespec/protobuf/field-index: field index 19000 falls within the implementation-reserved range of 19000-19999 inclusive
-/test/main.tsp:15:5 - error @typespec/protobuf/field-index: field index 19123 falls within the implementation-reserved range of 19000-19999 inclusive
+/test/main.tsp:15:19 - error @typespec/protobuf/field-index: field index 19123 falls within the implementation-reserved range of 19000-19999 inclusive
-/test/main.tsp:16:5 - error @typespec/protobuf/field-index: field index 19999 falls within the implementation-reserved range of 19000-19999 inclusive
+/test/main.tsp:16:19 - error @typespec/protobuf/field-index: field index 19999 falls within the implementation-reserved range of 19000-19999 inclusive
 /test/main.tsp:21:1 - error decorator-wrong-target: Cannot apply @message decorator to Test.Test since it is not assignable to {}

packages/protobuf/test/scenarios/union/diagnostics.txt

@@ -1 +1 @@
-/test/main.tsp:14:3 - error @typespec/protobuf/unsupported-field-type: a message field's type may not be a union
+/test/main.tsp:14:13 - error @typespec/protobuf/unsupported-field-type: a message field's type may not be a union