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

Fix bug where an interface definition had a conflicting symbol with a const in the same file #206

Merged
merged 4 commits into from
Nov 16, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
6 changes: 6 additions & 0 deletions Development.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,12 @@ npm run update-snapshots

Generate snapshots and update.

## Skipping files/test for local development

Search for the query `"Uncomment below if you want to skip` to find places where
you can uncomment code to skip tests/files for a faster edit/test/debug feedback
loop during local development.

## Snapshotting arbitrary projects

```sh
Expand Down
3 changes: 3 additions & 0 deletions snapshots/input/syntax/src/conflicting-const-interface.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export const ConflictingConst = 42
export interface ConflictingConst {}
export class ImplementsConflictingConst implements ConflictingConst {}
2 changes: 1 addition & 1 deletion snapshots/output/pure-js/src/main.js
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@
if (forever()) {
// ^^^^^^^ reference pure-js 1.0.0 src/`main.js`/forever().
var k = 1
// ^ definition local 14
// ^ definition local 17
// documentation ```ts\nvar k: number\n```
}
print_fib(k)
Expand Down
6 changes: 3 additions & 3 deletions snapshots/output/syntax/src/accessors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
// ^^^^^^^ reference syntax 1.0.0 src/`accessors.ts`/C#_length.
}
set length(value: number) {
// ^^^^^^ definition syntax 1.0.0 src/`accessors.ts`/C#`<get>length`().
// ^^^^^^ definition syntax 1.0.0 src/`accessors.ts`/C#`<set>length`().
// documentation ```ts\nget length: number\n```
// ^^^^^ definition syntax 1.0.0 src/`accessors.ts`/C#`<set>length`().(value)
// documentation ```ts\n(parameter) value: number\n```
Expand Down Expand Up @@ -46,7 +46,7 @@
// ^^^^^^^ reference syntax 1.0.0 src/`accessors.ts`/D#_length.
}
public set length(value: number) {
// ^^^^^^ definition syntax 1.0.0 src/`accessors.ts`/D#`<get>length`().
// ^^^^^^ definition syntax 1.0.0 src/`accessors.ts`/D#`<set>length`().
// documentation ```ts\nget length: number\n```
// ^^^^^ definition syntax 1.0.0 src/`accessors.ts`/D#`<set>length`().(value)
// documentation ```ts\n(parameter) value: number\n```
Expand All @@ -65,7 +65,7 @@
// ^^^^^^^^^ reference syntax 1.0.0 src/`accessors.ts`/D#_capacity.
}
private set capacity(value: number) {
// ^^^^^^^^ definition syntax 1.0.0 src/`accessors.ts`/D#`<get>capacity`().
// ^^^^^^^^ definition syntax 1.0.0 src/`accessors.ts`/D#`<set>capacity`().
// documentation ```ts\nget capacity: number\n```
// ^^^^^ definition syntax 1.0.0 src/`accessors.ts`/D#`<set>capacity`().(value)
// documentation ```ts\n(parameter) value: number\n```
Expand Down
15 changes: 15 additions & 0 deletions snapshots/output/syntax/src/conflicting-const-interface.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
export const ConflictingConst = 42
// definition syntax 1.0.0 src/`conflicting-const-interface.ts`/
//documentation ```ts\nmodule "conflicting-const-interface.ts"\n```
// ^^^^^^^^^^^^^^^^ definition syntax 1.0.0 src/`conflicting-const-interface.ts`/ConflictingConst.
// documentation ```ts\ninterface ConflictingConst\n```
export interface ConflictingConst {}
// ^^^^^^^^^^^^^^^^ definition syntax 1.0.0 src/`conflicting-const-interface.ts`/ConflictingConst#
// documentation ```ts\ninterface ConflictingConst\n```
export class ImplementsConflictingConst implements ConflictingConst {}
// ^^^^^^^^^^^^^^^^^^^^^^^^^^ definition syntax 1.0.0 src/`conflicting-const-interface.ts`/ImplementsConflictingConst#
// documentation ```ts\nclass ImplementsConflictingConst\n```
// relationship implementation scip-typescript npm syntax 1.0.0 src/`conflicting-const-interface.ts`/ConflictingConst#
// ^^^^^^^^^^^^^^^^ reference syntax 1.0.0 src/`conflicting-const-interface.ts`/ConflictingConst.
// ^^^^^^^^^^^^^^^^ reference syntax 1.0.0 src/`conflicting-const-interface.ts`/ConflictingConst#

9 changes: 8 additions & 1 deletion src/FileIndexer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ export class FileIndexer {
this.workingDirectoryRegExp = new RegExp(options.cwd, 'g')
}
public index(): void {
// Uncomment below if you want to skip certain files for local development.
// if (!this.sourceFile.fileName.includes('conflicting-const')) {
// return
// }
Comment on lines +40 to +43
Copy link
Contributor

Choose a reason for hiding this comment

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

Why add these special comments for particular test cases? We could potentially add a test filter regex or something if that's desired, this seems a little strange.

Copy link
Member Author

Choose a reason for hiding this comment

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

I haven't figured out yet how to implement this kind of filtering from the test framework so I prefer to keep it as a comment until we figure out a better solution.

this.emitSourceFileOccurrence()
this.visit(this.sourceFile)
}
Expand Down Expand Up @@ -104,7 +108,10 @@ export class FileIndexer {
if (isDefinitionNode) {
role |= scip.scip.SymbolRole.Definition
}
for (const declaration of sym?.declarations || []) {
const declarations = isDefinitionNode
? [node.parent] // Don't emit ambiguous definition at definition-site
: sym?.declarations || []
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment doesn't properly explain what is going on. Why does sym?.declarations lead to ambiguity? Can we trust it for non-definition nodes? It seems like there is something unusual going on with the AST structure here which needs more explanation.

Copy link
Member Author

Choose a reason for hiding this comment

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

You can reproduce the ambiguous definition with tsserver

export const Conflict = 42
interface Conflict {}
          ^^^^^^^^ Go to definition here shows two results: the const and the interface

I updated the comment to give this example and link to this PR.

Can we trust it for non-definition nodes?

sym.declarations is what tsserver uses so I think we need to trust it except for odd-cases like this one where it seems like the API has a bug.

for (const declaration of declarations) {
const scipSymbol = this.scipSymbol(declaration)

if (scipSymbol.isEmpty()) {
Expand Down
4 changes: 4 additions & 0 deletions src/main.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@ interface PackageJson {
workspaces: string[]
}
for (const snapshotDirectory of snapshotDirectories) {
// Uncomment below if you want to skip certain tests for local development.
// if (!snapshotDirectory.includes('syntax')) {
// continue
// }
const inputRoot = join(inputDirectory, snapshotDirectory)
const outputRoot = join(outputDirectory, snapshotDirectory)
if (!fs.statSync(inputRoot).isDirectory()) {
Expand Down