Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Import new_library assist should auto-remove unecessary import #55946

Open
FMorschel opened this issue Jun 5, 2024 · 5 comments
Open

Import new_library assist should auto-remove unecessary import #55946

FMorschel opened this issue Jun 5, 2024 · 5 comments
Labels
analyzer-quick-fix area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P3 A lower priority bug or feature request type-enhancement A request for a change that isn't a bug

Comments

@FMorschel
Copy link
Contributor

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
Copy link
Member

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.

@bwilkerson bwilkerson added area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. analyzer-quick-fix labels Jun 6, 2024
@FMorschel
Copy link
Contributor Author

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

@srawlins srawlins added type-enhancement A request for a change that isn't a bug P3 A lower priority bug or feature request labels Jun 7, 2024
@FMorschel
Copy link
Contributor Author

FMorschel commented Oct 31, 2024

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)

  • If the current import already has an alias, add it to the new import and use it.**
  • If the current import has a combinator (hide/show) add it to the new import.

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
Copy link
Member

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
Copy link
Contributor Author

FMorschel commented Nov 5, 2024

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 #55842 and #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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer-quick-fix area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P3 A lower priority bug or feature request type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

3 participants