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

Merged
merged 31 commits into from
Feb 19, 2025
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
64be743
add code for unused using
RodgeFu Dec 13, 2024
a1862d8
fix some tests
RodgeFu Dec 13, 2024
1c33f00
Merge remote-tracking branch 'upstream/main' into unused-using
RodgeFu Dec 26, 2024
bf0c11b
add changelog
RodgeFu Dec 26, 2024
7a78fdd
Merge branch 'main' into unused-using
RodgeFu Dec 26, 2024
a01cd7f
report hint as info to server log
RodgeFu Dec 26, 2024
d50e959
fix tests
RodgeFu Dec 27, 2024
0478187
fix test
RodgeFu Dec 27, 2024
52298a9
fix test and add one more test case
RodgeFu Dec 27, 2024
46ccfca
fix test
RodgeFu Dec 27, 2024
50e995b
fix e2e test
RodgeFu Dec 27, 2024
42a3db2
add changelog
RodgeFu Dec 27, 2024
73f2bc7
Merge branch 'main' into unused-using
RodgeFu Jan 13, 2025
299c118
Merge remote-tracking branch 'upstream/main' into unused-using
RodgeFu Jan 15, 2025
6617403
refine the message for unused using
RodgeFu Jan 15, 2025
c3b3e9b
Merge remote-tracking branch 'upstream/main' into unused-using
RodgeFu Jan 16, 2025
7d2d58d
update test
RodgeFu Jan 17, 2025
2ce546c
Merge remote-tracking branch 'upstream/main' into unused-using
RodgeFu Jan 17, 2025
a68e286
fix test
RodgeFu Jan 17, 2025
a5a4e38
Merge remote-tracking branch 'upstream/main' into unused-using
RodgeFu Jan 29, 2025
4060338
using linter to report unused-using
RodgeFu Feb 13, 2025
4df823c
update test
RodgeFu Feb 13, 2025
38c769a
Merge remote-tracking branch 'upstream/main' into unused-using
RodgeFu Feb 13, 2025
3eaf6ab
update tests
RodgeFu Feb 13, 2025
55db27b
update tests for unused using
RodgeFu Feb 13, 2025
35c674b
remove unnecessary changelot
RodgeFu Feb 13, 2025
98c5b81
add more test for unused using's logic in lsp
RodgeFu Feb 13, 2025
0a22aa3
refine some code and add more test
RodgeFu Feb 13, 2025
a95875e
update linter to expose registerLinterLibrary method
RodgeFu Feb 15, 2025
872495d
update test per feedback
RodgeFu Feb 15, 2025
59e6b2a
Merge branch 'main' into unused-using
RodgeFu Feb 16, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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`'${"code"}' is declared but never used.`,
},
},

/**
* 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