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.26k stars 1.58k forks source link

Import `new_library` assist should auto-remove unecessary import #55946

Open FMorschel opened 5 months ago

FMorschel commented 5 months ago

When working on Flutter projects, sometimes I extract a Widget to a new file.

While importing the missing dependencies with the assist, sometimes I miss click and import widgets first and then material. Which then warns me about the unnecessary import of widgets.

I believe that the assist should replace the old import if that happens.

bwilkerson commented 5 months ago

While importing the missing dependencies with the assist ...

Are you manually extracting the widget, or is the 'Move to File' refactoring sometimes not adding the imports automatically?

I believe that the assist should replace the old import if that happens.

That's definitely an interesting idea. I suspect the right way to do that would be to look to see whether the import being added exports an existing import.

FMorschel commented 5 months ago

Manually moving. This was before I found out about the new "Move to File" refactor.

FMorschel commented 2 weeks ago

I was thinking a bit more about this request. Some things I was considering:


If there is a need to add a new import which would not be aliased or have a combinator, it might give out some ambiguous_import errors because of it not differentiating or hiding some declarations. So, for this:

If there is only one existing library import that this new library exports (if are adding b which exports a and a is imported twice - different aliases or something - we wouldn't do this)

I'm not sure what else to consider or if you agree with this but this is what I think would make this more consistent.

** One thing that I was worrying about when suggesting moving the alias to the new library would be making the new import have an alias that would not reflect what it is like if we had an import 'dart:math' as math and we then imported a local library that exported it called lib1.dart it might not be as accurate to call it math but I'm unsure what to suggest here.


Or we simply won't affect either an aliased import or one with combinators. That would give the user more to deal with (the same as today, basically).

bwilkerson commented 1 week ago

I'm a bit confused. I think I've lost track of what we're talking about here. Can you provide a small example of what you'd like to see the fix do?

FMorschel commented 1 week ago

The base request is that code like this where Scaffold is not resolved you get an 'Import library' for material. This causes an unnecessary_import trigger because material exports widgets.

import 'package:flutter/widgets.dart';

Widget f() {
  return Scaffold(
    body: Column(),
  );
}

This case is simple and the widgets import should be replaced by material.


My last comment is talking about cases like the following where you have show (or hide) combinator because of some ambiguous import somewhere.

import 'package:flutter/widgets.dart' show Column, Widget;

Widget f() {
  return Scaffold(
    body: Column(),
  );
}

In this case, I believe we should still keep all show (and add the new declaration for Scaffold - as https://github.com/dart-lang/sdk/issues/55842 and https://github.com/dart-lang/sdk/issues/32234 ask) besides just swapping both imports.


My question on the above comment is whether or not we should handle cases like prefixed imports (today we now already have an "Import library with prefix" fix so this could mean the same prefix for both imports and it would trigger unnecessary_import as well - I think, I've not tested but if it doesn't trigger, I think it should) and how should this cases (and mixed) be handled.