Completion for default export should be '.default'#16742
Conversation
ghost
force-pushed
the
completionsDefaultExport
branch
2 times, most recently
from
f58a503 to
88ce5b0
Compare
ghost
force-pushed
the
completionsDefaultExport
branch
from
88ce5b0 to
95613fb
Compare
ghost
mentioned this pull request
sandersn
left a comment
sandersn
left a comment
There was a problem hiding this comment.
I like the results but I have some questions about the implementation.
| } | ||
|
|
||
| switch (location.kind) { | ||
| case SyntaxKind.SourceFile: |
There was a problem hiding this comment.
I don't understand why this case goes away.
There was a problem hiding this comment.
This comes from the second Update comment -- exported symbols aren't really the same as the symbols in scope.
| ////a./**/; | ||
|
|
||
| goTo.marker(); | ||
| verify.completionListContains("default", "function f(): void"); |
There was a problem hiding this comment.
Interesting, is a.f legal here?
There was a problem hiding this comment.
No, the property name is default; function f(): void is just the display text.
|
|
||
| /** See comment on `declareModuleMember` in `binder.ts`. */ | ||
| export function getCombinedLocalAndExportSymbolFlags(symbol: Symbol): SymbolFlags { | ||
| return symbol.exportSymbol ? symbol.exportSymbol.flags | symbol.flags : symbol.flags; |
There was a problem hiding this comment.
why combine flags in the true case instead of just returning symbol.exportSymbol.flags?
There was a problem hiding this comment.
In findAllRefsForDefaultExport08 (and findAllRefsForDefaultExport02), we have an exported class and a non-exported namespace merged with it. So the local symbol will have flags that the exported symbol is lacking; it has NamespaceModule|ExportValue|ExportType while the exported symbol just has Class.
|
|
||
| public symbolsInScope(range: Range): ts.Symbol[] { | ||
| const node = this.goToAndGetNode(range); | ||
| return this.getChecker().getSymbolsInScope(node, ts.SymbolFlags.Value | ts.SymbolFlags.Type | ts.SymbolFlags.Namespace); |
There was a problem hiding this comment.
nit: doesn't SymbolFlags have an alias defined like SymbolFlags.All that's equivalent?
There was a problem hiding this comment.
Not currently; All wouldn't be a good name since that excludes some (Constructor, Signature, and everything from ExportValue on), but this combination is often useful; I'm tempted to just name it ValueTypeNamespace.
| this.state.verifySymbolAtLocation(startRange, declarationRanges); | ||
| } | ||
|
|
||
| public typeOfSymbolAtLocation(range: FourSlash.Range, symbol: ts.Symbol, expected: string) { |
There was a problem hiding this comment.
can't we get this from the quick info? I don't know whether we need a separate method for comparing the stringified type of a symbol.
There was a problem hiding this comment.
Well, quick info gets us the type of the symbol under the cursor -- this lets us call getTypeOfSymbolAtLocation on any symbol, at the current location.
src/services/services.ts
Outdated
2 participants
Fixes #16706 and #12957
Just use
symbol.nameinstead ofgetDeclaredName, sincegetDeclaredNameintentionally gets the declared name of a default export instead of just "default".This change apparently also affects completions for unicode escapes, but I think the change is good. In this code:
We will now give you a completion for
B, which is correct --Bis in scope because\u0042is equivalent to it.Update: Also fixed #16741.
Update:
We must also change the behavior for
getSymbolsInScope()to return local symbols, not exported symbols. We need different behavior based on whether the symbols are in scope or from an export; today's example being that an export is accessed with '.default' while a local symbol is accessed by its name. Also fixed a bug wheregetSymbolAtLocationcouldn't handle a local symbol.