Skip to content

Nameless @typedef has implicit any error on the associated identifier #36375

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
TimvdLippe opened this issue Jan 23, 2020 · 9 comments
Closed
Assignees
Labels
Bug A bug in TypeScript Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@TimvdLippe
Copy link
Contributor

TypeScript Version: 3.7.5

Search Terms: typedef, jsdoc, noImplicitAny

Code

/**
 * @typedef {!{data: number}}
 */
export let EventTargetEvent;

Expected behavior:
No error when compiling with noImplicitAny

Actual behavior:
Variable 'EventTargetEvent' implicitly has an 'any' type.
Playground Link: https://www.typescriptlang.org/play/?ssl=1&ssc=1&pln=2&pc=22#code/PQKhCgAIUgBAXAngBwKYBNUDNIG8CEu6AhvMQFyQB2ArgLYBGqATgL6tQjDioAeyAe2bxIAG1QiAogDdUVeABVizAOYSZc+AG4gA

Related Issues: Originally discussed with @sandersn in #19983 who requested a separate issue

@sandersn sandersn changed the title Update noImplicitAny to not error on @typedef jsdoc Nameless @typedef has implicit any error on the associated identifier Jan 23, 2020
@sandersn
Copy link
Member

Currently the error is correct, since the value EventTargetEvent actually does have the type any. The best way to fix this is to decide what type it should have. If I remember correctly, though, Closure code often has initialisers on declarations such that the value has a different type than the type.

@TimvdLippe is there closure code of the form

/** @typedef {{ x: number }} */
export let T = class {
}

where the class does not have an x property? An even smaller example would be export let T = 1, but I don't think I've seen that.

@TimvdLippe
Copy link
Contributor Author

All @typedef that we have, have no initializer. Trying to add an initializer results in the following error:

[JSC_TYPE_MISMATCH] assignment
found   : null
required: None
Components._LinkInfo = null;

@DanielRosenwasser DanielRosenwasser added Working as Intended The behavior described is the intended behavior; this is not a bug and removed Working as Intended The behavior described is the intended behavior; this is not a bug labels Jan 23, 2020
@TimvdLippe
Copy link
Contributor Author

I looked at the implementation of the compiler and it seems rather straightforward to fix this. E.g. it would check, during variable initialization, if the jsdoc has a typedef. If so, set the variable to unknown, as usages of a variable with a typedef is illegal (but you should still be able to refer to it in jsdoc).

Do you mind if I try to create a PR for this issue with the above semantics?

TimvdLippe added a commit to TimvdLippe/TypeScript that referenced this issue Jan 27, 2020
Typedefs have two forms: specify a name in the `@typedef` declaration or
use the name of the variable declaration it is attached to. Since
`@typedef` types should only live in the type space, any reference to a
variable that has a `@typedef` attached to is illegal. As such, assign
it the `never` type if the `@typedef` itself does not specify a name.

Fixes microsoft#36375
@TimvdLippe
Copy link
Contributor Author

WIP PR at #36454 which passes all tests.

@RyanCavanaugh
Copy link
Member

@sandersn what's the desired behavior?

@sandersn
Copy link
Member

  • I think Infer typedef variable declaration to never #36454, which sets the type to never, is pretty good because it makes it hard to use.
  • You could make value-space usage completely illegal, but this is stricter than our other JS handling.
  • The current behaviour of any is not too bad either, just annoying in the case that noImplicitAny is on. It means that nameless typedefs will always have an implicit any error, with the only workaround being to add a name.

I don't think this bug is worth fixing, though, because the new code only benefits (1) closure code bases (2) with noImplicitAny: true. I don't believe there are many of these. I don't think that justifies the support cost of the new code.

@sandersn
Copy link
Member

Looks like #28162 includes the error I suggested, plus a lot more, already.

@RyanCavanaugh RyanCavanaugh added the Bug A bug in TypeScript label Feb 26, 2020
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 3.9.1 milestone Feb 26, 2020
@sandersn sandersn added the Working as Intended The behavior described is the intended behavior; this is not a bug label Apr 15, 2020
@sandersn
Copy link
Member

I still don't think this bug is worth fixing, for the same reason I gave before: compiling closure code bases with noImplicitAny: true is not a scenario that we've planned for. Instead I think it's better to convert to TS with noImplicitAny: false, and then follow up by turning on strict flags one at a time. Alternatively, automatic insertion of // @ts-ignore SEARCHABLE REASON HERE will squelch the errors until the conversion is done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants