From bb3d2df9b4cdecee09371652d107e2f2ed8aa947 Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Fri, 18 Aug 2023 12:42:11 -0700 Subject: [PATCH] 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. --- ...w-node-id-diagnostic_2023-08-14-18-50.json | 10 +++++ ...w-node-id-diagnostic_2023-08-14-19-16.json | 10 +++++ packages/compiler/src/core/diagnostics.ts | 39 +++++++++++----- packages/compiler/src/core/logger/logger.ts | 2 +- packages/compiler/src/server/serverlib.ts | 2 +- .../compiler/test/core/diagnostics.test.ts | 45 +++++++++++++++++++ .../enum-nonintegral/diagnostics.txt | 4 +- .../scenarios/simple-error/diagnostics.txt | 10 ++--- .../test/scenarios/union/diagnostics.txt | 2 +- 9 files changed, 104 insertions(+), 20 deletions(-) create mode 100644 common/changes/@typespec/compiler/show-node-id-diagnostic_2023-08-14-18-50.json create mode 100644 common/changes/@typespec/protobuf/show-node-id-diagnostic_2023-08-14-19-16.json create mode 100644 packages/compiler/test/core/diagnostics.test.ts diff --git a/common/changes/@typespec/compiler/show-node-id-diagnostic_2023-08-14-18-50.json b/common/changes/@typespec/compiler/show-node-id-diagnostic_2023-08-14-18-50.json new file mode 100644 index 000000000..c27025f2c --- /dev/null +++ b/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" +} \ No newline at end of file diff --git a/common/changes/@typespec/protobuf/show-node-id-diagnostic_2023-08-14-19-16.json b/common/changes/@typespec/protobuf/show-node-id-diagnostic_2023-08-14-19-16.json new file mode 100644 index 000000000..8f48e4e95 --- /dev/null +++ b/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" +} \ No newline at end of file diff --git a/packages/compiler/src/core/diagnostics.ts b/packages/compiler/src/core/diagnostics.ts index 6cfd429eb..609716ad0 100644 --- a/packages/compiler/src/core/diagnostics.ts +++ b/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 function getSourceLocation(target: typeof NoTarget | undefined): undefined; +export interface SourceLocationOptions { + /** + * 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 = "") { }; } -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, diff --git a/packages/compiler/src/core/logger/logger.ts b/packages/compiler/src/core/logger/logger.ts index 66d4dcb4c..65ed9437f 100644 --- a/packages/compiler/src/core/logger/logger.ts +++ b/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 }), }; } diff --git a/packages/compiler/src/server/serverlib.ts b/packages/compiler/src/server/serverlib.ts index a10ff34b8..36e2dbb4c 100644 --- a/packages/compiler/src/server/serverlib.ts +++ b/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 { diff --git a/packages/compiler/test/core/diagnostics.test.ts b/packages/compiler/test/core/diagnostics.test.ts new file mode 100644 index 000000000..5e0d3c079 --- /dev/null +++ b/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 } + )); + }); +}); diff --git a/packages/protobuf/test/scenarios/enum-nonintegral/diagnostics.txt b/packages/protobuf/test/scenarios/enum-nonintegral/diagnostics.txt index 327989959..01e9b389d 100644 --- a/packages/protobuf/test/scenarios/enum-nonintegral/diagnostics.txt +++ b/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 diff --git a/packages/protobuf/test/scenarios/simple-error/diagnostics.txt b/packages/protobuf/test/scenarios/simple-error/diagnostics.txt index 82fb428dd..d512addb1 100644 --- a/packages/protobuf/test/scenarios/simple-error/diagnostics.txt +++ b/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:13:5 - 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:15:5 - 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:12:15 - error @typespec/protobuf/field-index: field index 0 is invalid (must be an integer greater than zero) +/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:19 - error @typespec/protobuf/field-index: field index 19000 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: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 {} diff --git a/packages/protobuf/test/scenarios/union/diagnostics.txt b/packages/protobuf/test/scenarios/union/diagnostics.txt index d63834c57..a61784112 100644 --- a/packages/protobuf/test/scenarios/union/diagnostics.txt +++ b/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