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

fix(check/lsp): fix bugs with tsc type resolution, allow npm packages to augment ImportMeta #27690

Merged
merged 9 commits into from
Jan 16, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion cli/lsp/tsc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4341,7 +4341,9 @@ impl TscSpecifierMap {
if let Some(specifier) = self.normalized_specifiers.get(original) {
return Ok(specifier.clone());
}
let specifier_str = original.replace(".d.ts.d.ts", ".d.ts");
let specifier_str = original
.replace(".d.ts.d.ts", ".d.ts")
.replace("$node_modules", "node_modules");
Comment on lines -4344 to +4346
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, I kept trying this as a hail mary for some other bugs but could never manifest a case where tsc was constructing paths bypassing op_resolve() (which typically stores mappings for every specifier when denormalizing its results). I guess it's happening at host.resolveTypeReferenceDirectiveReferences().

Nice find!

let specifier = match ModuleSpecifier::parse(&specifier_str) {
Ok(s) => s,
Err(err) => return Err(err),
Expand Down
116 changes: 75 additions & 41 deletions cli/tsc/99_main_compiler.js
Original file line number Diff line number Diff line change
Expand Up @@ -500,6 +500,8 @@ delete Object.prototype.__proto__;
// Microsoft/TypeScript#26825 but that doesn't seem to be working here,
// so we will ignore complaints about this compiler setting.
5070,
// TS6053: File '{0}' not found.
6053,
// TS7016: Could not find a declaration file for module '...'. '...'
// implicitly has an 'any' type. This is due to `allowJs` being off by
// default but importing of a JavaScript module.
Expand Down Expand Up @@ -705,15 +707,14 @@ delete Object.prototype.__proto__;
resolveTypeReferenceDirectiveReferences(
typeDirectiveReferences,
containingFilePath,
redirectedReference,
_redirectedReference,
options,
containingSourceFile,
_reusedNames,
) {
const isCjs =
containingSourceFile?.impliedNodeFormat === ts.ModuleKind.CommonJS;
/** @type {Array<ts.ResolvedTypeReferenceDirectiveWithFailedLookupLocations>} */
const result = typeDirectiveReferences.map((arg) => {
const toResolve = typeDirectiveReferences.map((arg) => {
/** @type {ts.FileReference} */
const fileReference = typeof arg === "string"
? {
Expand All @@ -722,46 +723,50 @@ delete Object.prototype.__proto__;
fileName: arg,
}
: arg;
if (fileReference.fileName.startsWith("npm:")) {
/** @type {[string, ts.Extension | null] | undefined} */
const resolved = ops.op_resolve(
containingFilePath,
[
[
fileReference.resolutionMode == null
? isCjs
: fileReference.resolutionMode === ts.ModuleKind.CommonJS,
fileReference.fileName,
],
],
)?.[0];
if (resolved && resolved[1]) {
return {
resolvedTypeReferenceDirective: {
primary: true,
resolvedFileName: resolved[0],
// todo(dsherret): we should probably be setting this
isExternalLibraryImport: undefined,
},
};
} else {
return {
resolvedTypeReferenceDirective: undefined,
};
}
return [
fileReference.resolutionMode == null
? isCjs
: fileReference.resolutionMode === ts.ModuleKind.CommonJS,
fileReference.fileName,
];
});

/** @type {Array<[string, ts.Extension | null] | undefined>} */
const resolved = ops.op_resolve(
containingFilePath,
toResolve,
);

/** @type {Array<ts.ResolvedTypeReferenceDirectiveWithFailedLookupLocations>} */
const result = resolved.map((item) => {
if (item && item[1]) {
const [resolvedFileName, extension] = item;
return {
resolvedTypeReferenceDirective: {
primary: true,
resolvedFileName,
extension,
isExternalLibraryImport: false,
},
};
} else {
return ts.resolveTypeReferenceDirective(
fileReference.fileName,
containingFilePath,
options,
host,
redirectedReference,
undefined,
containingSourceFile?.impliedNodeFormat ??
fileReference.resolutionMode,
);
return {
resolvedTypeReferenceDirective: undefined,
};
}
});

if (logDebug) {
debug(
"resolveTypeReferenceDirectiveReferences ",
typeDirectiveReferences,
containingFilePath,
options,
containingSourceFile?.fileName,
" => ",
result,
);
}
return result;
},
resolveModuleNameLiterals(
Expand Down Expand Up @@ -1116,6 +1121,36 @@ delete Object.prototype.__proto__;
if (IGNORED_DIAGNOSTICS.includes(diagnostic.code)) {
return false;
}

// ignore diagnostics resulting from the `ImportMeta` declaration in deno merging with
// the one in @types/node. the types of the filename and dirname properties are different,
// which causes tsc to error.
const importMetaFilenameDirnameModifiersRe =
/^All declarations of '(filename|dirname)'/;
const importMetaFilenameDirnameTypesRe =
/^Subsequent property declarations must have the same type.\s+Property '(filename|dirname)'/;
// Declarations of X must have identical modifiers.
if (diagnostic.code === 2687) {
if (
typeof diagnostic.messageText === "string" &&
(importMetaFilenameDirnameModifiersRe.test(diagnostic.messageText)) &&
(diagnostic.file?.fileName.startsWith("asset:///") ||
diagnostic.file?.fileName?.includes("@types/node"))
) {
return false;
}
}
// Subsequent property declarations must have the same type.
if (diagnostic.code === 2717) {
if (
typeof diagnostic.messageText === "string" &&
(importMetaFilenameDirnameTypesRe.test(diagnostic.messageText)) &&
(diagnostic.file?.fileName.startsWith("asset:///") ||
diagnostic.file?.fileName?.includes("@types/node"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work for the global cache?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a test for this (across all of the node modules modes)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I misremembered how the global cache was structured.

) {
return false;
}
}
// make the diagnostic for using an `export =` in an es module a warning
if (diagnostic.code === 1203) {
diagnostic.category = ts.DiagnosticCategory.Warning;
Expand Down Expand Up @@ -1410,7 +1445,6 @@ delete Object.prototype.__proto__;
"ErrorConstructor",
"gc",
"Global",
"ImportMeta",
"localStorage",
"queueMicrotask",
"RequestInit",
Expand Down
79 changes: 73 additions & 6 deletions tests/integration/lsp_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17326,12 +17326,7 @@ fn compiler_options_types() {
let mut deno_json = deno_json.clone();
deno_json["nodeModulesDir"] = json!(node_modules_dir);
temp.write("deno.json", deno_json.to_string());
context
.new_command()
.args("install")
.run()
.skip_output_check()
.assert_exit_code(0);
context.run_deno("install");
client.did_change_watched_files(json!({
"changes": [{
"uri": temp.url().join("deno.json").unwrap(),
Expand All @@ -17345,3 +17340,75 @@ fn compiler_options_types() {
client.did_close_file(&source);
}
}

#[test]
fn type_reference_import_meta() {
let context = TestContextBuilder::for_npm().use_temp_cwd().build();
let mut client = context.new_lsp_command().build();
let temp = context.temp_dir();
let temp_dir = temp.path();
let source = source_file(
temp_dir.join("index.ts"),
r#"
const test = import.meta.env.TEST;
const bar = import.meta.bar;
console.log(test, bar);
"#,
);
/*
tests type reference w/ bare specifier, type reference in an npm package,
and augmentation of `ImportMeta` (this combination modeled after the vanilla vite template,
which uses `vite/client`)

@denotest/augments-global/import-meta:
```dts
/// <reference types="./real-import-meta.d.ts" />

export type Foo = number;
```

real-import-meta.d.ts:
```dts
interface ImportMetaEnv {
TEST: string;
}

interface ImportMeta {
env: ImportMetaEnv;
bar: number;
}
```
*/
temp.write(
"types.d.ts",
r#"
/// <reference types="@denotest/augments-global/import-meta" />
"#,
);

let deno_json = json!({
"imports": {
"@denotest/augments-global": "npm:@denotest/augments-global@1"
}
});
temp.write("deno.json", deno_json.to_string());

client.initialize_default();

for node_modules_dir in ["none", "auto", "manual"] {
let mut deno_json = deno_json.clone();
deno_json["nodeModulesDir"] = json!(node_modules_dir);
temp.write("deno.json", deno_json.to_string());
context.run_deno("install");
client.did_change_watched_files(json!({
"changes": [{
"uri": temp.url().join("deno.json").unwrap(),
"type": 2,
}],
}));

let diagnostics = client.did_open_file(&source);
assert_eq!(diagnostics.all().len(), 0);
client.did_close_file(&source);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
/// <reference types="./real-import-meta.d.ts" />

export type Foo = number;
10 changes: 9 additions & 1 deletion tests/registry/npm/@denotest/augments-global/1.0.0/package.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,13 @@
{
"name": "@denotest/augments-global",
"version": "1.0.0",
"types": "./index.d.ts"
"types": "./index.d.ts",
"exports": {
".": {
"types": "./index.d.ts"
},
"./import-meta": {
"types": "./import-meta.d.ts"
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
interface ImportMetaEnv {
TEST: string;
}

interface ImportMeta {
env: ImportMetaEnv;
bar: number;
}
53 changes: 53 additions & 0 deletions tests/specs/check/import_meta_no_errors/__test__.jsonc
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
{
"tempDir": true,
"tests": {
"node_modules_dir_none": {
"steps": [
{
"args": "run -A ./set_node_modules_dir.ts none",
"output": ""
},
{
"args": "install",
"output": "[WILDCARD]"
},
{
"args": "check --all ./main.ts",
"output": "Check [WILDCARD]main.ts\n"
}
]
},
"node_modules_dir_auto": {
"steps": [
{
"args": "run -A ./set_node_modules_dir.ts auto",
"output": ""
},
{
"args": "install",
"output": "[WILDCARD]"
},
{
"args": "check --all ./main.ts",
"output": "Check [WILDCARD]main.ts\n"
}
]
},
"node_modules_dir_manual": {
"steps": [
{
"args": "run -A ./set_node_modules_dir.ts auto",
"output": ""
},
{
"args": "install",
"output": "[WILDCARD]"
},
{
"args": "check --all ./main.ts",
"output": "Check [WILDCARD]main.ts\n"
}
]
}
}
}
5 changes: 5 additions & 0 deletions tests/specs/check/import_meta_no_errors/deno.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"imports": {
"@types/node": "npm:@types/node@*"
}
}
3 changes: 3 additions & 0 deletions tests/specs/check/import_meta_no_errors/main.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
/// <reference types="@types/node" />

const _foo = import.meta.dirname;
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
if (Deno.args.length !== 1) {
console.error("Usage: set_node_modules_dir.ts <setting>");
Deno.exit(1);
}
const setting = Deno.args[0].trim();
const denoJson = JSON.parse(Deno.readTextFileSync("./deno.json"));
denoJson["nodeModulesDir"] = setting;
Deno.writeTextFileSync("./deno.json", JSON.stringify(denoJson, null, 2));
Loading
Loading