Skip to content

fix static method reference non-static#38730

Merged
sheetalkamat merged 17 commits intomicrosoft:masterfrom
ShuiRuTian:fix-class-static-method-reference
Jul 9, 2020
Merged

fix static method reference non-static#38730
sheetalkamat merged 17 commits intomicrosoft:masterfrom
ShuiRuTian:fix-class-static-method-reference

Conversation

@ShuiRuTian
Copy link
Copy Markdown
Contributor

Fixes #37830

@ShuiRuTian
Copy link
Copy Markdown
Contributor Author

ShuiRuTian commented May 25, 2020

Not sure that the test behavior is correct, please check them.
And maybe the definition text of static method and instance method need to be different? They are all like '(method) X.foo(): void' now.

@DanielRosenwasser
Copy link
Copy Markdown
Member

Just as a heads up, your commits don't seem to be associated with your GitHub account. While this isn't technically a problem, you might care if you want more appropriate attribution. You can either make sure your GitHub account is associated with the email address that you're using for your commits, or rebase and amend your commits to fix the author name and email.

If you don't care about any of this stuff, we can start reviewing.

Comment thread src/services/findAllReferences.ts Outdated
@ShuiRuTian
Copy link
Copy Markdown
Contributor Author

ShuiRuTian commented May 27, 2020

Just as a heads up, your commits don't seem to be associated with your GitHub account. While this isn't technically a problem, you might care if you want more appropriate attribution. You can either make sure your GitHub account is associated with the email address that you're using for your commits, or rebase and amend your commits to fix the author name and email.

If you don't care about any of this stuff, we can start reviewing.

Oh, I did not notice it, a big thank you!

@ShuiRuTian
Copy link
Copy Markdown
Contributor Author

Friendly ping @DanielRosenwasser ~

1 similar comment
@ShuiRuTian
Copy link
Copy Markdown
Contributor Author

Friendly ping @DanielRosenwasser ~

Comment thread src/services/findAllReferences.ts Outdated
Comment thread src/services/findAllReferences.ts Outdated
Comment thread src/services/findAllReferences.ts Outdated
Comment thread src/services/findAllReferences.ts Outdated
}
}

return search.includes(baseSymbol || rootSymbol || sym)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

includes is too new, use one of the helper utilities in core.ts

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh, the type of search is Search rather than Array.

Comment thread src/services/findAllReferences.ts Outdated
Comment thread src/services/findAllReferences.ts Outdated
: undefined;
};
const allowBaseTypes = (rootSymbol: Symbol) =>
!(search.parents?.every(parent => !explicitlyInheritsFrom(rootSymbol.parent!, parent, state.inheritsFromCache, checker)));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
!(search.parents?.every(parent => !explicitlyInheritsFrom(rootSymbol.parent!, parent, state.inheritsFromCache, checker)));
forEach(search.parents, parent => !explicitlyInheritsFrom(rootSymbol.parent!, parent, state.inheritsFromCache, checker));

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

should allowBaseTypes be converted to function?

Comment thread src/services/findAllReferences.ts Outdated
Comment thread src/services/findAllReferences.ts Outdated
@ShuiRuTian
Copy link
Copy Markdown
Contributor Author

Isn't it finished? Why it says CI build is in progress?

Comment thread src/compiler/core.ts Outdated
if (!callback(array[i], i)) {
return false;
}
if (!array) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

interesting this should be !length(array) instead

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

A little confused, another review says "revert changes in core", so this is also reverted?

Comment thread src/services/findAllReferences.ts Outdated
Comment thread src/services/findAllReferences.ts Outdated
Comment thread src/services/findAllReferences.ts Outdated
Copy link
Copy Markdown
Member

@sheetalkamat sheetalkamat left a comment

Choose a reason for hiding this comment

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

Please incorporate feedback

Comment thread src/services/findAllReferences.ts Outdated
@ShuiRuTian ShuiRuTian requested a review from sheetalkamat July 9, 2020 03:49
Copy link
Copy Markdown
Member

@sheetalkamat sheetalkamat left a comment

Choose a reason for hiding this comment

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

Two things:

  1. This only needs to check if static with baseSymbol matches reference symbol so this keeps the change only to that.
  2. Checking if symbol is static is done only if there is basesymbol so avoids having to check that for other symbols

Comment thread src/services/findAllReferences.ts Outdated
Comment thread src/services/findAllReferences.ts Outdated
Comment thread src/services/findAllReferences.ts Outdated
Comment thread src/services/findAllReferences.ts Outdated
Comment thread src/services/findAllReferences.ts Outdated
@sheetalkamat sheetalkamat merged commit bf1ea65 into microsoft:master Jul 9, 2020
@sheetalkamat
Copy link
Copy Markdown
Member

@ShuiRuTian thank you for working on this

@microsoft microsoft locked as resolved and limited conversation to collaborators Oct 21, 2025
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.

Find all references counts references for static member and regular member with same name

3 participants