Fix bug where an interface definition had a conflicting symbol with a const in the same file #206
Conversation
| // ^^^^^^^^^^^^^^^^ 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. |
There was a problem hiding this comment.
The expected behavior
| // ^^^^^^^^^^^^^^^^ definition syntax 1.0.0 src/`conflicting-const-interface.ts`/ConflictingConst. | |
| // ^^^^^^^^^^^^^^^^ definition syntax 1.0.0 src/`conflicting-const-interface.ts`/ConflictingConst# |
olafurpg
changed the title
| // Uncomment below if you want to skip certain files for local development. | ||
| // if (!this.sourceFile.fileName.includes('conflicting-const')) { | ||
| // return | ||
| // } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
src/FileIndexer.ts
Outdated
| const declarations = isDefinitionNode | ||
| ? [node.parent] // Don't emit ambiguous definition at definition-site | ||
| : sym?.declarations || [] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Reported via Slack https://sourcegraph.slack.com/archives/CHXHX7XAS/p1668157176918589?thread_ts=1668128629.962219&cid=CHXHX7XAS
Test plan
See the snapshot test for a minimized reproduction. I also manually verified the bug has been fixed by indexing microsoft/vscode with a local build and uploading the index to https://sourcegraph.com/github.com/microsoft/vscode@9984da1/-/blob/src/vs/workbench/services/workspaces/common/workspaceEditing.ts?L13:18#tab=implementations_typescript