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 support of codefix for Strict Class Initialization #21528

Merged
9 commits merged into from
Feb 23, 2018

Conversation

Kingwl
Copy link
Contributor

@Kingwl Kingwl commented Feb 1, 2018

Fixes #21509

@Kingwl Kingwl changed the title [WIP] add support of add undefined type to propertyDeclaration add support of add undefined type to propertyDeclaration Feb 2, 2018
@Kingwl Kingwl changed the title add support of add undefined type to propertyDeclaration add support of codefix for Strict Class Initialization Feb 2, 2018
@@ -0,0 +1,112 @@
/* @internal */
namespace ts.codefix {
Copy link
Contributor

Choose a reason for hiding this comment

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

you do not need to split them into 3 fixes, one code fix can return multiple actions. for instance see https://github.com/Microsoft/TypeScript/blob/master/src/services/codefixes/fixUnusedIdentifier.ts#L11 we will add two actions possibly, one for _ prefix, and one for deleting the declaration, and the user gets to choose.

@mhegazy mhegazy requested a review from a user February 2, 2018 17:42
@mhegazy
Copy link
Contributor

mhegazy commented Feb 2, 2018

@andy-ms can you please review

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Could also use a test that isn't codeFixAll.

}

function addDefiniteAssignmentAssertion(changeTracker: textChanges.ChangeTracker, propertyDeclarationSourceFile: SourceFile, propertyDeclaration: PropertyDeclaration, newLineCharacter: string): void {
const property = clone(propertyDeclaration);
Copy link

Choose a reason for hiding this comment

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

This could use updateProperty (once #21577 is in)

return { token, propertyDeclaration };
}

function getActionsForAddMissingDefiniteAssignmentAssertion (context: CodeFixContext, token: Identifier, propertyDeclaration: PropertyDeclaration): CodeFixAction[] | undefined {
Copy link

Choose a reason for hiding this comment

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

This function could be inlined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for your review
i have not inline this, because it looks clear 😃


const { token, propertyDeclaration } = info;
if (!addToSeen(seenNames, token.text)) {
return;
Copy link

Choose a reason for hiding this comment

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

Nit: No need for early return if there's only one line following it, then you could just put that line inside the if and negate the condition.

},
});

interface Info { token: Identifier; propertyDeclaration: PropertyDeclaration; }
Copy link

Choose a reason for hiding this comment

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

Nit: Not much point returning token since that's just propertyDeclaration.name.


