Skip to content

fix(58801): "Move to file" on global code unnecessarily imports/exports, generates invalid code - #58811

Merged
Daniel Rosenwasser (DanielRosenwasser) merged 1 commit into
microsoft:mainfrom
a-tarasyuk:fix/58801
Jun 19, 2024
Merged

fix(58801): "Move to file" on global code unnecessarily imports/exports, generates invalid code#58811
Daniel Rosenwasser (DanielRosenwasser) merged 1 commit into
microsoft:mainfrom
a-tarasyuk:fix/58801

Conversation

@a-tarasyuk

@a-tarasyukOleksandr Tarasiuk (a-tarasyuk) commented Jun 9, 2024

Copy link
Copy Markdown
Contributor

Fixes#58801
Fixes#58806

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks right, though it seems like it doesn’t explain the export export export export export export function bug 😄 Was there any clue about why that happened as you were debugging?

@a-tarasyuk

Copy link
Copy Markdown
ContributorAuthor

Andrew Branch (@andrewbranch) The moveToFile refactoring adds the same symbol to each declaration...

targetFileImportsFromOldFile.set(symbol, isValidTypeOnlyUseSite);

for(constdeclofsymbol.declarations){
if(isInImport(decl)){
constprevIsTypeOnly=oldImportsNeededByTargetFile.get(symbol);
oldImportsNeededByTargetFile.set(symbol,[
prevIsTypeOnly===undefined ? isValidTypeOnlyUseSite : prevIsTypeOnly&&isValidTypeOnlyUseSite,
tryCast(decl,(d): d is codefix.ImportOrRequireAliasDeclaration=>isImportSpecifier(d)||isImportClause(d)||isNamespaceImport(d)||isImportEqualsDeclaration(d)||isBindingElement(d)||isVariableDeclaration(d)),
]);
}
elseif(isTopLevelDeclaration(decl)&&sourceFileOfTopLevelDeclaration(decl)===oldFile&&!movedSymbols.has(symbol)){
targetFileImportsFromOldFile.set(symbol,isValidTypeOnlyUseSite);
}
}

@a-tarasyuk

Copy link
Copy Markdown
ContributorAuthor

It looks like these changes fix the crash mentioned in #58806., the moveToNewFile_global3.ts test covers this case.

@andrewbranch

Copy link
Copy Markdown
Member

Good to merge as soon as navya9singh signs off!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think I'm good with this, and I'd like to get it in for 5.5.2.

@DanielRosenwasser

Copy link
Copy Markdown
Member

TypeScript Bot (@typescript-bot) cherry-pick this to release-5.5 and LKG

@typescript-bot

TypeScript Bot (typescript-bot) commented Jun 19, 2024

Copy link
Copy Markdown
Contributor

Starting jobs; this comment will be updated as builds start and complete.

CommandStatusResults
cherry-pick this to release-5.5 and LKG✅ Started✅ Results

@typescript-bot

Copy link
Copy Markdown
Contributor

Hey, Daniel Rosenwasser (@DanielRosenwasser)! I've created #58923 for you.

Daniel Rosenwasser (DanielRosenwasser) pushed a commit that referenced this pull request Jun 19, 2024
…e-5.5 (#58923)
Co-authored-by: Oleksandr T <oleksandr.tarasiuk@outlook.com>
@microsoftMicrosoft (microsoft) locked as resolved and limited conversation to collaborators Oct 16, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

For Uncommitted BugPR for untriaged, rejected, closed or missing bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Assertion violation in "move to new file" "Move to file" on global code unnecessarily imports/exports, generates invalid code

6 participants

@a-tarasyuk@andrewbranch@DanielRosenwasser@typescript-bot@sandersn@navya9singh