Skip to content

Change "isThisless" predicates to "mayReferenceThis" predicates #20036

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

Merged
3 commits merged into from
Nov 15, 2017

Conversation

ghost
Copy link

@ghost ghost commented Nov 15, 2017

From code review on #19655

@ghost ghost requested a review from sandersn November 15, 2017 15:44
@ahejlsberg
Copy link
Member

Why would we change this?

@ghost
Copy link
Author

ghost commented Nov 15, 2017

It's easier to think about, "a function references this if some parameter does", than "a function is this-less if there isn't any non-this-less parameter".

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

I would use the naming scheme xxxReferencesThis because at some point we may make this code more complete.

case SyntaxKind.Constructor: {
// A function-like declaration references `this` if its return type does or some parameter / type parameter does.
const fn = declaration as MethodDeclaration | MethodSignature | ConstructorDeclaration;
return (declaration.kind !== SyntaxKind.Constructor && typeMayReferenceThis(getEffectiveReturnTypeNode(fn)))
Copy link
Member

Choose a reason for hiding this comment

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

if getEffectiveReturnTypeNode returns undefined for nodes with SyntaxKind.Constructor, then you can skip the kind check here.

Copy link
Author

Choose a reason for hiding this comment

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

/** A type parameter is thisless if its contraint is thisless, or if it has no constraint. */
function isThislessTypeParameter(node: TypeParameterDeclaration) {
return !node.constraint || isThislessType(node.constraint);
return true;
Copy link
Member

Choose a reason for hiding this comment

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

should probably reference #20034 here too, because there are plenty of type nodes that could be inspected for this more closely.

@ghost ghost merged commit 3d05952 into master Nov 15, 2017
@ghost ghost deleted the isntNotThisless branch November 15, 2017 20:43
@ahejlsberg
Copy link
Member

Can we not merge this until we have actually discussed it?

@ahejlsberg
Copy link
Member

@Andy-MS @sandersn Please revert this back to the way it was. These functions were designed to return true when they can conclude that an entity doesn't reference this, but otherwise return a default of false. I don't like that they now have to default to true and return false when they prove something!

ghost pushed a commit that referenced this pull request Nov 15, 2017
ghost pushed a commit that referenced this pull request Nov 16, 2017
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 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