Widget snippets generate unused import

[navaronbracke]

The stateless/stateful/animation controller widget snippets generate an unused import in Flutter 3.24, so I think that the snippets should be updated. (might need to determine when the foundation import was made obsolete for the snippet)

For example, given an empty Dart file, selecting the stateless snippet generates

import 'package:flutter/foundation.dart';
import 'package:flutter/widgets.dart';

class MyWidget extends StatelessWidget {
  const MyWidget({super.key});

  @override
  Widget build(BuildContext context) {
    return const Placeholder();
  }
}

If you then run dart analyze on the snippet, you get

Unused import: 'package:flutter/foundation.dart'. Try removing the import directive. • unused_import

Related issue: #49081

Flutter Doctor

[✓] Flutter (Channel stable, 3.24.1, on macOS 14.6.1 23G93 darwin-x64, locale
    en-BE)
    • Flutter version 3.24.1 on channel stable at
      /Users/navaronbracke/Documents/flutter
    • Upstream repository git@github.com:navaronbracke/flutter.git
    • FLUTTER_GIT_URL = git@github.com:navaronbracke/flutter.git
    • Framework revision 5874a72aa4 (12 days ago), 2024-08-20 16:46:00 -0500
    • Engine revision c9b9d5780d
    • Dart version 3.5.1
    • DevTools version 2.37.2

[✓] Android toolchain - develop for Android devices (Android SDK version 34.0.0)
    • Android SDK at /Users/navaronbracke/Library/Android/sdk
    • Platform android-34, build-tools 34.0.0
    • ANDROID_HOME = /Users/navaronbracke/Library/Android/sdk
    • Java binary at: /Applications/Android
      Studio.app/Contents/jbr/Contents/Home/bin/java
    • Java version OpenJDK Runtime Environment (build
      17.0.10+0-17.0.10b1087.21-11572160)
    • All Android licenses accepted.

[✓] Xcode - develop for iOS and macOS (Xcode 15.4)
    • Xcode at /Applications/Xcode.app/Contents/Developer
    • Build 15F31d
    • CocoaPods version 1.15.2

[✓] Chrome - develop for the web
    • Chrome at /Applications/Google Chrome.app/Contents/MacOS/Google Chrome

[✓] Android Studio (version 2023.3)
    • Android Studio at /Applications/Android Studio.app/Contents
    • Flutter plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/9212-flutter
    • Dart plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/6351-dart
    • Java version OpenJDK Runtime Environment (build
      17.0.10+0-17.0.10b1087.21-11572160)

[✓] VS Code (version 1.92.2)
    • VS Code at /Applications/Visual Studio Code.app/Contents
    • Flutter extension version 3.94.0

[✓] Connected device (3 available)
    • iPhone Xs (mobile) • A1C65549-1101-4825-B9ED-41E7C098ACD5 • ios
      • com.apple.CoreSimulator.SimRuntime.iOS-17-5 (simulator)
    • macOS (desktop)    • macos                                • darwin-x64
      • macOS 14.6.1 23G93 darwin-x64
    • Chrome (web)       • chrome                               • web-javascript
      • Google Chrome 128.0.6613.113

[✓] Network resources
    • All expected network resources are available.

• No issues found!

[dart-github-bot]

Summary: Flutter widget snippets in version 3.24 generate an unused import of package:flutter/foundation.dart. This import is unnecessary and should be removed from the generated code.

[navaronbracke]

Since we had issues like this in the past, perhaps we should have the Flutter team communicate if a change could "break" a snippet? Having the snippets need to pass static analysis might also catch this.

[lrhn]

