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

Add method signature completions #46370

Merged
merged 39 commits into from
Oct 28, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
701c5f3
prototype creation for method override completion snippet
gabritto Sep 10, 2021
2c84d88
WIP: start using codefix `addNewNodeForMemberSymbol` to create method…
gabritto Sep 17, 2021
4d2d476
update type of addNewNodeForMemberSymbol
gabritto Sep 20, 2021
c608a6e
add more tests and support more cases
gabritto Sep 21, 2021
2fa43e8
add more tests and fix some details
gabritto Sep 22, 2021
1d04720
wip: more fixes and tests
gabritto Sep 23, 2021
905caad
expose check override modifier in checker
gabritto Sep 23, 2021
8ad4042
fix test
gabritto Sep 30, 2021
d20eb68
WIP: add snippet support
gabritto Oct 1, 2021
c81f385
WIP: snippet support on emitter, adding snippets in completions
gabritto Oct 5, 2021
b124c84
make add snippets work with overloads (not synced)
gabritto Oct 5, 2021
db408cc
fix snippet adding
gabritto Oct 5, 2021
d7809c1
rebase
gabritto Oct 6, 2021
41a3c19
WIP: try to add snippet escaping in emitter
gabritto Oct 7, 2021
69ee9fc
support escaping in snippets
gabritto Oct 12, 2021
c3a64bc
small fixes; fixed tests
gabritto Oct 12, 2021
e079f71
more tests and fixes
gabritto Oct 13, 2021
d7c26c4
fix new tests
gabritto Oct 14, 2021
fe4fce8
fix modifier inheritance for overloads
gabritto Oct 14, 2021
b8bb27a
Merge branch 'main' into gabritto/issue45670
gabritto Oct 14, 2021
952aac1
merge conflict fixes; remove comments
gabritto Oct 14, 2021
00dc206
throw error if setOptions is called but not implemented
gabritto Oct 14, 2021
28539b6
fix newline handling
gabritto Oct 14, 2021
01eb2bb
fix weird stuff
gabritto Oct 14, 2021
70ebe86
fix tests
gabritto Oct 15, 2021
e76c18d
fix more tests
gabritto Oct 18, 2021
35c1ea3
Fix unbound host.getNewLine
andrewbranch Oct 20, 2021
a639aef
fix isParameterDeclaration changes
gabritto Oct 25, 2021
4571e98
rename diagnostic to status and remove snippets from public api
gabritto Oct 25, 2021
3a6b6bf
rename emitter functions + fix indentation
gabritto Oct 25, 2021
302c0fc
check completion kind before calling isclasslikemembercompletion
gabritto Oct 25, 2021
7bdeaa8
fix missing type parameters
gabritto Oct 26, 2021
68e5913
Revert "fix missing type parameters"
gabritto Oct 26, 2021
d796043
add isAmbient flag to addNewNodeForMemberSymbol
gabritto Oct 26, 2021
f6ea5f2
add test for abstract overloads
gabritto Oct 26, 2021
221edac
refactor snippet escaping support
gabritto Oct 27, 2021
8e9cac9
add user preference flag for enabling class member snippets
gabritto Oct 27, 2021
c5f1166
update API baseline
gabritto Oct 27, 2021
a2665d8
update tabstop order
gabritto Oct 28, 2021
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
198 changes: 163 additions & 35 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -729,6 +729,7 @@ namespace ts {
isDeclarationVisible,
isPropertyAccessible,
getTypeOnlyAliasDeclaration,
getMemberOverrideModifierStatus,
};

function getResolvedSignatureWorker(nodeIn: CallLikeExpression, candidatesOutArray: Signature[] | undefined, argumentCount: number | undefined, checkMode: CheckMode): Signature | undefined {
Expand Down Expand Up @@ -38390,7 +38391,7 @@ namespace ts {
}
}

checkMembersForMissingOverrideModifier(node, type, typeWithThis, staticType);
checkMembersForOverrideModifier(node, type, typeWithThis, staticType);

const implementedTypeNodes = getEffectiveImplementsTypeNodes(node);
if (implementedTypeNodes) {
Expand Down Expand Up @@ -38427,8 +38428,7 @@ namespace ts {
}
}

