Skip to content

TypeScript '@deprecated' warning shown for wrong overload #40007

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

Closed
rsivan opened this issue Aug 11, 2020 · 10 comments
Closed

TypeScript '@deprecated' warning shown for wrong overload #40007

rsivan opened this issue Aug 11, 2020 · 10 comments
Labels
Bug A bug in TypeScript Domain: Quick Info e.g. hover text, tool-tips, and tooltips. Help Wanted You can do this Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented.
Milestone

Comments

@rsivan
Copy link

rsivan commented Aug 11, 2020

TS Template added by @mjbvz

TypeScript Version: 4.1.0-dev.20200811

Search Terms

  • deprecated
  • jsdoc

Repo code
from @IllusionMH

import { of, SchedulerLike } from 'rxjs';

declare const arr: number[];
declare const scheduler: SchedulerLike;

of(arr); // has @deprecated but only in comment

of(arr, scheduler); // marked as deprecated, has @deprecated in comment

Issue Type: Bug

In a Typescript source file, import 'of' from 'rxjs';
type a statement that includes 'of(someArray);
Hover near 'of', it shows a deprecation notice, however the actual deprecation is for a different overload of 'of'.
The same happens on any @deprecated annotations with multiple overloads

VS Code version: Code 1.47.3 (91899dcef7b8110878ea59626991a18c8a6a1b3e, 2020-07-23T13:08:29.692Z)
OS version: Darwin x64 19.6.0

System Info
Item Value
CPUs Intel(R) Core(TM) i7-6820HQ CPU @ 2.70GHz (8 x 2700)
GPU Status 2d_canvas: enabled
flash_3d: enabled
flash_stage3d: enabled
flash_stage3d_baseline: enabled
gpu_compositing: enabled
metal: disabled_off
multiple_raster_threads: enabled_on
oop_rasterization: disabled_off
protected_video_decode: unavailable_off
rasterization: enabled
skia_renderer: disabled_off_ok
video_decode: enabled
viz_display_compositor: enabled_on
viz_hit_test_surface_layer: disabled_off_ok
webgl: enabled
webgl2: enabled
Load (avg) 16, 11, 8
Memory (System) 16.00GB (7.20GB free)
Process Argv --disable-extensions
Screen Reader no
VM 0%
Extensions disabled

d ts
deprecated

@IllusionMH
Copy link
Contributor

Looks like duplicate of microsoft/vscode#80292 and #30181
They are marked as fixed, however reproducible with typescript@next

Interestingly that while it shows comment with deprecation, of isn't crossed out as deprecated signature 😄

/assign @mjbvz

@mjbvz
Copy link
Contributor

mjbvz commented Aug 11, 2020

Please share the text of the code along with screenshots

@IllusionMH
Copy link
Contributor

@mjbvz minimal repro I tested with Playground Link

@rsivan
Copy link
Author

rsivan commented Aug 11, 2020

@mjbvz minimal repro I tested with Playground Link

@mjbvz Do you still need more info after this reply was made with a link to reproducible code?

@mjbvz mjbvz transferred this issue from microsoft/vscode Aug 11, 2020
@mjbvz mjbvz removed their assignment Aug 11, 2020
@DanielRosenwasser DanielRosenwasser added Bug A bug in TypeScript Domain: Quick Info e.g. hover text, tool-tips, and tooltips. Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. labels Aug 11, 2020
@DanielRosenwasser
Copy link
Member

I think we have a heuristic where if a signature has no JSDoc comments, we'll pick out the first one. This is often useful to avoid duplication. You can try it out here, where quick info on the call picks up @badda:

/**
 * @badda
 */
declare function foo(): string;
/**
 * @bing
 */
declare function foo(a: string): string;
declare function foo(a: number): number;


foo(123)

It's unclear what we should do in some of these situations.

Maybe the right behavior is that if only the first signature has a comment, then we pick that up; otherwise, if more than one signature has a comment, we only try to pick the comment for the current signature's declaration.

@DanielRosenwasser DanielRosenwasser added Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". Help Wanted You can do this labels Aug 11, 2020
@DanielRosenwasser DanielRosenwasser added this to the Backlog milestone Aug 11, 2020
@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Aug 11, 2020

The feature is working correctly in terms of warnings though - that overload should not be highlighted as a deprecated use, so we don't need a prioritized fix for 4.0.

Marking as Moderate because we'll probably have to workshop the implementation on this.

@IllusionMH
Copy link
Contributor

Maybe make sense not to bring comments that has @deprecated JSDoc tag if signature itself isn't deprecated.

@rsivan
Copy link
Author

rsivan commented Aug 11, 2020

Agree with @IllusionMH

Maybe make sense not to bring comments that has @deprecated JSDoc tag if signature itself isn't deprecated.

@Kingwl
Copy link
Contributor

Kingwl commented Aug 12, 2020

In my memory, It's caused the quick info, completions, etc who used getNodeModifiers only take the jsdoc by first declaration for the signature
CC: @sandersn

@sandersn sandersn removed the Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". label Aug 12, 2020
@sandersn
Copy link
Member

This is a complex problem. It is a duplicate of #407, in particular my last comment on the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Domain: Quick Info e.g. hover text, tool-tips, and tooltips. Help Wanted You can do this Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented.
Projects
None yet
Development

No branches or pull requests

6 participants