organizeImports makes no changes if there are parse errors in the sourceFile

[iisaduan]

When called from our services, organizeImports returns no changes if there are parse errors in the sourceFile.

fixes #56252 / #56252 (comment)

The original crash happened because the parser was unable to parse a .js file, and as a result, the failed parse generated two duplicate/overlapping modules. Organize imports then tried to remove the same imports twice, leading to a change tracker crash.

[andrewbranch]

This seems very reasonable to me, given commands like Go To Definition do nothing when there are parse errors. However, I will note that there were existing tests that showed the feature working when a parse error occurred somewhere else in the file. I think the crash this fixes happened when there was a parse error on a node that organize imports was actually going to operate on / analyze, so it might be possible to make a narrower fix and ignore nodes with parse errors deeper in the organize imports logic. I’m skeptical that’s worth it, but might look for input from other reviewers.

[jakebailey]

I feel like this is fine; it's not like eslint or dprint or prettier will operate on malformed files to organize imports either.

[DanielRosenwasser]

This seems very reasonable to me, given commands like Go To Definition do nothing when there are parse errors.

Is that true? Do we have an example?

[DanielRosenwasser]

I think this makes sense - though if I had to make a suggestion, it would be that when a parse error is present in a file:

• Organize Imports will never remove unused imports (because the file might be borked and semantics won't work).
• Organize Imports only sorts imports when no parse error occurs by any import/export statement.
• Similarly, Sort Imports allows you to sort imports when no parse error occurs.

[iisaduan]

This seems very reasonable to me, given commands like Go To Definition do nothing when there are parse errors.

Is that true? Do we have an example?

Quick example from the test case that I had to change (it had a parse error since case is a keyword)

import { case/*1*/, Sensitive/*2*/ } from './a'

//a.ts
export const case = 1;
export const Sensitive = 2;

GoToDefinition works fine at 2, but does nothing at 1, so it has more specific behavior than just "no definitions if there's any parse issues anywhere in the file"

[andrewbranch]

commands like Go To Definition do nothing when there are parse errors

GoToDefinition works fine at 2

I should have said Go To Definition does nothing when there are parse errors in the file where it’s triggered

[DanielRosenwasser]

@andrewbranch it still works in part, it is just isn't guaranteed to give you correct results. The language service is generally pretty resilient within a file even in the presence of a parse error. Maybe we're talking about the same thing/actually in agreement?

[andrewbranch]

You’re right—I based that assertion off a random single experiment, but putting another one together, I see it’s not disabled.