interface Info { token: Identifier; propertyDeclaration: PropertyDeclaration; }
function getInfo (sourceFile: SourceFile, pos: number): Info | undefined {
const token = getTokenAtPosition(sourceFile, pos, /*includeJsDocComment*/ false);
Copy link

Choose a reason for hiding this comment

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

return isIdentifier(token) ? cast(token.parent, isPropertyDeclaration) : undefined;.

return createLiteral((<LiteralType>type).value);
}
else if (type.getFlags() & TypeFlags.Union) {
for (const t of (<UnionType>type).types) {
Copy link

Choose a reason for hiding this comment

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

return firstDefined((<UnionType>type).types, t => getDefaultValueFromType(checker, t));

}
else if (type.getFlags() & TypeFlags.Object) {
const objectType = <ObjectType>type;
if (getObjectFlags(objectType) & ObjectFlags.Class) {
Copy link

Choose a reason for hiding this comment

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

getObjectFlags will handle any type as input, so no need for if (type.getFlags() & TypeFlags.Object) check first. You don't need objectType since none of the uses of type require a more specific type.

else if (type.getFlags() & TypeFlags.Object) {
const objectType = <ObjectType>type;
if (getObjectFlags(objectType) & ObjectFlags.Class) {
const classDeclaration = <ClassLikeDeclaration>getClassLikeDeclarationOfSymbol(objectType.symbol);
Copy link

Choose a reason for hiding this comment

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

I think the return type of getClassLikeDeclarationOfSymbol should be changed to ClassLikeDeclaration | undefined.

if (getObjectFlags(objectType) & ObjectFlags.Class) {
const classDeclaration = <ClassLikeDeclaration>getClassLikeDeclarationOfSymbol(objectType.symbol);
const constructorDeclaration = find<ClassElement, ConstructorDeclaration>(classDeclaration.members, (m): m is ConstructorDeclaration => isConstructorDeclaration(m) && !!m.body)!;
if (constructorDeclaration &&
Copy link

Choose a reason for hiding this comment

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

You might also check that the class isn't abstract. Also, a class with no explicit constructor declaration has an implicit one with no parameters.

const classDeclaration = <ClassLikeDeclaration>getClassLikeDeclarationOfSymbol(objectType.symbol);
const constructorDeclaration = find<ClassElement, ConstructorDeclaration>(classDeclaration.members, (m): m is ConstructorDeclaration => isConstructorDeclaration(m) && !!m.body)!;
if (constructorDeclaration &&
(!constructorDeclaration.parameters || !constructorDeclaration.parameters.length) &&
Copy link

Choose a reason for hiding this comment

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

parameters should always be defined. Don't think you need to look at typeParameters since those are illegal on a constructor.

@Kingwl Kingwl force-pushed the codefix-with-strictPropertyInitialization branch from 57b1033 to 8dd5037 Compare February 5, 2018 04:14
@@ -3960,5 +3960,17 @@
"Convert to ES6 module": {
"category": "Message",
"code": 95017
},
"add Undefined type to property '{0}'": {
Copy link

Choose a reason for hiding this comment

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

Nit: Start with a capital letter, but don't capitalize other things (e.g. say "definite assignment assertion" not "Definite Assignment Assertion".) Also put 'undefined' in single quotes.

if (!propertyDeclaration) return undefined;

const newLineCharacter = getNewLineOrDefaultFromHost(context.host, context.formatContext.options);
return [
Copy link

Choose a reason for hiding this comment

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

getActionsForAddMissingInitializer is the only one that can be undefined. Maybe you could use ...optionToArray(getActionsForMissingInitializer(...)) (#21628) instead of filter.

},
fixIds: [fixIdAddDefiniteAssignmentAssertions, fixIdAddUndefinedType, fixIdAddInitializer],
getAllCodeActions: context => {
const seenNames = createMap<true>();
Copy link

Choose a reason for hiding this comment

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

Are you sure we need this? I don't think we would have two of these errors for the same property. But we might get this error for two separate properties that happen to share a name, which would result in only one fix here.

else if (type.flags & TypeFlags.Union) {
return firstDefined((<UnionType>type).types, t => getDefaultValueFromType(checker, t));
}
else if (type.flags & TypeFlags.Object) {
Copy link

Choose a reason for hiding this comment

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

Earlier comment marked as out of date, so copying here: getObjectFlags will handle any type as input, so no need for if (type.getFlags() & TypeFlags.Object) check first.

//// foo: T;
//// }

verify.codeFixAll({
Copy link

Choose a reason for hiding this comment

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

Also add tests using verify.codeFixAvailable instead of verify.codeFixAll.

"category": "Message",
"code": 95018
},
"add default Initializer to property '{0}'": {
Copy link

Choose a reason for hiding this comment

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

Nit: I think this can just say "initializer", not "default initializer".

@Kingwl Kingwl force-pushed the codefix-with-strictPropertyInitialization branch 2 times, most recently from 984c2c8 to 4b1e444 Compare February 6, 2018 02:49
@Kingwl
Copy link
Contributor Author

Kingwl commented Feb 18, 2018

ping @andy-ms
hello... still need review😅

return firstDefined((<UnionType>type).types, t => getDefaultValueFromType(checker, t));
}
else {
if (getObjectFlags(type) & ObjectFlags.Class) {
Copy link

Choose a reason for hiding this comment

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

Nit: Use else if

//// foo: T;
//// }

verify.codeFixAvailable();
Copy link

Choose a reason for hiding this comment

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

Also test the content of the code fix -- use verify.codeFix and verify.codeFixAll (in separate tests).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

errorCodes,
getCodeActions: (context) => {
const propertyDeclaration = getPropertyDeclaration(context.sourceFile, context.span.start);
if (!propertyDeclaration) return undefined;
Copy link

Choose a reason for hiding this comment

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

Nit: Just return; not return undefined; since this is a void function.

getActionsForAddMissingDefiniteAssignmentAssertion(context, propertyDeclaration, newLineCharacter)
];
let action: CodeFixAction | undefined;
if ((action = getActionsForAddMissingInitializer(context, propertyDeclaration, newLineCharacter))) result.push(action);
Copy link

Choose a reason for hiding this comment

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

append(result, getActionsForAddMissingInitializer(context, propertyDeclaration, newLineCharacter)))

return isIdentifier(token) ? cast(token.parent, isPropertyDeclaration) : undefined;
}

function getActionsForAddMissingDefiniteAssignmentAssertion (context: CodeFixContext, propertyDeclaration: PropertyDeclaration, newLineCharacter: string): CodeFixAction {
Copy link

Choose a reason for hiding this comment

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

Name the function getActionFor not getActionsFor if it doesn't return an array

@Kingwl Kingwl force-pushed the codefix-with-strictPropertyInitialization branch from 4b1e444 to 5d1bf55 Compare February 22, 2018 09:18
@ghost
Copy link

ghost commented Feb 22, 2018

Could you merge from master again and fix the lint failure in fixStrictClassInitialization.ts?

@ghost
Copy link

ghost commented Feb 23, 2018

Thanks!

@ghost ghost merged commit 30a96ba into microsoft:master Feb 23, 2018
@Kingwl Kingwl deleted the codefix-with-strictPropertyInitialization branch February 24, 2018 01:57
@microsoft microsoft locked and limited conversation to collaborators Jul 3, 2018
This pull request was closed.
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.

2 participants