function checkMembersForMissingOverrideModifier(node: ClassLikeDeclaration, type: InterfaceType, typeWithThis: Type, staticType: ObjectType) {
const nodeInAmbientContext = !!(node.flags & NodeFlags.Ambient);
function checkMembersForOverrideModifier(node: ClassLikeDeclaration, type: InterfaceType, typeWithThis: Type, staticType: ObjectType) {
const baseTypeNode = getEffectiveBaseTypeNode(node);
const baseTypes = baseTypeNode && getBaseTypes(type);
const baseWithThis = baseTypes?.length ? getTypeWithThisArgument(first(baseTypes), type.thisType) : undefined;
Expand All @@ -38442,77 +38442,163 @@ namespace ts {
if (isConstructorDeclaration(member)) {
forEach(member.parameters, param => {
if (isParameterPropertyDeclaration(param, member)) {
checkClassMember(param, /*memberIsParameterProperty*/ true);
checkExistingMemberForOverrideModifier(
node,
staticType,
baseStaticType,
baseWithThis,
type,
typeWithThis,
param,
/* memberIsParameterProperty */ true
);
}
});
}
checkClassMember(member);
checkExistingMemberForOverrideModifier(
node,
staticType,
baseStaticType,
baseWithThis,
type,
typeWithThis,
member,
/* memberIsParameterProperty */ false,
);
}
}

function checkClassMember(member: ClassElement | ParameterPropertyDeclaration, memberIsParameterProperty?: boolean) {
const hasOverride = hasOverrideModifier(member);
const hasStatic = isStatic(member);
const isJs = isInJSFile(member);
if (baseWithThis && (hasOverride || compilerOptions.noImplicitOverride)) {
const declaredProp = member.name && getSymbolAtLocation(member.name) || getSymbolAtLocation(member);
if (!declaredProp) {
return;
}

const thisType = hasStatic ? staticType : typeWithThis;
const baseType = hasStatic ? baseStaticType : baseWithThis;
const prop = getPropertyOfType(thisType, declaredProp.escapedName);
const baseProp = getPropertyOfType(baseType, declaredProp.escapedName);
/**
* @param member Existing member node to be checked.
* Note: `member` cannot be a synthetic node.
*/
function checkExistingMemberForOverrideModifier(
node: ClassLikeDeclaration,
staticType: ObjectType,
baseStaticType: Type,
baseWithThis: Type | undefined,
type: InterfaceType,
typeWithThis: Type,
member: ClassElement | ParameterPropertyDeclaration,
memberIsParameterProperty: boolean,
reportErrors = true,
): MemberOverrideStatus {
const declaredProp = member.name
&& getSymbolAtLocation(member.name)
|| getSymbolAtLocation(member);
if (!declaredProp) {
return MemberOverrideStatus.Ok;
}

return checkMemberForOverrideModifier(
node,
staticType,
baseStaticType,
baseWithThis,
type,
typeWithThis,
hasOverrideModifier(member),
hasAbstractModifier(member),
isStatic(member),
memberIsParameterProperty,
symbolName(declaredProp),
reportErrors ? member : undefined,
);
}

const baseClassName = typeToString(baseWithThis);
if (prop && !baseProp && hasOverride) {
const suggestion = getSuggestedSymbolForNonexistentClassMember(symbolName(declaredProp), baseType);
/**
* Checks a class member declaration for either a missing or an invalid `override` modifier.
* Note: this function can be used for speculative checking,
* i.e. checking a member that does not yet exist in the program.
* An example of that would be to call this function in a completions scenario,
* when offering a method declaration as completion.
* @param errorNode The node where we should report an error, or undefined if we should not report errors.
*/
function checkMemberForOverrideModifier(
node: ClassLikeDeclaration,
staticType: ObjectType,
baseStaticType: Type,
baseWithThis: Type | undefined,
type: InterfaceType,
typeWithThis: Type,
memberHasOverrideModifier: boolean,
memberHasAbstractModifier: boolean,
memberIsStatic: boolean,
andrewbranch marked this conversation as resolved.
Show resolved Hide resolved
memberIsParameterProperty: boolean,
memberName: string,
errorNode?: Node,
): MemberOverrideStatus {
const isJs = isInJSFile(node);
const nodeInAmbientContext = !!(node.flags & NodeFlags.Ambient);
if (baseWithThis && (memberHasOverrideModifier || compilerOptions.noImplicitOverride)) {
const memberEscapedName = escapeLeadingUnderscores(memberName);
const thisType = memberIsStatic ? staticType : typeWithThis;
const baseType = memberIsStatic ? baseStaticType : baseWithThis;
const prop = getPropertyOfType(thisType, memberEscapedName);
const baseProp = getPropertyOfType(baseType, memberEscapedName);

const baseClassName = typeToString(baseWithThis);
if (prop && !baseProp && memberHasOverrideModifier) {
if (errorNode) {
const suggestion = getSuggestedSymbolForNonexistentClassMember(memberName, baseType); // Again, using symbol name: note that's different from `symbol.escapedName`
suggestion ?
error(
member,
errorNode,
isJs ?
Diagnostics.This_member_cannot_have_a_JSDoc_comment_with_an_override_tag_because_it_is_not_declared_in_the_base_class_0_Did_you_mean_1 :
Diagnostics.This_member_cannot_have_an_override_modifier_because_it_is_not_declared_in_the_base_class_0_Did_you_mean_1,
baseClassName,
symbolToString(suggestion)) :
error(
member,
errorNode,
isJs ?
Diagnostics.This_member_cannot_have_a_JSDoc_comment_with_an_override_tag_because_it_is_not_declared_in_the_base_class_0 :
Diagnostics.This_member_cannot_have_an_override_modifier_because_it_is_not_declared_in_the_base_class_0,
baseClassName);
}
else if (prop && baseProp?.declarations && compilerOptions.noImplicitOverride && !nodeInAmbientContext) {
const baseHasAbstract = some(baseProp.declarations, hasAbstractModifier);
if (hasOverride) {
return;
}
return MemberOverrideStatus.HasInvalidOverride;
}
else if (prop && baseProp?.declarations && compilerOptions.noImplicitOverride && !nodeInAmbientContext) {
const baseHasAbstract = some(baseProp.declarations, hasAbstractModifier);
if (memberHasOverrideModifier) {
return MemberOverrideStatus.Ok;
}

if (!baseHasAbstract) {
if (!baseHasAbstract) {
if (errorNode) {
const diag = memberIsParameterProperty ?
isJs ?
Diagnostics.This_parameter_property_must_have_a_JSDoc_comment_with_an_override_tag_because_it_overrides_a_member_in_the_base_class_0 :
Diagnostics.This_parameter_property_must_have_an_override_modifier_because_it_overrides_a_member_in_base_class_0 :
isJs ?
Diagnostics.This_member_must_have_a_JSDoc_comment_with_an_override_tag_because_it_overrides_a_member_in_the_base_class_0 :
Diagnostics.This_member_must_have_an_override_modifier_because_it_overrides_a_member_in_the_base_class_0;
error(member, diag, baseClassName);
error(errorNode, diag, baseClassName);
}
else if (hasAbstractModifier(member) && baseHasAbstract) {
error(member, Diagnostics.This_member_must_have_an_override_modifier_because_it_overrides_an_abstract_method_that_is_declared_in_the_base_class_0, baseClassName);
return MemberOverrideStatus.NeedsOverride;
}
else if (memberHasAbstractModifier && baseHasAbstract) {
if (errorNode) {
error(errorNode, Diagnostics.This_member_must_have_an_override_modifier_because_it_overrides_an_abstract_method_that_is_declared_in_the_base_class_0, baseClassName);
}
return MemberOverrideStatus.NeedsOverride;
}
}
else if (hasOverride) {
}
else if (memberHasOverrideModifier) {
if (errorNode) {
const className = typeToString(type);
error(
member,
errorNode,
isJs ?
Diagnostics.This_member_cannot_have_a_JSDoc_comment_with_an_override_tag_because_its_containing_class_0_does_not_extend_another_class :
Diagnostics.This_member_cannot_have_an_override_modifier_because_its_containing_class_0_does_not_extend_another_class,
className);
}
return MemberOverrideStatus.HasInvalidOverride;
}

return MemberOverrideStatus.Ok;
}

function issueMemberSpecificError(node: ClassLikeDeclaration, typeWithThis: Type, baseWithThis: Type, broadDiag: DiagnosticMessage) {
Expand Down Expand Up @@ -38559,6 +38645,48 @@ namespace ts {
}
}

/**
* Checks a member declaration node to see if has a missing or invalid `override` modifier.
* @param node Class-like node where the member is declared.
* @param member Member declaration node.
* Note: `member` can be a synthetic node without a parent.
*/
function getMemberOverrideModifierStatus(node: ClassLikeDeclaration, member: ClassElement): MemberOverrideStatus {
gabritto marked this conversation as resolved.
Show resolved Hide resolved
if (!member.name) {
return MemberOverrideStatus.Ok;
}

const symbol = getSymbolOfNode(node);
const type = getDeclaredTypeOfSymbol(symbol) as InterfaceType;
const typeWithThis = getTypeWithThisArgument(type);
const staticType = getTypeOfSymbol(symbol) as ObjectType;

const baseTypeNode = getEffectiveBaseTypeNode(node);
const baseTypes = baseTypeNode && getBaseTypes(type);
const baseWithThis = baseTypes?.length ? getTypeWithThisArgument(first(baseTypes), type.thisType) : undefined;
const baseStaticType = getBaseConstructorTypeOfClass(type);

const memberHasOverrideModifier = member.parent
? hasOverrideModifier(member)
: hasSyntacticModifier(member, ModifierFlags.Override);

const memberName = unescapeLeadingUnderscores(getTextOfPropertyName(member.name));

return checkMemberForOverrideModifier(
node,
staticType,
baseStaticType,
baseWithThis,
type,
typeWithThis,
memberHasOverrideModifier,
hasAbstractModifier(member),
isStatic(member),
/* memberIsParameterProperty */ false,
memberName,
);
}

function getTargetSymbol(s: Symbol) {
// if symbol is instantiated its flags are not copied from the 'target'
// so we'll need to get back original 'target' symbol to work with correct set of flags
Expand Down
4 changes: 4 additions & 0 deletions src/compiler/debug.ts
Original file line number Diff line number Diff line change
Expand Up @@ -351,6 +351,10 @@ namespace ts {
return formatEnum(kind, (ts as any).SyntaxKind, /*isFlags*/ false);
}

export function formatSnippetKind(kind: SnippetKind | undefined): string {
return formatEnum(kind, (ts as any).SnippetKind, /*isFlags*/ false);
}

export function formatNodeFlags(flags: NodeFlags | undefined): string {
return formatEnum(flags, (ts as any).NodeFlags, /*isFlags*/ true);
}
Expand Down
44 changes: 43 additions & 1 deletion src/compiler/emitter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1283,7 +1283,13 @@ namespace ts {
currentParenthesizerRule = undefined;
}

function pipelineEmitWithHintWorker(hint: EmitHint, node: Node): void {
function pipelineEmitWithHintWorker(hint: EmitHint, node: Node, allowSnippets = true): void {
if (allowSnippets) {
const snippet = getSnippetElement(node);
if (snippet) {
return emitSnippetNode(hint, node, snippet);
}
}
if (hint === EmitHint.SourceFile) return emitSourceFile(cast(node, isSourceFile));
if (hint === EmitHint.IdentifierName) return emitIdentifier(cast(node, isIdentifier));
if (hint === EmitHint.JsxAttributeValue) return emitLiteral(cast(node, isStringLiteral), /*jsxAttributeEscape*/ true);
Expand Down Expand Up @@ -1924,6 +1930,32 @@ namespace ts {
}
}

//
// Snippet Elements
//

function emitSnippetNode(hint: EmitHint, node: Node, snippet: SnippetElement) {
switch (snippet.kind) {
case SnippetKind.Placeholder:
emitPlaceholder(hint, node, snippet);
break;
case SnippetKind.TabStop:
emitTabStop(snippet);
break;
}
}

function emitPlaceholder(hint: EmitHint, node: Node, snippet: Placeholder) {
nonEscapingWrite(`\$\{${snippet.order}:`); // `${2:`
pipelineEmitWithHintWorker(hint, node, /*allowSnippets*/ false); // `...`
Copy link
Member

Choose a reason for hiding this comment

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

This bypasses the normal emit pipeline, which might be a problem if you want to emit synthetic JSDoc comments for JS files in the future (since that requires pipelineEmitWithComments). I'm not sure if this needs to change given the narrow conditions under which a snippet is printed, but its worth pointing out for future reference.

nonEscapingWrite(`\}`); // `}`
// `${2:...}`
}

function emitTabStop(snippet: TabStop) {
nonEscapingWrite(`\$${snippet.order}`);
}

//
// Identifiers
//
Expand Down Expand Up @@ -4457,6 +4489,16 @@ namespace ts {
writer.writeProperty(s);
}

function nonEscapingWrite(s: string) {
// This should be defined in a snippet-escaping text writer.
if (writer.nonEscapingWrite) {
writer.nonEscapingWrite(s);
}
else {
writer.write(s);
}
}

function writeLine(count = 1) {
for (let i = 0; i < count; i++) {
writer.writeLine(i > 0);
Expand Down
18 changes: 18 additions & 0 deletions src/compiler/factory/emitNode.ts
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,24 @@ namespace ts {
}
}

/**
* Gets the SnippetElement of a node.
*/
/* @internal */
export function getSnippetElement(node: Node): SnippetElement | undefined {
gabritto marked this conversation as resolved.
Show resolved Hide resolved
return node.emitNode?.snippetElement;
}

/**
* Sets the SnippetElement of a node.
*/
/* @internal */
export function setSnippetElement<T extends Node>(node: T, snippet: SnippetElement): T {
const emitNode = getOrCreateEmitNode(node);
emitNode.snippetElement = snippet;
return node;
}

/* @internal */
export function ignoreSourceNewlines<T extends Node>(node: T): T {
getOrCreateEmitNode(node).flags |= EmitFlags.IgnoreSourceNewlines;
Expand Down
Loading