fixUnusedIdentifier: Don't delete node whose ancestor was already deleted #24207
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
requested review from
mhegazy
reviewed
May 17, 2018
| tryDeleteDeclaration(changes, sourceFile, token); | ||
| getAllCodeActions: context => { | ||
| // Track a set of deleted nodes that may be ancestors of other marked for deletion -- only delete the ancestors. | ||
| const deleted = new NodeSet(); |
There was a problem hiding this comment.
should we do this in the chanetracker instead, and have deleteRange filter out deletions if they are already subsumed with other ones?
There was a problem hiding this comment.
I think that indicates a genuine error though, that we might want to know about. There's also one case where we replaceNode instead that would presumably still be seen as an error by ChangeTracker.
There was a problem hiding this comment.
so every quick fix/refactor that needs to remove things will need to do this book keeping?
There was a problem hiding this comment.
As far as I can tell, this is the only code fix that should potentially delete a node and one of its children. In other cases that would be a bug that we would want to catch..
mhegazy
approved these changes
May 17, 2018
weswigham
added a commit
that referenced
this pull request
May 17, 2018
* Optimize intersections of unions of unit types * Add regression test * Accept new baselines * LEGO: check in for master to temporary branch. * Add Unicode ThirdPartyNotice * Properly handle edge cases * Sort the whole diagnostic, plus giving up on isolating tests (#24186) * Sort the whole diagnostic * Also strip references to our repos node_modules, since removing it is hard * LEGO: check in for master to temporary branch. * add quick fix for import type missing typeof * Add callback tag, with type parameters (#23947) * Add initial tests * Add types * Half of parsing (builds but does not pass tests) * Parsing done; types are uglier; doesn't crash but doesn't pass * Bind callback tag Builds but tests still don't pass * Only bind param tags inside callback tags * Fix binding switch to only handle param tags once * Checking is 1/3 done or so. Now I'm going to go rename some members to be more uniform. I hate unnnecessary conditionals. * Rename typeExpression to type (for some jsdoc) (maybe I'll rename more later) * Rename the rest of typeExpressions Turns out there is a constraint in services such that they all need to be named the same. * Few more checker changes * Revert "Rename the rest of typeExpressions" This reverts commit f41a96b. * Revert "Rename typeExpression to type (for some jsdoc)" This reverts commit 7d2233a. * Finish undoing typeExpression rename * Rename and improve getTypeParametersForAliasSymbol Plus some other small fixes * Core checking works, but is flabbergastingly messy I'm serious. * Callback return types work now * Fix crash in services * Make github diff smaller * Try to make github diff even smaller * Fix rename for callback tag * Fix nav bar for callback tag Also clean up some now-redundant code there to find the name of typedefs. * Handle ooorder callback tags Also get rid of redundant typedef name code *in the binder*. It's everywhere! * Add ooorder callback tag test * Parse comments for typedef/callback+display param comments * Always export callbacks This requires almost no new code since it is basically the same as typedefs * Update baselines * Fix support for nested namespaced callbacks And add test * Callbacks support type parameters 1. Haven't run it with all tests 2. Haven't tested typedef tags yet 3. Still allows shared symbols when on function or class declarations. * Template tags are now bound correctly * Test oorder template tags It works. * Parser cleanup * Cleanup types and utilities As much as possible, and not as much as I would like. * Handle callback more often in services * Cleanup of binder and checker * More checker cleanup * Remove TODOs and one more cleanup * Support parameter-less callback tags * Remove extra bind call on template type parameters * Bind template tag containers Doesn't quite work with typedefs, but that's because it's now stricter, without the typedef fixes. I'm going to merge with jsdoc/callback and see how it goes. * Fix fourslash failures * Stop pre-binding js type aliases Next up, stop pre-binding js type parameters * Further cleanup of delayed js type alias binding * Stop prebinding template tags too This gets rid of prebinding entirely * Remove TODO * Fix lint * Finish merge with use-jsdoc-aliases * Update callback tag baselines * Rename getTypeParametersForAliasSymbol The real fix is *probably* to rename Type.aliasTypeArguments to aliasTypeParameters, but I want to make sure and then put it in a separate PR. * moveToNewFile: Don't move imports (#24177) * Reduce map lookups (#24203) * Fix jsdoc type resolution [merge to master] (#24204) * Fix JSDoc type resolution Breaks type parameter resolution that is looked up through prototype methods, though. I need to fix that still. * Check for prototype method assignments first * Undo dedupe changes to getJSDocTags * JS Type aliases can't refer to host type params Previously, js type aliases (@typedef and @callback) could refer to type paremeters defined in @template tags in a *different* jsdoc tag, as long as both tags were hosted on the same signature. * Reduce dedupe changes+update baseline The only reason I had undone them was to merge successfully with an older state of master. * Add undefined guard * moveToNewFile: Fix bug for VariableDeclaration missing initializer (#24214) * Use import types to refer to declarations in declaration emit (#24071) * Stand up a simple implementation using import types for exports of modules which are otherwise inaccessible * Ensure references exist to link to modules containing utilized ambient modules * Accept baselines with new import type usage * Fix lint * Accept changed baseline (#24222) * fixUnusedIdentifier: Don't delete node whose ancestor was already deleted (#24207) * More robust circularity detection in node builder (#24225) * Improve ChangeTracker#deleteNodeInList (#24221) * moveToNewFile: Fix bug for missing importClause (#24224) * Set startPos at EOF in jsdoc token scanner so node end positions for nodes terminated at EoF are right (#24184) * Set startPos at EOF in jsdoc token scanner to node end positions for nodes terminated at EoF are right * More complete nonwhitespace token check, fix syntactica jsdoc classifier * Use loop and no nested lookahead * Do thigns unrelated to the bug in the test * Fix typo move return * Patch up typedef end pos * Fix indentation, make end pos target more obvious
ghost
mentioned this pull request
This pull request was closed.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #24202