Skip to content

Validate JSON imports into ESM in --module nodenext#60019

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.

Sign up for GitHub

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

Merged
Andrew Branch (andrewbranch) merged 3 commits into from
Sep 24, 2024
Merged

Validate JSON imports into ESM in --module nodenext#60019

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d75da77
WIP
andrewbranch Sep 19, 2024
cc52223
Validate JSON imports in nodenext ESM
andrewbranch Sep 20, 2024
15b0406
Update older tests
andrewbranch Sep 20, 2024
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
33 changes: 25 additions & 8 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -163,7 +163,6 @@ import {
EmitResolver,
EmitTextWriter,
emptyArray,
endsWith,
EntityName,
EntityNameExpression,
EntityNameOrEntityNameExpression,
Expand DownExpand Up@@ -265,6 +264,7 @@ import {
getContainingClassStaticBlock,
getContainingFunction,
getContainingFunctionOrClassStaticBlock,
getDeclarationFileExtension,
getDeclarationModifierFlagsFromSymbol,
getDeclarationOfKind,
getDeclarationsOfKind,
Expand DownExpand Up@@ -3700,11 +3700,15 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
return usageMode === ModuleKind.ESNext && targetMode === ModuleKind.CommonJS;
}

function isOnlyImportableAsDefault(usage: Expression) {
function isOnlyImportableAsDefault(usage: Expression, resolvedModule?: Symbol) {
// In Node.js, JSON modules don't get named exports
if (ModuleKind.Node16 <= moduleKind && moduleKind <= ModuleKind.NodeNext) {
const usageMode = getEmitSyntaxForModuleSpecifierExpression(usage);
return usageMode === ModuleKind.ESNext && endsWith((usage as StringLiteralLike).text, Extension.Json);

@andrewbranchAndrew Branch (andrewbranch)Sep 20, 2024

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

The text of the module specifier is not sufficient to know whether we're importing a JSON file; this produced both false positives and false negatives (see imports of "actually-json" and "not.json" in the test)

if (usageMode === ModuleKind.ESNext) {
resolvedModule ??= resolveExternalModuleName(usage, usage, /*ignoreErrors*/ true);
const targetFile = resolvedModule && getSourceFileOfModule(resolvedModule);
return targetFile && (isJsonSourceFile(targetFile) || getDeclarationFileExtension(targetFile.fileName) === ".d.json.ts");
}
}
return false;
}
Expand DownExpand Up@@ -3774,7 +3778,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
if (!specifier) {
return exportDefaultSymbol;
}
const hasDefaultOnly = isOnlyImportableAsDefault(specifier);
const hasDefaultOnly = isOnlyImportableAsDefault(specifier, moduleSymbol);
const hasSyntheticDefault = canHaveSyntheticDefault(file, moduleSymbol, dontResolveAlias, specifier);
if (!exportDefaultSymbol && !hasSyntheticDefault && !hasDefaultOnly) {
if (hasExportAssignmentSymbol(moduleSymbol) && !allowSyntheticDefaultImports) {
Expand DownExpand Up@@ -3959,15 +3963,19 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
let symbolFromModule = getExportOfModule(targetSymbol, nameText, specifier, dontResolveAlias);
if (symbolFromModule === undefined && nameText === InternalSymbolName.Default) {
const file = moduleSymbol.declarations?.find(isSourceFile);
if (isOnlyImportableAsDefault(moduleSpecifier) || canHaveSyntheticDefault(file, moduleSymbol, dontResolveAlias, moduleSpecifier)) {
if (isOnlyImportableAsDefault(moduleSpecifier, moduleSymbol) || canHaveSyntheticDefault(file, moduleSymbol, dontResolveAlias, moduleSpecifier)) {
symbolFromModule = resolveExternalModuleSymbol(moduleSymbol, dontResolveAlias) || resolveSymbol(moduleSymbol, dontResolveAlias);
}
}

const symbol = symbolFromModule && symbolFromVariable && symbolFromModule !== symbolFromVariable ?
combineValueAndTypeSymbols(symbolFromVariable, symbolFromModule) :
symbolFromModule || symbolFromVariable;
if (!symbol) {

if (isImportOrExportSpecifier(specifier) && isOnlyImportableAsDefault(moduleSpecifier, moduleSymbol) && nameText !== InternalSymbolName.Default) {
error(name, Diagnostics.Named_imports_from_a_JSON_file_into_an_ECMAScript_module_are_not_allowed_when_module_is_set_to_0, ModuleKind[moduleKind]);
}
else if (!symbol) {
errorNoModuleMemberSymbol(moduleSymbol, targetSymbol, node, name);
}
return symbol;
Expand DownExpand Up@@ -47675,6 +47683,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
grammarErrorOnFirstToken(node, Diagnostics.An_import_declaration_cannot_have_modifiers);
}
if (checkExternalImportOrExportDeclaration(node)) {
let resolvedModule;
const importClause = node.importClause;
if (importClause && !checkGrammarImportClause(importClause)) {
if (importClause.name) {
Expand All@@ -47689,12 +47698,16 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
}
}
else {
const moduleExisted = resolveExternalModuleName(node, node.moduleSpecifier);
if (moduleExisted) {
resolvedModule = resolveExternalModuleName(node, node.moduleSpecifier);
if (resolvedModule) {
forEach(importClause.namedBindings.elements, checkImportBinding);
}
}
}

if (isOnlyImportableAsDefault(node.moduleSpecifier, resolvedModule) && !hasTypeJsonImportAttribute(node)) {
error(node.moduleSpecifier, Diagnostics.Importing_a_JSON_file_into_an_ECMAScript_module_requires_a_type_Colon_json_import_attribute_when_module_is_set_to_0, ModuleKind[moduleKind]);
}
}
else if (noUncheckedSideEffectImports && !importClause) {
void resolveExternalModuleName(node, node.moduleSpecifier);
Expand All@@ -47703,6 +47716,10 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
checkImportAttributes(node);
}

function hasTypeJsonImportAttribute(node: ImportDeclaration) {
return !!node.attributes && node.attributes.elements.some(attr => getTextOfIdentifierOrLiteral(attr.name) === "type" && tryCast(attr.value, isStringLiteralLike)?.text === "json");
}

function checkImportEqualsDeclaration(node: ImportEqualsDeclaration) {
if (checkGrammarModuleElementContext(node, isInJSFile(node) ? Diagnostics.An_import_declaration_can_only_be_used_at_the_top_level_of_a_module : Diagnostics.An_import_declaration_can_only_be_used_at_the_top_level_of_a_namespace_or_module)) {
// If we hit an import declaration in an illegal context, just bail out to avoid cascading errors.
Expand Down
8 changes: 8 additions & 0 deletions src/compiler/diagnosticMessages.json
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -1826,6 +1826,14 @@
"category": "Error",
"code": 1542
},
"Importing a JSON file into an ECMAScript module requires a 'type: \"json\"' import attribute when 'module' is set to '{0}'.": {
"category": "Error",
"code": 1543
},
"Named imports from a JSON file into an ECMAScript module are not allowed when 'module' is set to '{0}'.": {
"category": "Error",
"code": 1544
},

"The types of '{0}' are incompatible between these types.": {
"category": "Error",
Expand Down
5 changes: 3 additions & 2 deletions src/compiler/parser.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -10517,9 +10517,10 @@ export function getDeclarationFileExtension(fileName: string): string | undefine
return standardExtension;
}
if (fileExtensionIs(fileName, Extension.Ts)) {
const index = getBaseFileName(fileName).lastIndexOf(".d.");
const baseName = getBaseFileName(fileName);
const index = baseName.lastIndexOf(".d.");
if (index >= 0) {
return fileName.substring(index);
return baseName.substring(index);

@andrewbranchAndrew Branch (andrewbranch)Sep 20, 2024

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This was extremely bad; I guess we were only really using this to determine whether something was a declaration file name, not to actually use the extension.

}
}
return undefined;
Expand Down
78 changes: 78 additions & 0 deletions tests/baselines/reference/nodeModulesJson.errors.txt
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,78 @@
/loosey.cts(1,36): error TS2856: Import attributes are not allowed on statements that compile to CommonJS 'require' calls.
/loosey.cts(6,9): error TS2339: Property 'default' does not exist on type '{ version: number; }'.
/main.mts(2,22): error TS1543: Importing a JSON file into an ECMAScript module requires a 'type: "json"' import attribute when 'module' is set to 'NodeNext'.
/main.mts(3,19): error TS1543: Importing a JSON file into an ECMAScript module requires a 'type: "json"' import attribute when 'module' is set to 'NodeNext'.
/main.mts(7,21): error TS1543: Importing a JSON file into an ECMAScript module requires a 'type: "json"' import attribute when 'module' is set to 'NodeNext'.
/main.mts(8,10): error TS1544: Named imports from a JSON file into an ECMAScript module are not allowed when 'module' is set to 'NodeNext'.
/main.mts(10,9): error TS2339: Property 'version' does not exist on type '{ default: { version: number; }; }'.


==== /node_modules/not.json/package.json (0 errors) ====
{
"name": "not.json",
"version": "1.0.0",
"type": "module",
"exports": "./index.js"
}

==== /node_modules/not.json/index.d.ts (0 errors) ====
export function oops(json: string): any;

==== /node_modules/actually-json/package.json (0 errors) ====
{
"name": "actually-json",
"version": "1.0.0",
"type": "module",
"exports": {
".": "./index.json",
"./typed": "./typed.d.json.ts"
}
}

==== /node_modules/actually-json/index.json (0 errors) ====
{}

==== /node_modules/actually-json/typed.d.json.ts (0 errors) ====
declare const _default: {};
export default _default;

==== /config.json (0 errors) ====
{
"version": 1
}

==== /main.mts (5 errors) ====
import { oops } from "not.json"; // Ok
import moreOops from "actually-json"; // Error
~~~~~~~~~~~~~~~
!!! error TS1543: Importing a JSON file into an ECMAScript module requires a 'type: "json"' import attribute when 'module' is set to 'NodeNext'.
import typed from "actually-json/typed"; // Error
~~~~~~~~~~~~~~~~~~~~~
!!! error TS1543: Importing a JSON file into an ECMAScript module requires a 'type: "json"' import attribute when 'module' is set to 'NodeNext'.

import config from "./config.json" with { type: "json" }; // Ok
import { default as config1 } from "./config.json" with { type: "json" }; // Ok
import config2 from "./config.json"; // Error, no attribute
~~~~~~~~~~~~~~~
!!! error TS1543: Importing a JSON file into an ECMAScript module requires a 'type: "json"' import attribute when 'module' is set to 'NodeNext'.
import { version } from "./config.json" with { type: "json" }; // Error, named import
~~~~~~~
!!! error TS1544: Named imports from a JSON file into an ECMAScript module are not allowed when 'module' is set to 'NodeNext'.
import * as config3 from "./config.json" with { type: "json" };
config3.version; // Error
~~~~~~~
!!! error TS2339: Property 'version' does not exist on type '{ default: { version: number; }; }'.
config3.default; // Ok

==== /loosey.cts (2 errors) ====
import config from "./config.json" with { type: "json" }; // Error
~~~~~~~~~~~~~~~~~~~~~
!!! error TS2856: Import attributes are not allowed on statements that compile to CommonJS 'require' calls.
import config2 from "./config.json"; // Ok
import { version } from "./config.json"; // Ok
import * as config3 from "./config.json";
config3.version; // Ok
config3.default; // Error
~~~~~~~
!!! error TS2339: Property 'default' does not exist on type '{ version: number; }'.

79 changes: 79 additions & 0 deletions tests/baselines/reference/nodeModulesJson.symbols
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
//// [tests/cases/conformance/node/nodeModulesJson.ts] ////

=== /node_modules/not.json/index.d.ts ===
export function oops(json: string): any;
>oops : Symbol(oops, Decl(index.d.ts, 0, 0))
>json : Symbol(json, Decl(index.d.ts, 0, 21))

=== /node_modules/actually-json/index.json ===

{}

=== /node_modules/actually-json/typed.d.json.ts ===
declare const _default: {};
>_default : Symbol(_default, Decl(typed.d.json.ts, 0, 13))

export default _default;
>_default : Symbol(_default, Decl(typed.d.json.ts, 0, 13))

=== /config.json ===
{
"version": 1
>"version" : Symbol("version", Decl(config.json, 0, 1))
}

=== /main.mts ===
import { oops } from "not.json"; // Ok
>oops : Symbol(oops, Decl(main.mts, 0, 8))

import moreOops from "actually-json"; // Error
>moreOops : Symbol(moreOops, Decl(main.mts, 1, 6))

import typed from "actually-json/typed"; // Error
>typed : Symbol(typed, Decl(main.mts, 2, 6))

import config from "./config.json" with { type: "json" }; // Ok
>config : Symbol(config, Decl(main.mts, 4, 6))

import { default as config1 } from "./config.json" with { type: "json" }; // Ok
>default : Symbol(config, Decl(config.json, 0, 0))
>config1 : Symbol(config1, Decl(main.mts, 5, 8))

import config2 from "./config.json"; // Error, no attribute
>config2 : Symbol(config2, Decl(main.mts, 6, 6))

import { version } from "./config.json" with { type: "json" }; // Error, named import
>version : Symbol(version, Decl(main.mts, 7, 8))

import * as config3 from "./config.json" with { type: "json" };
>config3 : Symbol(config3, Decl(main.mts, 8, 6))

config3.version; // Error
>config3 : Symbol(config3, Decl(main.mts, 8, 6))

config3.default; // Ok
>config3.default : Symbol("/config")
>config3 : Symbol(config3, Decl(main.mts, 8, 6))
>default : Symbol("/config")

=== /loosey.cts ===
import config from "./config.json" with { type: "json" }; // Error
>config : Symbol(config, Decl(loosey.cts, 0, 6))

import config2 from "./config.json"; // Ok
>config2 : Symbol(config2, Decl(loosey.cts, 1, 6))

import { version } from "./config.json"; // Ok
>version : Symbol(version, Decl(loosey.cts, 2, 8))

import * as config3 from "./config.json";
>config3 : Symbol(config3, Decl(loosey.cts, 3, 6))

config3.version; // Ok
>config3.version : Symbol(version, Decl(config.json, 0, 1))
>config3 : Symbol(config3, Decl(loosey.cts, 3, 6))
>version : Symbol(version, Decl(config.json, 0, 1))

config3.default; // Error
>config3 : Symbol(config3, Decl(loosey.cts, 3, 6))

Loading