Should the Flutter team be the ones adding the Flutter snippets to the Flutter SDK version of the analyzer, instead of the analyzer in the Dart SDK containing (generators for) code that can't be tested with just the Dart SDK?
(Assuming these snippets are in the SDK analyzer code, otherwise the issue isn't even in the correct repo.)

[bwilkerson]

@DanTup

[DanTup]

I don't seem to be able to reproduce this in 3.24.1 in an empty file:

snippet.mp4

The imports aren't hard-coded, but are computed by the server from the types that are referenced in the snippets (there are many places where we might write a reference to a type and need to import it, so there is code to determine the "best" import to add - eg. we don't want to import the real src/ file that contains the types used in the snippet). My feeling is that something is wrong wrong here (or we're first importing a library for one type, then later import a library that also includes it along with other referenced types).

@navaronbracke is it possible you could capture a log while reproducing this? Ideally in a new project (to minimize differences between us):

• close the empty file
• run the Dart: Capture Analysis Server Logs command from the palette (F1)
• open the empty file
• use the stless snippet
• click cancel on the logging notification to stop logging and open the log file
• attach the log here

Thanks!

@lrhn

Should the Flutter team be the ones adding the Flutter snippets to the Flutter SDK version of the analyzer, instead of the analyzer in the Dart SDK containing

Currently there's no mechanism for snippets that aren't built into the server (written in Dart). Probably the ideal solution is that any package can provide snippets - there's an issue for that at #32103, although it only has three 👍 's so far so I don't know how much demand there is from 3p packages.

[DanTup]

Looking back at my video, cupertino also seems an odd (though valid) choice here. I wonder if snippets should have some preferred imports that are added first (or whether there's some way a package could express some preference for which libraries to prefer).

[navaronbracke]

@DanTup I also cannot reproduce it on a new project. Strangely enough the project that I reproduced it with was upgraded to Flutter 3.24.0 (I can still repro it there, though)

Edit: I just now updated that project's pubspec from Flutter 3.24.0 / Dart 3.5.0 to use Flutter 3.24.1 / Dart 3.5.1 and now I also cannot reproduce it there. Maybe something got fixed in the hotfix release?

(My local Flutter checkout was up to date for 3.24.1 though)

[DanTup]

If you mean you updated the environment in the pubspec to require Dart 3.5.1, I wouldn't expect that to change anything here (you'd have still been using the same compiled analysis server that shipped with 3.5.1 even if the SDK constraint was lower).

I wonder if some server state caused us to enumerate the libraries in a different order and that resulted in us adding different imports. I can't see any obvious reason for this, though I do see that we discover files in the server using Folder.getChildren() and the docs for that say "in no particular order". I don't know what would influence that order though (or how stable it would be across IDE sessions etc.).

Out of interest, when you try now - does it select the same cupertino.dart library that my video showed or something else?

[navaronbracke]

Out of interest, when you try now - does it select the same cupertino.dart library that my video showed or something else?

I never had any snippets provide cupertino imports.

That said, I did an SDK upgrade to Flutter 3.24.2 / Dart 3.5.2 and the bug reappeared in that old project.

I did capture the log this time:
Dart-Code-Log-2024-08-04 19-33-28.txt

I can't really tell why an existing project would be affected, but a new one isn't. (although the existing project was originally created a while back and had a few SDK upgrades since, so it's not a "fresh" workspace)

[DanTup]

Thanks, the logs show a file being opened (lib/test.dart) with the content "\n", typing st and then the completion response containing this snippet:

{
    "additionalTextEdits": [
        {
            "insertTextFormat": 2,
            "newText": "import 'package:flutter/foundation.dart';\nimport 'package:flutter/widgets.dart';\n\n",
            "range": {
                "end": {
                    "character": 0,
                    "line": 1
                },
                "start": {
                    "character": 0,
                    "line": 1
                }
            }
        }
    ],
    "documentation": {
        "kind": "markdown",
        "value": "Insert a Flutter StatefulWidget with an AnimationController."
    },
    "filterText": "stanim",
    "insertTextFormat": 2,
    "insertTextMode": 1,
    "kind": 15,
    "label": "Flutter Widget with AnimationController",
    "sortText": "zzzstanim",
    "textEditText": "class ${1:MyWidget} extends StatefulWidget {\n  const ${1:MyWidget}({super.key});\n\n  @override\n  State<${1:MyWidget}> createState() => _${1:MyWidget}State();\n}\n\nclass _${1:MyWidget}State extends State<${1:MyWidget}>\n    with SingleTickerProviderStateMixin {\n  late AnimationController _controller;\n\n  @override\n  void initState() {\n    super.initState();\n    _controller = AnimationController(vsync: this);\n  }\n\n  @override\n  void dispose() {\n    _controller.dispose();\n    super.dispose();\n  }\n\n  @override\n  Widget build(BuildContext context) {\n    return ${0:const Placeholder()};\n  }\n}"
}

When I try to reproduce on 3.42.2 I get different imports, but ones that produce the same problem:

import 'package:flutter/animation.dart';
import 'package:flutter/cupertino.dart';

The first import is added as we try to resolve AnimationController but then for other symbols we pick cupertino which also includes AnimationController, making it redundant (although interestingly I see UNNECESSARY_IMPORT where you see to have unused_import?).

Some ideas:

1. When we add a new import for an Element in DartFileEditBuilder, we check whether that import includes all Elements tied to another pending import, and if so replace that pending import with this one.
2. During DartFileEditBuilder.finalize(), loop over the pending imports and see if any of them are to import only elements available in other pending imports (in which case, remove it).
3. Run some code in a command on the snippet afterwards, that "cleans up" after applying the original edits (though we'd have to be careful not to remove anything we didn't just add)
4. Have snippets "preferred" import that they will always select first if a symbol is not already imported (for example using widgets here).. this is probably simplest, but it doesn't avoid the same issue cropping up in different places if more code starts to import elements this way

One slight concern I have is that these edits are all built in-line when computing code completions (these edits are not delayed until resolve), so any additional work here is going into the completion time. I wonder if we should change snippets so their edits (or at least imports) are computed lazily?

@bwilkerson any thoughts about this?

[bwilkerson]

I don't have a definitive answer, but yes, I have thoughts. :-)

1. Assuming that we're already not adding an import if there's already a pending import that would already make the element available in scope, then it seems like an oversight that we're not already doing this.

2. I'm a bit concerned about the cost of that approach, especially if we're already partially optimizing the list as we're building it. I suspect that the first item will be more performant just by virtue of keeping the list minimal at all times.

3. It sounds to me like this would be difficult to get right, largely because of the need to not remove imports that weren't added.

4. I generally prefer general solutions like (1) over more targeted solutions like (4) just because they tend to minimize future problems. Sometimes the complexity of the general solution make it prohibitive, but it doesn't sound like that's the case here.

I wonder if we should change snippets so their edits (or at least imports) are computed lazily?

That's an interesting optimization. I don't know how often snippets are causing performance problems, though, so I don't know whether it's a problem worth solving.

@keertip Something to keep in mind as we start to look at the performance of completion post-refactoring.

[DanTup]

1. Assuming that we're already not adding an import if there's already a pending import that would already make the element available in scope, then it seems like an oversight that we're not already doing this.

The first part is correct, we do check existing pending imports to see if they cover the new element, but we don't check if any existing imports may now be redundant. We'd need to store some additional info against pending imports for this (because we can only remove a pending import if it's only being used to provide those elements, and the caller hasn't explicitly called importLibrary for it, and then it was just reused by an Element import).

I think doing this might be the nicest solution though - it does mean a bit more looping as we're adding imports, but given the number of imports being added is likely very small, it's probably significantly less than the existing work to locate a non-src import, for example. I'll try doing this.

That's an interesting optimization. I don't know how often snippets are causing performance problems, though, so I don't know whether it's a problem worth solving.

We did have some reports of perf issues here in the past (because each snippet would perform some of the same work) which we solved in 092bdb1. I don't know of any complaints since then so perhaps it's not worth changing now, but it's something to keep in mind (we don't build the edits up-front for other kinds of completions as they're handled via resolve instead).