Conversation
ghost
force-pushed
the
completionsThis
branch
from
312976f to
9550efd
Compare
ghost
force-pushed
the
completionsThis
branch
from
9550efd to
17d62c8
Compare
src/compiler/checker.ts
Outdated
| return node && tryGetThisTypeAt(node); | ||
| }, | ||
| isMemberSymbol: symbol => | ||
| symbol.flags & SymbolFlags.ClassMember |
There was a problem hiding this comment.
Asked @sandersn and @weswigham and they didn't know why these are properties, but if I try to change undefined to a variable I can't build the compiler.
There was a problem hiding this comment.
seems that this should be in utilities instead on the checker. nothing here is about the checker internal state, but rather how we mark symbols.
|
I think this one could use a second review. @sheetalkamat @weswigham @sandersn @rbuckton @mhegazy |
src/services/completions.ts
Outdated
|
|
||
| function getInsertTextAndReplacementSpan(): { insertText?: string, replacementSpan?: TextSpan } { | ||
| if (kind === CompletionKind.Global) { | ||
| if (typeChecker.isMemberSymbol(symbol)) { |
There was a problem hiding this comment.
seems kinda weird that we add the symbol, then later on ask the checker if they were the ones we added.. i wounder if we can do something different here..
There was a problem hiding this comment.
alas.. i could not come up with a better idea here myself. ignore my previous rant.
There was a problem hiding this comment.
We could use SymbolOriginInfoMap with a union type { type: "this-type" } | { type: "export", ... }, would that be a good idea?
src/services/completions.ts
Outdated
| hasAction: trueOrUndefined(needsConvertPropertyAccess || origin !== undefined), | ||
| hasAction: trueOrUndefined(origin !== undefined), | ||
| isRecommended: trueOrUndefined(isRecommendedCompletionMatch(symbol, recommendedCompletion, typeChecker)), | ||
| ...getInsertTextAndReplacementSpan(), |
There was a problem hiding this comment.
can we optimize this a bit instead of generating an extra object allocation on every entry.
| let insertText: string | undefined; | ||
| let replacementSpan: TextSpan | undefined; | ||
| if (kind === CompletionKind.Global && origin && origin.type === "this-type") { | ||
| insertText = needsConvertPropertyAccess ? `this["${name}"]` : `this.${name}`; |
There was a problem hiding this comment.
do you not need replacement span eg. if refering to property x and you want to replace it with this.x ?
There was a problem hiding this comment.
The default replacement span should already be the identifier itself. We only need to set replacementSpan if we are replacing something else, too, such as replacing .x with ["x"].
3 participants
Fixes #21202