dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.28k stars 1.58k forks source link

Remove unused imports is not "across files" #57142

Closed FMorschel closed 3 days ago

FMorschel commented 3 days ago

I see that RemoveUnusedImport from analysis_server\lib\src\services\correction\dart\remove_unused_import.dart has the CorrectionApplicability.acrossSingleFile and this is the comment:

Bulk application is supported by a distinct import cleanup fix phase.

I had a project with multiple unused imports because I added an export. And now multiple files had this warning - a single unused import each.

VS Code problems tab had no option to fix them all at once so I ran the CLI dart fix and almost everything got fixed but my tests (I'm unsure why I could not fix anything there even though my analysis_option says nothing about ignoring them (working on a package).

What can we do about this? The above comment tells me nothing about what other phase is fixing that as well.

dart-github-bot commented 3 days ago

Summary: The RemoveUnusedImport analyzer only handles unused imports within a single file. A user requests a solution to address unused imports across multiple files simultaneously.

bwilkerson commented 3 days ago

... I ran the CLI dart fix and almost everything got fixed but my tests ...

Which directory were you in when you ran dart fix. By default it will apply fixes in the current working directory.

If that wasn't the problem, then I don't know what the issue is, but it's a bug for it to have not fixed code in the test directory.

The above comment tells me nothing about what other phase is fixing that as well.

I'm not sure what you mean by "fixing that as well." If you mean imports in test files, then the answer is that they should be fixed in the same phase as imports in any other file. Otherwise, please clarify.

FMorschel commented 3 days ago

Which directory were you in when you ran dart fix. By default it will apply fixes in the current working directory.

Nice question. I simulated it and it was fixed on the tests.

I may have changed the directory to lib or something and forgot about it. I'll keep that in mind next time. Maybe I was under the impression it would look at the full project because it needs to look at analysis_options.yaml to define what should it test for with the analysis server.


I'm not sure what you mean by "fixing that as well." If you mean imports in test files, then the answer is that they should be fixed in the same phase as imports in any other file.

Yes, thanks. But I guess my real question here is:

Bulk application is supported by a distinct import cleanup fix phase.

What is this "distinct import cleanup fix phase"?

I don't think it was clear enough for me to understand.

bwilkerson commented 3 days ago

In case it's useful, you can pass the target directory on the command-line. That would allow you to write a script to run dart fix on your project's root directory that would work no matter which directory you happen to be in at the moment.

What is this "distinct import cleanup fix phase"?

The dart fix tool works by analyzing the files in a directory, looking for fixes that have a CorrectionApplicability that allows them to be bulk-applied, and applying those fixes. These steps are repeated until either there are no remaining diagnostics with fixes or until the maximum number of iterations have been performed (to prevent infinite loops). That's the first phase.

After this it performs one more analysis looking for unused imports and applies those fixes all in the final phase. The reason it's a separate phase is because a fix in one iteration can make an import be unused while a fix in a subsequent phase can cause it to be used again. To reduce the total amount of work being done and the chance of conflicting edits from one iteration to the next, we moved that work to happen once at the end.

FMorschel commented 3 days ago

I see. Thanks for answering everything!