Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Report diagnostics for unused "using" so that they will be shown properly in ide #5453

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
7 changes: 7 additions & 0 deletions .chronus/changes/unused-using-2024-11-26-19-12-40.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
changeKind: feature
packages:
- "@typespec/compiler"
---

Support diagnostics for unused using statement
14 changes: 14 additions & 0 deletions .chronus/changes/unused-using-2024-11-27-14-37-20.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
---
changeKind: internal
packages:
- "@typespec/http-specs"
- "@typespec/http"
- "@typespec/openapi"
- "@typespec/openapi3"
- "@typespec/rest"
- "@typespec/streams"
- "@typespec/xml"
- "@typespec/http-server-csharp"
---

Fix test for diagnostics of unused using statement
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import { defineCodeFix, getSourceLocation } from "../diagnostics.js";
import { type ImportStatementNode, type UsingStatementNode } from "../types.js";

/**
* Quick fix that remove unused code.
*/
export function removeUnusedCodeCodeFix(node: ImportStatementNode | UsingStatementNode) {
return defineCodeFix({
id: "remove-unused-code",
label: `Remove unused code`,
fix: (context) => {
const location = getSourceLocation(node);
return context.replaceText(location, "");
},
});
}
4 changes: 2 additions & 2 deletions packages/compiler/src/core/diagnostics.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ export type DiagnosticHandler = (diagnostic: Diagnostic) => void;
export function logDiagnostics(diagnostics: readonly Diagnostic[], logger: LogSink) {
for (const diagnostic of diagnostics) {
logger.log({
level: diagnostic.severity,
level: diagnostic.severity === "hint" ? "trace" : diagnostic.severity,
message: diagnostic.message,
code: diagnostic.code,
url: diagnostic.url,
Expand All @@ -52,7 +52,7 @@ export function formatDiagnostic(diagnostic: Diagnostic, options: FormatDiagnost
return formatLog(
{
code: diagnostic.code,
level: diagnostic.severity,
level: diagnostic.severity === "hint" ? "trace" : diagnostic.severity,
message: diagnostic.message,
url: diagnostic.url,
sourceLocation: getSourceLocation(diagnostic.target, { locateId: true }),
Expand Down
6 changes: 6 additions & 0 deletions packages/compiler/src/core/messages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -576,6 +576,12 @@ const diagnostics = {
default: "The #deprecated directive cannot be used more than once on the same declaration.",
},
},
"unused-using": {
severity: "hint",
messages: {
default: paramMessage`Unused using: ${"code"}`,
},
},

/**
* Configuration
Expand Down
49 changes: 48 additions & 1 deletion packages/compiler/src/core/name-resolver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@
import { Mutable, mutate } from "../utils/misc.js";
import { createSymbol, createSymbolTable, getSymNode } from "./binder.js";
import { compilerAssert } from "./diagnostics.js";
import { visitChildren } from "./parser.js";
import { getFirstAncestor, visitChildren } from "./parser.js";
import { Program } from "./program.js";
import {
AliasStatementNode,
Expand Down Expand Up @@ -94,6 +94,7 @@ import {
TypeReferenceNode,
TypeSpecScriptNode,
UnionStatementNode,
UsingStatementNode,
} from "./types.js";

export interface NameResolver {
Expand Down Expand Up @@ -142,6 +143,9 @@ export interface NameResolver {
node: TypeReferenceNode | IdentifierNode | MemberExpressionNode,
): ResolutionResult;

/** Get the using statement nodes which is not used in resolving yet */
getUnusedUsings(): UsingStatementNode[];

/** Built-in symbols. */
readonly symbols: {
/** Symbol for the global namespace */
Expand Down Expand Up @@ -175,6 +179,11 @@ export function createResolver(program: Program): NameResolver {
mutate(globalNamespaceNode).symbol = globalNamespaceSym;
mutate(globalNamespaceSym.exports).set(globalNamespaceNode.id.sv, globalNamespaceSym);

/**
* Tracking the symbols that are used through using.
*/
const usedUsingSym = new Map<TypeSpecScriptNode, Set<Sym>>();

const metaTypePrototypes = createMetaTypePrototypes();

const nullSym = createSymbol(undefined, "null", SymbolFlags.None);
Expand Down Expand Up @@ -222,8 +231,33 @@ export function createResolver(program: Program): NameResolver {
resolveTypeReference,

getAugmentDecoratorsForSym,
getUnusedUsings,
};

function getUnusedUsings(): UsingStatementNode[] {
const unusedUsings: Set<UsingStatementNode> = new Set();
for (const file of program.sourceFiles.values()) {
const lc = program.getSourceFileLocationContext(file.file);
if (lc.type === "project") {
const usedSym = usedUsingSym.get(file) ?? new Set<Sym>();
for (const using of file.usings) {
const table = getNodeLinks(using.name).resolvedSymbol;
let used = false;
for (const [_, sym] of table?.exports ?? new Map<string, Sym>()) {
if (usedSym.has(getMergedSymbol(sym))) {
used = true;
break;
}
}
if (used === false) {
unusedUsings.add(using);
}
}
}
}
return [...unusedUsings];
}

function getAugmentDecoratorsForSym(sym: Sym) {
return augmentDecoratorsForSym.get(sym) ?? [];
}
Expand Down Expand Up @@ -960,6 +994,15 @@ export function createResolver(program: Program): NameResolver {
if ("locals" in scope && scope.locals !== undefined) {
binding = tableLookup(scope.locals, node, options.resolveDecorators);
if (binding) {
if (binding.flags & SymbolFlags.Using && binding.symbolSource) {
const fileNode = getFirstAncestor(node, (n) => n.kind === SyntaxKind.TypeSpecScript) as
| TypeSpecScriptNode
| undefined;
if (fileNode) {
usedUsingSym.get(fileNode)?.add(binding.symbolSource) ??
usedUsingSym.set(fileNode, new Set([binding.symbolSource]));
}
}
return resolvedResult(binding);
}
}
Expand Down Expand Up @@ -997,6 +1040,10 @@ export function createResolver(program: Program): NameResolver {
[]),
]);
}
if (usingBinding.flags & SymbolFlags.Using && usingBinding.symbolSource) {
usedUsingSym.get(scope)?.add(usingBinding.symbolSource) ??
usedUsingSym.set(scope, new Set([usingBinding.symbolSource]));
}
return resolvedResult(usingBinding.symbolSource!);
}
}
Expand Down
28 changes: 28 additions & 0 deletions packages/compiler/src/core/program.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import { PackageJson } from "../types/package-json.js";
import { deepEquals, findProjectRoot, isDefined, mapEquals, mutate } from "../utils/misc.js";
import { createBinder } from "./binder.js";
import { Checker, createChecker } from "./checker.js";
import { removeUnusedCodeCodeFix } from "./compiler-code-fixes/remove-unused-code.codefix.js";
import { createSuppressCodeFix } from "./compiler-code-fixes/suppress.codefix.js";
import { compilerAssert } from "./diagnostics.js";
import { resolveTypeSpecEntrypoint } from "./entrypoint-resolution.js";
Expand Down Expand Up @@ -45,11 +46,13 @@ import {
EmitContext,
EmitterFunc,
Entity,
IdentifierNode,
JsSourceFileNode,
LibraryInstance,
LibraryMetadata,
LiteralType,
LocationContext,
MemberExpressionNode,
ModuleLibraryMetadata,
Namespace,
NoTarget,
Expand Down Expand Up @@ -250,6 +253,8 @@ export async function compile(
return program;
}

validateUnusedCode();

// Linter stage
program.reportDiagnostics(linter.lint());

Expand All @@ -260,6 +265,29 @@ export async function compile(

return program;

function validateUnusedCode() {
const getUsingName = (node: MemberExpressionNode | IdentifierNode): string => {
if (node.kind === SyntaxKind.MemberExpression) {
return `${getUsingName(node.base)}${node.selector}${node.id.sv}`;
} else {
// identifier node
return node.sv;
}
};
resolver.getUnusedUsings().forEach((target) => {
reportDiagnostic(
createDiagnostic({
code: "unused-using",
target: target,
format: {
code: `using ${getUsingName(target.name)}`,
},
codefixes: [removeUnusedCodeCodeFix(target)],
}),
);
});
}

/**
* Validate the libraries loaded during the compilation process are compatible.
*/
Expand Down
5 changes: 3 additions & 2 deletions packages/compiler/src/core/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2300,7 +2300,7 @@ export interface TemplateInstanceTarget {

export type DiagnosticTarget = TypeSpecDiagnosticTarget | SourceLocation;

export type DiagnosticSeverity = "error" | "warning";
export type DiagnosticSeverity = "error" | "warning" | "hint";

export interface Diagnostic {
code: string;
Expand Down Expand Up @@ -2529,8 +2529,9 @@ export interface DiagnosticDefinition<M extends DiagnosticMessages> {
* Diagnostic severity.
* - `warning` - Suppressable, should be used to represent potential issues but not blocking.
* - `error` - Non-suppressable, should be used to represent failure to move forward.
* - `hint` - Something to hint to a better way of doing it, like proposing a refactoring.
*/
readonly severity: "warning" | "error";
readonly severity: "warning" | "error" | "hint";
/** Messages that can be reported with the diagnostic. */
readonly messages: M;
/** Short description of the diagnostic */
Expand Down
4 changes: 3 additions & 1 deletion packages/compiler/src/server/diagnostics.ts
Original file line number Diff line number Diff line change
Expand Up @@ -141,11 +141,13 @@ function getVSLocationWithTypeInfo(
};
}

function convertSeverity(severity: "warning" | "error"): DiagnosticSeverity {
function convertSeverity(severity: "warning" | "error" | "hint"): DiagnosticSeverity {
switch (severity) {
case "warning":
return DiagnosticSeverity.Warning;
case "error":
return DiagnosticSeverity.Error;
case "hint":
return DiagnosticSeverity.Hint;
}
}
7 changes: 6 additions & 1 deletion packages/compiler/src/server/serverlib.ts
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,7 @@ export function createServer(host: ServerHost): Server {
if (!validationResult.valid) {
for (const diag of validationResult.diagnostics) {
log({
level: diag.severity,
level: diag.severity === "hint" ? "info" : diag.severity,
message: diag.message,
detail: {
code: diag.code,
Expand Down Expand Up @@ -490,6 +490,11 @@ export function createServer(host: ServerHost): Server {
}
if (each.code === "deprecated") {
diagnostic.tags = [DiagnosticTag.Deprecated];
} else if (each.code === "unused-using") {
// Unused or unnecessary code. Diagnostics with this tag are rendered faded out, so no extra work needed from IDE side
// https://vscode-api.js.org/enums/vscode.DiagnosticTag.html#google_vignette
// https://learn.microsoft.com/en-us/dotnet/api/microsoft.visualstudio.languageserver.protocol.diagnostictag?view=visualstudiosdk-2022
diagnostic.tags = [DiagnosticTag.Unnecessary];
}
diagnostic.data = { id: diagnosticIdCounter++ };
const diagnostics = diagnosticMap.get(diagDocument);
Expand Down
2 changes: 1 addition & 1 deletion packages/compiler/src/testing/expect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ export interface DiagnosticMatch {
/**
* Match the severity.
*/
severity?: "error" | "warning";
severity?: "error" | "warning" | "hint";

/**
* Name of the file for this diagnostic.
Expand Down
16 changes: 11 additions & 5 deletions packages/compiler/src/testing/test-host.ts
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ export const StandardTestLibrary: TypeSpecTestLibrary = {
};

export async function createTestHost(config: TestHostConfig = {}): Promise<TestHost> {
const testHost = await createTestHostInternal();
const testHost = await createTestHostInternal(config);
await testHost.addTypeSpecLibrary(StandardTestLibrary);
if (config.libraries) {
for (const library of config.libraries) {
Expand All @@ -257,7 +257,7 @@ export async function createTestRunner(host?: TestHost): Promise<BasicTestRunner
return createTestWrapper(testHost);
}

async function createTestHostInternal(): Promise<TestHost> {
async function createTestHostInternal(config: TestHostConfig): Promise<TestHost> {
let program: Program | undefined;
const libraries: TypeSpecTestLibrary[] = [];
const testTypes: Record<string, Type> = {};
Expand Down Expand Up @@ -315,7 +315,10 @@ async function createTestHostInternal(): Promise<TestHost> {

async function compile(main: string, options: CompilerOptions = {}) {
const [testTypes, diagnostics] = await compileAndDiagnose(main, options);
expectDiagnosticEmpty(diagnostics);
const filteredDiagnostics = config.diagnosticFilter
? diagnostics.filter(config.diagnosticFilter)
: diagnostics;
expectDiagnosticEmpty(filteredDiagnostics);
return testTypes;
}

Expand All @@ -334,10 +337,13 @@ async function createTestHostInternal(): Promise<TestHost> {
}
const p = await compileProgram(fileSystem.compilerHost, resolveVirtualPath(mainFile), options);
program = p;
const filteredDiagnostics = config.diagnosticFilter
? p.diagnostics.filter(config.diagnosticFilter)
: p.diagnostics;
logVerboseTestOutput((log) =>
logDiagnostics(p.diagnostics, createLogger({ sink: fileSystem.compilerHost.logSink })),
logDiagnostics(filteredDiagnostics, createLogger({ sink: fileSystem.compilerHost.logSink })),
);
return [testTypes, p.diagnostics];
return [testTypes, filteredDiagnostics];
}
}

Expand Down
2 changes: 2 additions & 0 deletions packages/compiler/src/testing/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@ export interface TypeSpecTestLibrary {

export interface TestHostConfig {
libraries?: TypeSpecTestLibrary[];
/** the diagnostics will be ignored if this return false */
diagnosticFilter?: (diag: Diagnostic) => boolean;
}

export class TestHostError extends Error {
Expand Down
Loading
Loading