Skip to content

Fix debug failure from fixUnusedIdentifier#23973

Closed
ghost wants to merge 1 commit intomasterfrom
formatterDebugFailure
Closed

Fix debug failure from fixUnusedIdentifier#23973
ghost wants to merge 1 commit intomasterfrom
formatterDebugFailure

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented May 8, 2018

Fixes #23942

Instead of replacing the entire arrow function node, just replace the parameter with the string "()". This avoids reformatting the body, which can contain invalid emitted nodes like " (with no closing quote) that trigger formatter assertions. (When we emit it we also emit the closing quote, but we're still using the original positions which end after the opening quote.)

@ghost ghost requested a review from amcasey May 8, 2018 20:46
@ghost ghost force-pushed the formatterDebugFailure branch from c862232 to fc2424f Compare May 8, 2018 21:20
@mhegazy
Copy link
Copy Markdown
Contributor

mhegazy commented May 8, 2018

This avoids reformatting the body, which can contain invalid emitted nodes like " (with no closing quote) that trigger formatter assertions.

I think we need a more general solution here. I am fine with taking this change, but we should not keep going down this wake-a-mole path, we either say we run the formatter as a best-effort attempt, or we will fix the formatting scanner so that it does not assert on invalid nodes.

@ghost
Copy link
Copy Markdown
Author

ghost commented May 8, 2018

I think the problem in this case is that we create a new node with old nodes as children. When we emit the new nodes we create new emitted text which emits the string literal correctly, but the formatter looks at the old source file's text which only has the opening quote.

@mhegazy
Copy link
Copy Markdown
Contributor

mhegazy commented May 8, 2018

Would cloning the nodes solve the issue?

@ghost
Copy link
Copy Markdown
Author

ghost commented May 8, 2018

getSynthesizedDeepClone doesn't change positions of nodes: eb4f067

If it did we wouldn't be able to get the comments from the old nodes... but if we do keep the old positions the formatter can run into problems.
We are good when inserting new nodes and deleting old nodes, but replacing an old node with a partly-old-partly-new node is iffy.

@mhegazy
Copy link
Copy Markdown
Contributor

mhegazy commented May 8, 2018

Well, then we should always be cloning them and their comments. I think that is a simpler invariant to assert, do not repurpose nodes from the existing tree, you can only add new nodes, and that is recursive..

@typescript-bot
Copy link
Copy Markdown
Collaborator

Thanks for your contribution. This PR has not been updated in a while and cannot be automatically merged at the time being. For housekeeping purposes we are closing stale PRs. If you'd still like to continue working on this PR, please leave a message and one of the maintainers can reopen it.

@ghost ghost deleted the formatterDebugFailure branch September 17, 2018 23:47
@microsoft microsoft locked as resolved and limited conversation to collaborators Oct 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants