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

Property assignment in constructor with JSDoc @param wrongly reports type as any #61005

Closed
gthb opened this issue Jan 20, 2025 · 6 comments
Closed
Labels
Duplicate An existing issue was already created

Comments

@gthb
Copy link

gthb commented Jan 20, 2025

Type: Bug

In this code:

class Foo {
  /** @param {number} bar */
  constructor (bar) {
    this.bar = bar;
  }
}

const foo = new Foo(37);
console.log(foo.bar);

the type of property bar is inferred from the parameter type via the constructor assignment, as verified by:

  • VS Code showing (property) Foo.bar: number when I hover on foo.bar in the last line
  • tsc --noImplicitAny making no complaints

but VS Code shows (property) Foo.bar: any when I hover on this.bar in the constructor.

I think that's simply a bug — at least I can't think of a reason it ought to be that way, and it has misled me into spending time troubleshooting inference or explicitly declaring the type.

Seen originally in latest stable VS Code and now also in VS Code Insiders with no extensions.

VS Code version: Code - Insiders 1.97.0-insider (Universal) (3d0aeb47a2ecfde9ff5141470b30c36d41c321d9, 2025-01-20T05:04:25.114Z)
OS version: Darwin arm64 24.2.0
Modes:

System Info
Item Value
CPUs Apple M1 Pro (10 x 2400)
GPU Status 2d_canvas: enabled
canvas_oop_rasterization: enabled_on
direct_rendering_display_compositor: disabled_off_ok
gpu_compositing: enabled
multiple_raster_threads: enabled_on
opengl: enabled_on
rasterization: enabled
raw_draw: disabled_off_ok
skia_graphite: disabled_off
video_decode: enabled
video_encode: enabled
webgl: enabled
webgl2: enabled
webgpu: enabled
webnn: disabled_off
Load (avg) 7, 6, 5
Memory (System) 32.00GB (0.72GB free)
Process Argv
Screen Reader no
VM 0%
Extensions: none
A/B Experiments
vsliv368cf:30146710
vspor879:30202332
vspor708:30202333
vspor363:30204092
vscod805:30301674
vsaa593cf:30376535
py29gd2263:31024238
c4g48928:30535728
2i9eh265:30646982
962ge761:30841072
pythonnoceb:30776497
dsvsc014:30777825
dsvsc015:30821418
pythonmypyd1:30859725
h48ei257:31000450
pythontbext0:30879054
cppperfnew:30980852
pythonait:30973460
dvdeprecation:31040973
dwnewjupyter:31046869
nativerepl1:31134653
pythonrstrctxt:31093868
nativeloc1:31118317
cf971741:31144450
e80f6927:31120813
iacca1:31150324
notype1:31143044
dwcopilot:31158714
h409b430:31177054
5b1c1929:31184661
6074i472:31201624
dwoutputs:31217127
6e63a538:31218797

@mjbvz mjbvz transferred this issue from microsoft/vscode Jan 21, 2025
@mjbvz mjbvz removed their assignment Jan 21, 2025
@Andarist
Copy link
Contributor

It's not incorrect, within the constructor this.bar here is an "auto type". The property's type has not yet been fixed at this point because, in a sense, this is its declaration and it's not annotated etc. See how the type of this property can "evolve" on this playground: TS playground

I admit this is surprising though when you don't know about those auto/evolving types, see #54414

@gthb
Copy link
Author

gthb commented Jan 22, 2025

It's not incorrect, within the constructor this.bar here is an "auto type". The property's type has not yet been fixed at this point because, in a sense, this is its declaration and it's not annotated etc. [...]

I admit this is surprising though when you don't know about those auto/evolving types, see #54414

Understood, thanks! But the terms “auto type” and “evolving type” are not found in the typescriptlang.org search, so this is an internal detail (or at least the terminology is). So this does sound like it's “correct” only in an overly narrow sense (which is probably what led you to phrase it as “not incorrect” :) ) ... narrow meaning:

  1. it is “an expected consequence of an internal design”, rather than “the consistent and useful result for end users”
  2. while it is correct that within the constructor this property can still have anything assigned to it without violating type rules, that still doesn't mean the property's type is any — rather, the property's type is some type (which may well be narrower than any) which is “not yet determined here”.
  3. it is not so correct as to be consistent with tsc --noImplicitAny accepting this without comment (even though the any, if it were true, is very much implicit)

When the constructor does assign the property in different places with different types (like in your playground example), a more consistent and useful behavior would be to show the type as:

  1. auto or evolving or TBD or not known until constructor completes or something like that — vague, but more truthful than any
  2. or the result of completing the evolution, i.e. the same as is shown for that property when referenced outside the constructor — but (a) maybe this is hard to implement or not even feasible in general, and (b) maybe it can be misleading in its own way, for an end user reasoning about the code that's involved in the type's evolution
  3. or a mix of both: showing the type that will result at the end of analyzing the constructor, along with a note that this is an auto/evolving type

Note that I originally reported this in https://github.com/microsoft/vscode/, because this is a matter of how type information is expressed to an end user in VS Code (not of how code is analyzed locally in the constructor) ... so it can make use of information known at the end of type analysis (i.e. it can know the result of the type evolution, and express that to the user).

@gthb
Copy link
Author

gthb commented Jan 22, 2025

a more consistent and useful behavior would be to show the type as:

... oh, right, I guess this is pretty much what is proposed in #54414 :) ... so maybe this is just a duplicate.

@gthb
Copy link
Author

gthb commented Jan 22, 2025

... oh, right, I guess this is pretty much what is proposed in #54414 :) ... so maybe this is just a duplicate.

Well, except #54414 focuses on arrays. The case of JS class properties whose existence and type is inferred from assignments in the constructor is maybe more tractable, because the type is presumably auto/evolving only within the constructor body, right?

@Andarist
Copy link
Contributor

That’s the same DX problem really. The „scope” in which this is allowed to evolve doesn’t quite matter if we think about the implementation complexities involved with improving this

@RyanCavanaugh RyanCavanaugh added the Duplicate An existing issue was already created label Jan 24, 2025
@typescript-bot
Copy link
Collaborator

This issue has been marked as "Duplicate" and has seen no recent activity. It has been automatically closed for house-keeping purposes.

@typescript-bot typescript-bot closed this as not planned Won't fix, can't repro, duplicate, stale Jan 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

5 participants