-
Notifications
You must be signed in to change notification settings - Fork 17
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
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 {} |
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# | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
// } | ||
this.emitSourceFileOccurrence() | ||
this.visit(this.sourceFile) | ||
} | ||
|
@@ -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 || [] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment doesn't properly explain what is going on. Why does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can reproduce the ambiguous definition with tsserver
I updated the comment to give this example and link to this PR.
|
||
for (const declaration of declarations) { | ||
const scipSymbol = this.scipSymbol(declaration) | ||
|
||
if (scipSymbol.isEmpty()) { | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.