support overload signature for deprecated. #39221
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@typescript-bot perf test. |
|
Heya @Kingwl, I've started to run the perf test suite on this PR at 482a4d3. You can monitor the build here. Update: The results are in! |
|
Hey @Kingwl, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build. |
|
@Kingwl Here they are:Comparison Report - master..39221
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@typescript-bot perf test. |
|
Heya @Kingwl, I've started to run the perf test suite on this PR at 482a4d3. You can monitor the build here. Update: The results are in! |
weswigham
reviewed
Jun 24, 2020
|
@Kingwl Here they are:Comparison Report - master..39221
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Update for rules. Let me know if something is incorrect. |
Kingwl
changed the title
|
@DanielRosenwasser this look good to you? It looks like it resolves the issue pretty well to me. |
|
@mjbvz please look at this changes. |
sandersn
requested changes
Jun 24, 2020
src/compiler/binder.ts
Outdated
Comment on lines
550
to
551
| else if (hasDeprecated) { | ||
|
|
There was a problem hiding this comment.
Suggested change
| else if (hasDeprecated) { | |
| else if (hasDeprecated) { |
src/compiler/types.ts
Outdated
| @@ -4592,6 +4592,7 @@ namespace ts { | |||
| /* @internal */ isReplaceableByMethod?: boolean; // Can this Javascript class property be replaced by a method symbol? | |||
| /* @internal */ isAssigned?: boolean; // True if the symbol is a parameter with assignments | |||
| /* @internal */ assignmentDeclarationMembers?: Map<Declaration>; // detected late-bound assignment declarations associated with the symbol | |||
| /* @internal */ allSignatureDeprecated?: boolean; // All signatures have the deprecated tag | |||
There was a problem hiding this comment.
I would add an s: allSignaturesDeprecated
src/compiler/binder.ts
Outdated
| symbol.flags &= ~SymbolFlags.Deprecated; | ||
| } | ||
| } | ||
| else if (hasDeprecated) { |
There was a problem hiding this comment.
merged signatures/other things will end up deprecating the whole thing, right?
/** @deprecated */
interface F { }
function F() { }There was a problem hiding this comment.
Later: No, the code correctly unsets the flag, but please add a test like this (or point me to an existing one).
There was a problem hiding this comment.
I had the same fear - I don't really understand why it needs to check the symbol if the resolved signature is already there though.
There was a problem hiding this comment.
Because to avoid duplicate diagnostic. eg:
/* @deprecated */
declare function foo (); void;
foo;
~~~ // foo is deprecated. checked by identifier
foo();
~~~ // foo is deprecated. checked by identifier
~~~~~ // (): void is deprecated. checked by callExpression
|
@typescript-bot perf test. |
|
Heya @Kingwl, I've started to run the perf test suite on this PR at fcd722a. You can monitor the build here. Update: The results are in! |
|
@Kingwl Here they are:Comparison Report - master..39221
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@typescript-bot perf test. |
|
Heya @Kingwl, I've started to run the perf test suite on this PR at fcd722a. You can monitor the build here. Update: The results are in! |
|
@Kingwl Here they are:Comparison Report - master..39221
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
After discussing this one-on-one, I think the best thing is to switch away from SymbolFlags.Deprecated and make the checker look at NodeFlags.Deprecated for specific declarations. |
|
@typescript-bot perf test. |
|
Heya @Kingwl, I've started to run the perf test suite on this PR at 2410ea6. You can monitor the build here. Update: The results are in! |
|
@typescript-bot perf test. |
|
Heya @Kingwl, I've started to run the perf test suite on this PR at aab7a32. You can monitor the build here. Update: The results are in! |
|
@Kingwl Here they are:Comparison Report - master..39221
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@Kingwl Here they are:Comparison Report - master..39221
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@typescript-bot perf test. |
|
Heya @Kingwl, I've started to run the perf test suite on this PR at aab7a32. You can monitor the build here. Update: The results are in! |
|
@Kingwl Here they are:Comparison Report - master..39221
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Kingwl
force-pushed
the
overload_deprecated
branch
from
0dcf09c to
c1911f6
Compare
This reverts commit d7a7297.
Kingwl
force-pushed
the
overload_deprecated
branch
from
c1911f6 to
11340da
Compare
sandersn
reviewed
Jun 29, 2020
src/compiler/utilities.ts
Outdated
| } | ||
|
|
||
| export function getDeprecatedFlags(symbol: Symbol) { | ||
| if (symbol.flags & SymbolFlags.Deprecated) { |
There was a problem hiding this comment.
instead of modifying Symbol, please using SymbolLinks with getSymbolLinks to cache deprecatedFlags.
There was a problem hiding this comment.
(the function should also move into the checker; pretty sure getSymbolLinks is not exported)
src/compiler/utilities.ts
Outdated
| } | ||
|
|
||
| export function getDeprecatedFlags(symbol: Symbol) { | ||
| if (symbol.flags & SymbolFlags.Deprecated) { |
There was a problem hiding this comment.
I would invert the symbol.flags & SymbolFlags.Deprecated check to immediately return None.
|
@typescript-bot pack this. |
|
Hey @Kingwl, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build. |
|
@typescript-bot perf test. |
|
Heya @Kingwl, I've started to run the perf test suite on this PR at 11340da. You can monitor the build here. Update: The results are in! |
|
@Kingwl Here they are:Comparison Report - master..39221
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@typescript-bot perf test. |
|
Heya @Kingwl, I've started to run the perf test suite on this PR at 2dd4148. You can monitor the build here. Update: The results are in! |
|
@Kingwl Here they are:Comparison Report - master..39221
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #39212
This or follow the below rules: