Fix assertion violation in auto-imports for JSX tag when React is type-imported#3650
Fix assertion violation in auto-imports for JSX tag when React is type-imported#3650
Conversation
…SX tags When a JSX tag triggers the "cannot be used as a value because it was imported using import type" error, getSymbolNamesToImport may return multiple names (the component name AND the JSX namespace like React). Instead of panicking when there are multiple names, iterate over all names attempting type-only promotion for each, and fall back to regular auto-import logic for names where promotion is not applicable. Agent-Logs-Url: https://github.com/microsoft/typescript-go/sessions/d390d5fd-644f-4031-8a82-ca0441ed46cb Co-authored-by: DanielRosenwasser <972891+DanielRosenwasser@users.noreply.github.com>
|
Are each of these discrete fixes? Can they not be combined? Aside, I noticed that |
andrewbranch
left a comment
There was a problem hiding this comment.
Huh yeah let's actually fix the behavior instead of doing something weird that doesn't panic. The question is, why is there more than one symbol name here?
|
@copilot ^ |
…import fallback For the type-only promotion error, iterate over symbol names from getSymbolNamesToImport and return the first valid promotion fix. Other missing symbols (like unresolved JSX component names) have their own separate error diagnostics that trigger auto-import independently. Agent-Logs-Url: https://github.com/microsoft/typescript-go/sessions/f28dae04-ed90-4058-926b-6ad8d3234cec Co-authored-by: andrewbranch <3277153+andrewbranch@users.noreply.github.com>
Multiple symbol names come from Simplified in 8134793 to just iterate over the names and return the first valid promotion fix. No auto-import fallback — other missing symbols get their own error diagnostics.
Simplified in 8134793 — now only returns the single promotion fix (no more separate fixes to combine). The auto-import for the missing component comes from its own "Cannot find name" error diagnostic, not from this code path. Re |
| `import type React from "./react"; | ||
| import React from "./react"; |
There was a problem hiding this comment.
This should not be offered as a fix. This code is silly. I understand it may be part of a different code path but we may as well make this whole test behave like it should.
There was a problem hiding this comment.
Fixed in cfe5f6c. The silly import type React + import React fix is no longer offered. Changed needsJsxNamespaceFix to return false for type-only imports, since the promotion path handles them separately. The test now expects only the single promotion fix.
| for _, symbolName := range symbolNames { | ||
| fix := getTypeOnlyPromotionFix(ctx, fixContext.SourceFile, symbolToken, symbolName, fixContext.Program) | ||
| if fix != nil { | ||
| return []*fixInfo{{fix: fix, symbolName: symbolName, errorIdentifierText: symbolToken.Text()}}, nil |
There was a problem hiding this comment.
This seems a little sketchy. If I'm reading the situation right, the deal is that you have an error '...it was imported using import type', but by looking at the error position information alone, you actually don't know whether the error is talking about React or Foo (the JSX component name). Right now, you are just blindly trying to generate a promotion fix for each. But what about the edge case where both Foo and React are imported with import type already? I think the client would request a fix for each diagnostic (or would request fixes at that location and we would see both diagnostics, and run this code for each one), but the fix implementation would return the same fix for the two different instances of the diagnostic (whichever identifier happened to be returned first by getSymbolNamesToImport).
I am loathe to suggest that getSymbolNamesToImport parse out the identifier from the error message. We could add something onto lsproto.Diagnostic.Data but I don't want to overcomplicate this. Why don't you add a test where both the component name and React are imported as type-only, request fixes at that location, and see if this is indeed a problem.
There was a problem hiding this comment.
Fixed in cfe5f6c. The type-only promotion path now directly determines JSX candidates (token text + JSX namespace) instead of relying on getSymbolNamesToImport.
Added TestCodeFixPromoteTypeOnlyImportJsxTagBothTypeOnly for the edge case where both Foo and React are type-only imported. As you predicted, since we can't match diagnostic to symbol without parsing the error message, both diagnostics produce both promotion fixes, resulting in duplicates (4 fixes, 2 unique). The test documents this as a known limitation.
|
@copilot ^ |
- Type-only promotion path: directly determine JSX candidates (token text + JSX namespace) instead of using getSymbolNamesToImport, which has auto-import filtering logic that doesn't apply - needsJsxNamespaceFix: return false for type-only imports since the promotion path handles them separately, preventing silly duplicate imports - getFixesInfoForNonUMDImport: set isJsxNamespaceFix to match Strada - Add edge case test where both Foo and React are type-only imported Agent-Logs-Url: https://github.com/microsoft/typescript-go/sessions/a4d3e15d-7971-430e-99c5-81a0ae184f30 Co-authored-by: andrewbranch <3277153+andrewbranch@users.noreply.github.com>
|
@copilot your inline checks look less sophisticated than
From there, if both symbols resolve to a type-only import, we can easily choose between just providing both fixes, resulting in duplicates like the current test shows (not the end of the world, better than crashing or omitting a fix) or correlating the symbol name to the error message, whether through string matching or add-on data later. |
…flag Modify getSymbolNamesToImport to return symbolNameInfo structs with name and isTypeOnly fields. This centralizes the JSX checking logic and allows callers to properly filter: - Type-only promotion path: filters to isTypeOnly symbols only - Auto-import path: skips isTypeOnly symbols (handled by promotion) Also reverts needsJsxNamespaceFix to match Strada behavior (includes type-only imports in the candidate list) and includes component name when it resolves as type-only (for the promotion path to use). Agent-Logs-Url: https://github.com/microsoft/typescript-go/sessions/d5749c27-5468-436b-adb5-cfe32e9f8e35 Co-authored-by: andrewbranch <3277153+andrewbranch@users.noreply.github.com>
Refactored in 32a7b79. |
| f.GoToMarker(t, "") | ||
| // Both Foo and React are type-only imported. Each of the two diagnostics | ||
| // produces promotion fixes for both symbols (since we can't match diagnostic | ||
| // to symbol without parsing the error message), so we get duplicates. |
There was a problem hiding this comment.
@copilot Maybe we should do a best-effort simple error message string matching for contains '${jsxNamespaceName}' and '${tokenText}' to disambiguate (single quotes included)
There was a problem hiding this comment.
Implemented in 4fa2ea3. The type-only promotion path now checks fixContext.Diagnostic.Message for '<symbolName>' (single-quoted) to disambiguate which symbol the diagnostic is about. The both-type-only test no longer produces duplicates — each diagnostic now produces only its own promotion fix.
Use the diagnostic message to correlate which symbol each type-only promotion error is about. The message format is "'X' cannot be used as a value because it was imported using 'import type'." so we check for the symbol name in single quotes. This eliminates duplicate fixes when both component and JSX namespace are type-only imported. Agent-Logs-Url: https://github.com/microsoft/typescript-go/sessions/eb5910bf-d44c-483e-bdb5-5b6b250eb6e7 Co-authored-by: andrewbranch <3277153+andrewbranch@users.noreply.github.com>
When a JSX tag triggers the
_0_cannot_be_used_as_a_value_because_it_was_imported_using_import_typeerror,getSymbolNamesToImportcan return multiple names (e.g., both the component name and the JSX namespace). The code had a hardpanicrequiring exactly one name.This happens because
getSymbolNamesToImportis a general-purpose function that returns all names needed for a JSX tag (e.g.,["Foo", "React"]), but for the type-only promotion error, only the type-imported symbol (React) needs promotion. The component name (Foo) isn't imported at all — it has its own separate "Cannot find name" error diagnostic.getSymbolNamesToImportto return richersymbolNameInfostructs containing the symbol name and anisTypeOnlyflag (determined viaGetTypeOnlyAliasDeclaration). This centralizes the resolution logic and allows callers to properly filter:isTypeOnlysymbols and generates promotion fixes for each.isTypeOnlysymbols (handled by promotion), preventing sillyimport type React+import Reactduplicates.'<symbolName>'in single quotes) to correlate each diagnostic to its symbol, eliminating duplicate fixes.needsJsxNamespaceFixreverted to Strada behavior: Now correctly includes type-only imports in the candidate list (the filtering happens at the call site viaisTypeOnly).getSymbolNamesToImportnow includes the component name when it resolves as type-only (not just when unresolved), so the promotion path can generate fixes for it.isJsxNamespaceFixset:getFixesInfoForNonUMDImportnow setsisJsxNamespaceFixto match Strada (symbolName != symbolToken.Text()).