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

Codefix for implementing interfaces #11547

Merged
merged 101 commits into from
Dec 30, 2016
Merged

Codefix for implementing interfaces #11547

merged 101 commits into from
Dec 30, 2016

Conversation

paulvanbrenk
Copy link
Contributor

@paulvanbrenk paulvanbrenk commented Oct 12, 2016

And fixing extends vs. implements

if (token.kind === SyntaxKind.Identifier && token.parent.parent.kind === SyntaxKind.HeritageClause) {
const children = (<HeritageClause>token.parent.parent).getChildren();
ts.forEach(children, child => {
if (child.kind === SyntaxKind.ExtendsKeyword) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not sufficient. what if the class already has an implements clause. you want to first look for implements clause, and if you found one, move the interface to the implements list otherwise rename the extends to implements. one option is to generate two edits, one that always removes the extends clause all together, and another one that adds the target to the right place.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also please add tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed. The edit is to change extends -> implements, and if implements is already present, change implements -> ','

@@ -0,0 +1,268 @@
/* @internal */
namespace ts.codefix {
registerCodeFix({
Copy link
Contributor

Choose a reason for hiding this comment

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

please split these into different files. let's keep each code fix in its own file.

Copy link
Contributor

Choose a reason for hiding this comment

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

you can still share the helpers between files if you need to.

Copy link
Contributor

Choose a reason for hiding this comment

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

Split, and helpers moved to src/services/utilities.ts


let textChanges: TextChange[] = [];

if (token.kind === SyntaxKind.Identifier && token.parent.kind === SyntaxKind.ClassDeclaration) {
Copy link
Contributor

Choose a reason for hiding this comment

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

what about classExpression? var x = class C implements I { }? use isClassLike instead

Copy link
Contributor

Choose a reason for hiding this comment

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

Changed.


let textChanges: TextChange[] = [];

if (token.kind === SyntaxKind.Identifier && token.parent.kind === SyntaxKind.ClassDeclaration) {
Copy link
Contributor

Choose a reason for hiding this comment

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

use isClassLike

Copy link
Contributor

Choose a reason for hiding this comment

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

Changed

const token = getTokenAtPosition(sourceFile, start);
const checker = context.program.getTypeChecker();

let textChanges: TextChange[] = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the point of this initialization, you can just override it below. consider just leaving this as undefined, and checking for undefined below.

Copy link
Contributor

Choose a reason for hiding this comment

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

Refactored

if (token.kind === SyntaxKind.Identifier && token.parent.kind === SyntaxKind.ClassDeclaration) {
const classDeclaration = <ClassDeclaration>token.parent;
const startPos = classDeclaration.members.pos;
const classMembers = getClassMembers(classDeclaration);
Copy link
Contributor

Choose a reason for hiding this comment

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

you just need the abstract ones.

Copy link
Contributor

Choose a reason for hiding this comment

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

Changed.

const classMembers = getClassMembers(classDeclaration);
const trackingAddedMembers: string[] = [];
const extendsClause = ts.getClassExtendsHeritageClauseElement(classDeclaration);
textChanges = textChanges.concat(getChanges(extendsClause, classMembers, startPos, checker, /*reference*/ false, trackingAddedMembers, context.newLineCharacter));
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not handle properties declared in the constructor as parameter property declarations, e.g. this should only add b and not a.

interface I {
    a: number,
    b:string;
}
class C implements I {
    constructor(public a) {}
}

Please add tests.

if (interfaceMembers[j].name && existingMembers.indexOf(interfaceMembers[j].name.getText()) === -1) {
if (interfaceMembers[j].kind === SyntaxKind.PropertySignature) {
const interfaceProperty = <PropertySignature>interfaceMembers[j];
if (trackingAddedMembers.indexOf(interfaceProperty.name.getText()) === -1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this does not handle computed properties correctly. e.g. [Symbol.Iterator]. please add tests.

}

function getMembersAndStartPosFromReference(variableDeclaration: VariableDeclaration): { startPos: number, members: string[] } {
const children = variableDeclaration.getChildren();
Copy link
Contributor

Choose a reason for hiding this comment

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

variableDeclaration.initializer ?

/* @internal */
namespace ts.codefix {
registerCodeFix({
errorCodes: [Diagnostics.Type_0_is_not_assignable_to_type_1.code],
Copy link
Contributor

Choose a reason for hiding this comment

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

this code fix is not nearly close to being complete. please remove this from this patch and added by itself later on. i am not sure we should do this either. there are issues that are not fixable. and we are not detecting them and handling them correctly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Removed, and tests changed.

}
`);
verify.not.codeFixAvailable();
// verify.codeFixAtPosition(`
Copy link
Contributor

Choose a reason for hiding this comment

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

why are these still here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Removed.

getReturnTypeOfSignature,
getNonNullableType,
getSymbolsInScope,
createSymbol,
Copy link
Contributor

Choose a reason for hiding this comment

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

i would say use new Symbol() here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Changed.

getSignaturesOfType,
getIndexTypeOfType,
getBaseTypes,
getUnionType,
getIntersectionType,
Copy link
Contributor

Choose a reason for hiding this comment

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

if this is not used i would remove it for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Removed.

getSymbolAtLocation,
getShorthandAssignmentValueSymbol,
getExportSpecifierLocalTargetSymbol,
getTypeAtLocation: getTypeOfNode,
getPropertySymbolOfDestructuringAssignment,
signatureToString,
Copy link
Contributor

Choose a reason for hiding this comment

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

consider inlining these two using checker.getSymbolDisplayBuilder().

Copy link
Contributor

Choose a reason for hiding this comment

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

signatureToString is needed independently in the checker anyways, so I'd prefer to avoid copying the code if it can be avoided.

indexSignatureToString: Only used in one place in the current PR, but we may find it useful in a future codefix for adding index signatures. Will inline for now, but may have to re-declare it in the future.

const classDecl = <ClassDeclaration>token.parent;
const startPos = classDecl.members.pos;

const InstantiatedExtendsType = checker.getTypeFromTypeReference(getClassExtendsHeritageClauseElement(classDecl)) as InterfaceType;
Copy link
Contributor

Choose a reason for hiding this comment

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

please add a test for expression with type arguments case:

function foo<T>(a: T) {
    abstract class C<U> { 
        a: T;
        b: U;
        abstract method2();
    }
    return C;
}


class B extends foo("s")<number> { 
    method() { 
    }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Added.

const decls = symbol.getDeclarations();
Debug.assert(!!(decls && decls.length > 0));
const flags = getModifierFlags(decls[0]);
return !(flags & ModifierFlags.Private) && !!(flags & ModifierFlags.Abstract);
Copy link
Contributor

Choose a reason for hiding this comment

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

i would ignore private here.

Copy link
Contributor

Choose a reason for hiding this comment

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

return symbol.valueDeclaration.flags & ModifierFlags.Private

Copy link
Contributor

Choose a reason for hiding this comment

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

Needed to suppress fix when the user creates a class with a private abstract property/member. Is there a better way to detect general errors on a declaration/node? This would be useful to suppress codefixes in many situations.

}

function symbolRefersToNonPrivateMember(symbol: Symbol): boolean {
const decls = symbol.getDeclarations();
Copy link
Contributor

Choose a reason for hiding this comment

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

just:

return symbol.valueDecaration.flags & ModifierFlags.Private

Copy link
Contributor

@aozgaa aozgaa Dec 14, 2016

Choose a reason for hiding this comment

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

EDIT: moved response to the right place.

Made the change here.


function createParameterDeclaration(index: number, minArgCount: number, typeNode: TypeNode, enclosingSignatureDeclaration: SignatureDeclaration): ParameterDeclaration {
const newParameter = createNode(SyntaxKind.Parameter) as ParameterDeclaration;
newParameter.symbol = checker.createSymbol(SymbolFlags.FunctionScopedVariable, "arg" + index);
Copy link
Contributor

Choose a reason for hiding this comment

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

i would rather we use a name form one of hte signatures.

}

if (hasRestParameter) {
const anyArrayTypeNode = createNode(SyntaxKind.ArrayType) as ArrayTypeNode;
Copy link
Contributor

Choose a reason for hiding this comment

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

consider making these implicitAny's instead by not setting a type, and handling this in the writer instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Changed.

@zhengbli zhengbli dismissed their stale review December 21, 2016 19:47

Most of my reviews are updated

@mhegazy mhegazy closed this Dec 30, 2016
@mhegazy mhegazy reopened this Dec 30, 2016
@mhegazy mhegazy merged commit 524fa64 into master Dec 30, 2016
@mhegazy mhegazy deleted the interfaceFixes branch December 30, 2016 22:28
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants