Port PR #63200: Add symbol name to unsafe import error (TS2883)#2931
Port PR #63200: Add symbol name to unsafe import error (TS2883)#2931
Conversation
Add new diagnostic TS2883 that includes the symbol name in the error message for unsafe import references. Update SymbolTracker interface and implementations to pass through the symbol name parameter. Co-authored-by: gabritto <19519347+gabritto@users.noreply.github.com>
Co-authored-by: gabritto <19519347+gabritto@users.noreply.github.com>
Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Ports TypeScript PR #63200 to improve the unsafe import diagnostic by including the referenced symbol’s name when available (new TS2883), updating tracker interfaces/implementations accordingly, and refreshing affected test baselines.
Changes:
- Added/used a new diagnostic (TS2883) that includes the referenced symbol name in the message.
- Updated
SymbolTrackerAPI and bothSymbolTrackerImplimplementations to threadsymbolNamethrough. - Updated multiple compiler baseline files to reflect the improved error text/code.
Reviewed changes
Copilot reviewed 33 out of 33 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| testdata/baselines/reference/tsc/declarationEmit/when-pkg-references-sibling-package-through-indirect-symlink.js | Baseline update to TS2883 with symbol name included. |
| testdata/baselines/reference/submodule/conformance/nodeModulesExportsSourceTs(module=...).errors.txt(.diff) | Baselines updated to TS2883 message variant. |
| testdata/baselines/reference/submodule/conformance/nodeModulesExportsBlocksSpecifierResolution(module=...).errors.txt(.diff) | Baselines updated to TS2883 message variant. |
| testdata/baselines/reference/submodule/conformance/legacyNodeModulesExportsSpecifierGenerationConditions.errors.txt(.diff) | Baseline updated to TS2883 message variant. |
| testdata/baselines/reference/submodule/compiler/declarationEmitUsingTypeAlias1.errors.txt(.diff) | Baseline updated; now shows multiple TS2883 instances at same span. |
| testdata/baselines/reference/submodule/compiler/declarationEmitUnsafeImportSymbolName.errors.txt(.diff) | Baseline updated to TS2883 message variant. |
| testdata/baselines/reference/submodule/compiler/declarationEmitReexportedSymlinkReference3.errors.txt(.diff) | Baseline updated to TS2883 message variant. |
| testdata/baselines/reference/submodule/compiler/declarationEmitObjectAssignedDefaultExport.errors.txt(.diff) | Baseline updated to TS2883 message variant. |
| testdata/baselines/reference/submodule/compiler/declarationEmitCommonJsModuleReferencedType.errors.txt(.diff) | Baseline updated to TS2883 message variant. |
| internal/transformers/declarations/tracker.go | Chooses TS2883 when symbolName is provided; otherwise falls back to TS2742. |
| internal/nodebuilder/types.go | Updates SymbolTracker interface method signature to accept symbolName. |
| internal/checker/symboltracker.go | Threads symbolName through checker’s SymbolTrackerImpl wrapper. |
| internal/checker/nodebuilderimpl.go | Passes a guarded symbol name into ReportLikelyUnsafeImportRequiredError. |
internal/checker/nodebuilderimpl.go
Outdated
| symbolName := "" | ||
| if symbol != nil { | ||
| symbolName = symbol.Name | ||
| } |
There was a problem hiding this comment.
Passing a varying symbolName here can cause multiple distinct TS2883 diagnostics at the same source span (since message args differ), which shows up in the updated baseline (e.g. declarationEmitUsingTypeAlias1.errors.txt reports two TS2883 errors for bar at the same (7,14)). If only one diagnostic per declaration is intended, consider deduping reports (e.g., per error location + specifier) or choosing a single canonical symbol name to report so repeated calls don’t produce extra distinct diagnostics.
| symbolName := "" | |
| if symbol != nil { | |
| symbolName = symbol.Name | |
| } | |
| // Use a canonical symbol name to avoid multiple distinct diagnostics for the same location. | |
| symbolName := "" |
There was a problem hiding this comment.
I hadn't considered this in the original PR, but how do we want to deal with this?
On the one hand, all of the different error messages are valid, e.g. you could get two different messages if you're relying on two different unexported types, and maybe you need to fix both. But it's also more error messages, so could be too much information.
Also, I think we didn't port the diagnostic message canonicalization thing we had in Strada, because we got rid of all (most?) sources of differing messages that were morally the same.
There was a problem hiding this comment.
I think for now we should just leave it, and see what happens.
…quiredError call Co-authored-by: gabritto <19519347+gabritto@users.noreply.github.com>
diagnostics_generated.goSymbolTrackerinterface innodebuilder/types.goto addsymbolNameparameter toReportLikelyUnsafeImportRequiredErrorSymbolTrackerImplinchecker/symboltracker.goto pass throughsymbolNameSymbolTrackerImplintransformers/declarations/tracker.goto use new diagnostic when symbolName is availablechecker/nodebuilderimpl.goto passsymbol.Namedirectly (symbol can't be nil here)✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.