denoland / deno

A modern runtime for JavaScript and TypeScript.
https://deno.com
MIT License
98.01k stars 5.39k forks source link

`@import` (JSDoc) causes type checking to fail silently #26983

Closed FND closed 20 hours ago

FND commented 21 hours ago

As discussed in #25516 (and also on Discord and elsewhere), deno check incorrectly reports type checking was successful when using TypeScript's @import with JSDoc.

This is a discrepancy vis-Γ -vis TypeScript's official tooling (i.e. tsc and the language server used by IDEs).

Combined with the fact that such silent failures are very subtle and hard to identify, this seems like a significant issue.

reduced test case Starting with a single JS file to make sure type checking itself works as expected: πŸ“„ `index.js` ```javascript /** @typedef {string | null} Entry */ /** @type {Entry[]} */ const items = []; items.push("hello"); items.push(null); items.push(123); // πŸ’₯ ``` πŸ“„ `tsconfig.json` ```json { "compilerOptions": { "allowJs": true, "checkJs": true, "noEmit": true, "strict": true } } ``` ```shell $ deno check --config ./tsconfig.json ./index.js error: TS2345 [ERROR]: Argument of type '123' is not assignable to parameter of type 'Entry'. ``` After moving this type definition into a separate file, Deno *falsely* reports everything's fine: πŸ“„ `index.js` ```javascript /** @import { Entry } from "./util.js" */ /** @type {Entry[]} */ const items = []; items.push("hello"); items.push(null); items.push(123); // πŸ’₯ ``` πŸ“„ `util.js` ```javascript /** @typedef {string | null} Entry */ ``` ```shell $ deno check --config ./tsconfig.json ./index.js && echo OK OK ``` If we now replace that `@import` line with an old-style `@typedef` import, Deno correctly reports the type error again: ```javascript /** @typedef {import("./util.js").Entry} Entry */ ```

Version: tested with both Deno 1.46.3 and 2.0.6

dsherret commented 20 hours ago

Tracked in https://github.com/denoland/deno/issues/25516?

FND commented 20 hours ago

🀷 @bartlomieju had asked me on Discord to turn this into a separate issue.

dsherret commented 20 hours ago

Let's track it there. This hasn't been implemented.