Skip to content

implemented @deprecated diagnostics reporting #430

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Oct 2, 2025
Merged

implemented @deprecated diagnostics reporting #430

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
366a608
implemented @deprecated diagnostics reporting
matthewnitschke-wk Sep 26, 2025
2c3f3c5
addressed PR comments
matthewnitschke-wk Sep 29, 2025
62e1fcc
added multiline call diagnostic
matthewnitschke-wk Sep 30, 2025
b7dceb3
fmt
matthewnitschke-wk Oct 1, 2025
02a1ddf
Fix eslint errors
varungandhi-src Oct 2, 2025
8ab1b4d
Push boolean down to be adjacent to deprecated diagnostic logic
varungandhi-src Oct 2, 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
10 changes: 10 additions & 0 deletions snapshots/input/diagnostics/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/** @deprecated This class is deprecated */
class Foo {}

/** @deprecated This function is deprecated */
function bar() {}

function main() {
new Foo()
bar()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a reference to car() to trigger the logic for multiline diagnostics formatting.

}
4 changes: 4 additions & 0 deletions snapshots/input/diagnostics/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"name": "diagnostics",
"version": "0.0.1"
}
1 change: 1 addition & 0 deletions snapshots/input/diagnostics/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{}
26 changes: 26 additions & 0 deletions snapshots/output/diagnostics/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// < definition diagnostics 0.0.1 `index.ts`/

/** @deprecated This class is deprecated */
class Foo {}
// ^^^ definition diagnostics 0.0.1 `index.ts`/Foo#
// diagnostic Information:
// > This class is deprecated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong feeling on this (we can change it later), but I'm wondering if it makes sense to emit this at the definition site as well. What does the Dart indexer do? I'm thinking of downstream functionality, and it generally doesn't make sense to flag warnings for definitions that are deprecated, but only at usage sites.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dart omits the diagnostic on the definition side: https://github.com/Workiva/scip-dart/blob/master/snapshots/output/diagnostics/lib/main.dart#L21

I'll update accordingly


/** @deprecated This function is deprecated */
function bar() {}
// ^^^ definition diagnostics 0.0.1 `index.ts`/bar().
// diagnostic Information:
// > This function is deprecated

function main() {
// ^^^^ definition diagnostics 0.0.1 `index.ts`/main().
new Foo()
// ^^^ reference diagnostics 0.0.1 `index.ts`/Foo#
// diagnostic Information:
// > This class is deprecated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be good to add some test cases for multiline messages with @deprecated to capture whether they work/don't work, and make sure that the final formatting is correct.

bar()
//^^^ reference diagnostics 0.0.1 `index.ts`/bar().
//diagnostic Information:
//> This function is deprecated
}

22 changes: 22 additions & 0 deletions src/FileIndexer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,7 @@ export class FileIndexer {
range,
symbol: scipSymbol.value,
symbol_roles: role,
diagnostics: this.diagnosticsForSymbol(sym),
})
)
if (isDefinitionNode) {
Expand Down Expand Up @@ -718,6 +719,27 @@ export class FileIndexer {
}
loop(node)
}

// Returns the scip diagnostics for a given typescript symbol
private diagnosticsForSymbol(
sym: ts.Symbol
): scip.scip.Diagnostic[] | undefined {
const jsDocTags = sym.getJsDocTags()

const deprecatedTag = jsDocTags.find(tag => tag.name === 'deprecated')
if (deprecatedTag) {
return [
new scip.scip.Diagnostic({
severity: scip.scip.Severity.Information,
code: 'DEPRECATED',
message: deprecatedTag.text?.map(part => part.text).join(''),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite understand when we'll have multiple elements here, and why it's justified to join with '' instead of say ' '. Could you explain that? It'd be good to add a code comment for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The result of getJsDocTags() provides a JsDocTagInfo which contains a text field on it correlating to a fully tokenized version of the jsdoc

Because the individual parts of this include spaces, we simply join('') to generate the full tag text

I've added a comment to explain this better (as well as added a test covering this tokenizing better)

tags: [scip.scip.DiagnosticTag.Deprecated],
}),
]
}

return undefined
}
}

function isAnonymousContainerOfSymbols(node: ts.Node): boolean {
Expand Down
12 changes: 12 additions & 0 deletions src/SnapshotTesting.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,18 @@ export function formatSnapshot(
out.push(symbol.replace('\n', '|'))

pushDoc(range, occurrence.symbol, isDefinition, isStartOfLine)

if (occurrence.diagnostics && occurrence.diagnostics.length > 0) {
for (let diagnostic of occurrence.diagnostics) {
let indent = ' '.repeat(range.start.character - 2)
out.push(commentSyntax + indent)
out.push(`diagnostic ${scip.Severity[diagnostic.severity]}:\n`)
if (diagnostic.message) {
out.push(commentSyntax + indent)
out.push(`> ${diagnostic.message}\n`)
}
}
}
}

// Check if any enclosing ranges end on this line
Expand Down