fix #3271: bad import-is-undefined warning in ts

Author: evanw

CHANGELOG.md

@@ -1,5 +1,52 @@
 # Changelog
 
+## Unreleased
+
+* Adjust esbuild's warning about undefined imports for TypeScript `import` equals declarations ([#3271](https://github.com/evanw/esbuild/issues/3271))
+
+    In JavaScript, accessing a missing property on an import namespace object is supposed to result in a value of `undefined` at run-time instead of an error at compile-time. This is something that esbuild warns you about by default because doing this can indicate a bug with your code. For example:
+
+    ```js
+    // app.js
+    import * as styles from './styles'
+    console.log(styles.buton)
+    ```
+
+    ```js
+    // styles.js
+    export let button = {}
+    ```
+
+    If you bundle `app.js` with esbuild you will get this:
+
+    ```
+    ▲ [WARNING] Import "buton" will always be undefined because there is no matching export in "styles.js" [import-is-undefined]
+
+        app.js:2:19:
+          2 │ console.log(styles.buton)
+            │                    ~~~~~
+            ╵                    button
+
+      Did you mean to import "button" instead?
+
+        styles.js:1:11:
+          1 │ export let button = {}
+            ╵            ~~~~~~
+    ```
+
+    However, there is TypeScript-only syntax for `import` equals declarations that can represent either a type import (which esbuild should ignore) or a value import (which esbuild should respect). Since esbuild doesn't have a type system, it tries to only respect `import` equals declarations that are actually used as values. Previously esbuild always generated this warning for unused imports referenced within `import` equals declarations even when the reference could be a type instead of a value. Starting with this release, esbuild will now only warn in this case if the import is actually used. Here is an example of some code that no longer causes an incorrect warning:
+
+    ```ts
+    // app.ts
+    import * as styles from './styles'
+    import ButtonType = styles.Button
+    ```
+
+    ```ts
+    // styles.ts
+    export interface Button {}
+    ```
+
 ## 0.18.16
 
 * Fix a regression with whitespace inside `:is()` ([#3265](https://github.com/evanw/esbuild/issues/3265))

internal/bundler_tests/bundler_importstar_test.go

@@ -1668,21 +1668,29 @@ internal-ns-def.js: WARNING: Import "def" will always be undefined because there
 }
 
 func TestImportDefaultNamespaceComboNoDefault(t *testing.T) {
+	// Note: "entry-dead.js" checks that this warning doesn't happen for dead code
 	importstar_suite.expectBundled(t, bundled{
 		files: map[string]string{
 			"/entry-default-ns-prop.js": `import def, * as ns from './foo'; console.log(def, ns, ns.default)`,
 			"/entry-default-ns.js":      `import def, * as ns from './foo'; console.log(def, ns)`,
 			"/entry-default-prop.js":    `import def, * as ns from './foo'; console.log(def, ns.default)`,
 			"/entry-default.js":         `import def from './foo'; console.log(def)`,
 			"/entry-prop.js":            `import * as ns from './foo'; console.log(ns.default)`,
-			"/foo.js":                   `export let foo = 123`,
+			"/entry-dead.js":            `import * as ns from './foo'; 0 && console.log(ns.default)`,
+			"/entry-typo.js":            `import * as ns from './foo'; console.log(ns.buton)`,
+			"/entry-typo-indirect.js":   `import * as ns from './indirect'; console.log(ns.buton)`,
+			"/foo.js":                   `export let button = {}`,
+			"/indirect.js":              `export * from './foo'`,
 		},
 		entryPaths: []string{
 			"/entry-default-ns-prop.js",
 			"/entry-default-ns.js",
 			"/entry-default-prop.js",
 			"/entry-default.js",
 			"/entry-prop.js",
+			"/entry-dead.js",
+			"/entry-typo.js",
+			"/entry-typo-indirect.js",
 		},
 		options: config.Options{
 			Mode:         config.ModeBundle,
@@ -1695,6 +1703,10 @@ entry-default-prop.js: ERROR: No matching export in "foo.js" for import "default
 entry-default-prop.js: WARNING: Import "default" will always be undefined because there is no matching export in "foo.js"
 entry-default.js: ERROR: No matching export in "foo.js" for import "default"
 entry-prop.js: WARNING: Import "default" will always be undefined because there is no matching export in "foo.js"
+entry-typo-indirect.js: WARNING: Import "buton" will always be undefined because there is no matching export in "indirect.js"
+foo.js: NOTE: Did you mean to import "button" instead?
+entry-typo.js: WARNING: Import "buton" will always be undefined because there is no matching export in "foo.js"
+foo.js: NOTE: Did you mean to import "button" instead?
 `,
 	})
 }

internal/bundler_tests/bundler_ts_test.go

@@ -867,6 +867,36 @@ func TestTSImportEqualsBundle(t *testing.T) {
 	})
 }
 
+func TestTSImportEqualsUndefinedImport(t *testing.T) {
+	ts_suite.expectBundled(t, bundled{
+		files: map[string]string{
+			"/entry.ts": `
+				import * as ns from './import.ts'
+				import value_copy = ns.value
+				import Type_copy = ns.Type
+				let foo: Type_copy = value_copy
+				console.log(foo)
+			`,
+			"/import.ts": `
+				export let value = 123
+				export type Type = number
+			`,
+		},
+		entryPaths: []string{"/entry.ts"},
+		options: config.Options{
+			Mode:          config.ModeBundle,
+			AbsOutputFile: "/out.js",
+			ExternalSettings: config.ExternalSettings{
+				PreResolve: config.ExternalMatchers{
+					Exact: map[string]bool{
+						"pkg": true,
+					},
+				},
+			},
+		},
+	})
+}
+
 func TestTSMinifiedBundleES6(t *testing.T) {
 	ts_suite.expectBundled(t, bundled{
 		files: map[string]string{

internal/bundler_tests/snapshots/snapshots_ts.txt

@@ -1491,7 +1491,6 @@ TestTSImportEqualsTreeShakingFalse
 ---------- /out.js ----------
 import { foo } from "pkg";
 const used = foo.used;
-const unused = foo.unused;
 export { used };
 
 ================================================================================
@@ -1501,6 +1500,17 @@ import { foo } from "pkg";
 const used = foo.used;
 export { used };
 
+================================================================================
+TestTSImportEqualsUndefinedImport
+---------- /out.js ----------
+// import.ts
+var value = 123;
+
+// entry.ts
+var value_copy = value;
+var foo = value_copy;
+console.log(foo);
+
 ================================================================================
 TestTSImportMTS
 ---------- /out.js ----------

internal/graph/meta.go

@@ -5,6 +5,7 @@ package graph
 
 import (
 	"github.com/evanw/esbuild/internal/ast"
+	"github.com/evanw/esbuild/internal/helpers"
 	"github.com/evanw/esbuild/internal/js_ast"
 	"github.com/evanw/esbuild/internal/logger"
 )
@@ -92,8 +93,9 @@ type JSReprMeta struct {
 	//
 	// Re-exports come from other files and are the result of resolving export
 	// star statements (i.e. "export * from 'foo'").
-	ResolvedExports    map[string]ExportData
-	ResolvedExportStar *ExportData
+	ResolvedExports     map[string]ExportData
+	ResolvedExportStar  *ExportData
+	ResolvedExportTypos *helpers.TypoDetector
 
 	// Never iterate over "resolvedExports" directly. Instead, iterate over this
 	// array. Some exports in that map aren't meant to end up in generated code.

internal/js_parser/js_parser.go

@@ -15741,11 +15741,18 @@ func (p *parser) checkForUnusedTSImportEquals(s *js_ast.SLocal, result *importsE
 		}
 
 		// Is this an identifier reference and not a require() call?
-		if id, ok := value.Data.(*js_ast.EIdentifier); ok {
+		valueRef := ast.InvalidRef
+		switch v := value.Data.(type) {
+		case *js_ast.EIdentifier:
+			valueRef = v.Ref
+		case *js_ast.EImportIdentifier:
+			valueRef = v.Ref
+		}
+		if valueRef != ast.InvalidRef {
 			// Is this import statement unused?
 			if ref := decl.Binding.Data.(*js_ast.BIdentifier).Ref; p.symbols[ref.InnerIndex].UseCountEstimate == 0 {
 				// Also don't count the referenced identifier
-				p.ignoreUsage(id.Ref)
+				p.ignoreUsage(valueRef)
 
 				// Import-equals statements can come in any order. Removing one
 				// could potentially cause another one to be removable too.

internal/linker/linker.go

@@ -2506,21 +2506,66 @@ loop:
 
 			// Report mismatched imports and exports
 			if symbol.ImportItemStatus == ast.ImportItemGenerated {
-				// This is a debug message instead of an error because although it
-				// appears to be a named import, it's actually an automatically-
-				// generated named import that was originally a property access on an
-				// import star namespace object. Normally this property access would
-				// just resolve to undefined at run-time instead of failing at binding-
-				// time, so we emit a debug message and rewrite the value to the literal
-				// "undefined" instead of emitting an error.
+				// This is not an error because although it appears to be a named
+				// import, it's actually an automatically-generated named import
+				// that was originally a property access on an import star
+				// namespace object:
+				//
+				//   import * as ns from 'foo'
+				//   const undefinedValue = ns.notAnExport
+				//
+				// If this code wasn't bundled, this property access would just resolve
+				// to undefined at run-time instead of failing at binding-time, so we
+				// emit rewrite the value to the literal "undefined" instead of
+				// emitting an error.
 				symbol.ImportItemStatus = ast.ImportItemMissing
-				kind := logger.Warning
-				if helpers.IsInsideNodeModules(trackerFile.InputFile.Source.KeyPath.Text) {
-					kind = logger.Debug
+
+				// Don't emit a log message if this symbol isn't used, since then the
+				// log message isn't helpful. This can happen with "import" assignment
+				// statements in TypeScript code since they are ambiguously either a
+				// type or a value. We consider them to be a type if they aren't used.
+				//
+				//   import * as ns from 'foo'
+				//
+				//   // There's no warning here because this is dead code
+				//   if (false) ns.notAnExport
+				//
+				//   // There's no warning here because this is never used
+				//   import unused = ns.notAnExport
+				//
+				if symbol.UseCountEstimate > 0 {
+					nextFile := &c.graph.Files[nextTracker.sourceIndex].InputFile
+					msg := logger.Msg{
+						Kind: logger.Warning,
+						Data: trackerFile.LineColumnTracker().MsgData(r, fmt.Sprintf(
+							"Import %q will always be undefined because there is no matching export in %q",
+							namedImport.Alias, nextFile.Source.PrettyPath)),
+					}
+					if helpers.IsInsideNodeModules(trackerFile.InputFile.Source.KeyPath.Text) {
+						msg.Kind = logger.Debug
+					}
+
+					// Attempt to correct an import name with a typo
+					repr := nextFile.Repr.(*graph.JSRepr)
+					if repr.Meta.ResolvedExportTypos == nil {
+						valid := make([]string, 0, len(repr.Meta.ResolvedExports))
+						for alias := range repr.Meta.ResolvedExports {
+							valid = append(valid, alias)
+						}
+						sort.Strings(valid)
+						typos := helpers.MakeTypoDetector(valid)
+						repr.Meta.ResolvedExportTypos = &typos
+					}
+					if corrected, ok := repr.Meta.ResolvedExportTypos.MaybeCorrectTypo(namedImport.Alias); ok {
+						msg.Data.Location.Suggestion = corrected
+						export := repr.Meta.ResolvedExports[corrected]
+						importedFile := &c.graph.Files[export.SourceIndex]
+						msg.Notes = append(msg.Notes, importedFile.LineColumnTracker().MsgData(
+							js_lexer.RangeOfIdentifier(importedFile.InputFile.Source, export.NameLoc),
+							fmt.Sprintf("Did you mean to import %q instead?", corrected)))
+					}
+					c.log.AddMsgID(logger.MsgID_Bundler_ImportIsUndefined, msg)
 				}
-				c.log.AddID(logger.MsgID_Bundler_ImportIsUndefined, kind, trackerFile.LineColumnTracker(), r, fmt.Sprintf(
-					"Import %q will always be undefined because there is no matching export in %q",
-					namedImport.Alias, c.graph.Files[nextTracker.sourceIndex].InputFile.Source.PrettyPath))
 			} else {
 				c.log.AddError(trackerFile.LineColumnTracker(), r, fmt.Sprintf("No matching export in %q for import %q",
 					c.graph.Files[nextTracker.sourceIndex].InputFile.Source.PrettyPath, namedImport.Alias))