New error message for depend_on_referenced_packages quick-fix

[FMorschel]

I have a use case for a change in depend_on_referenced_packages quick-fix error message.

In my company we create some local packages. They sometimes depend on each other.

Just now I was editing a package, let's call it a, that has a dependency in a second package b which ultimately depends on c.

When I wrote down some code that needed package c and then added the import line, I got a this lint:
The imported package 'c' isn't a dependency of the importing package.

But when trying to use the assist I got an error.

[a] flutter pub add c
Because a depends on c any which doesn't exist (could not find package c at https://pub.dev), version solving failed.
exit code 65

But (I believe) the analysis server knows that my b package depends on a c package from path.

I dont't think the analyzer should be able to figure out the path from both packages to solve where c is at in my machine, but shouldn't it at least show me that the dependency is not from pub.dev?

I have not tested but it got me guessing about dependencies that rely on GitHub repository links or similar. What then? If I need something that is not in release yet I'll need to manually change there anyway so the quick-fix would not be actually helping but IMO it would be generating doubts.

[bwilkerson]

Those are good point. It does seem like server ought to minimally be able to tell whether the package_config.json file maps the package name to a directory inside the .pub-cache directory, and if not it shouldn't offer the fix.

Whether it's worth looking in the pubspec.yaml files to figure out the right way to depend on non-pub-provided packages is another question, but it minimally shouldn't be creating new issues that might be even more confusing for users.

[DanTup]

It's possible this fix is coming from Dart-Code and not the server. Although we do call pub for a few things in server now, at one time it seemed like an odd thing to do and Dart-Code was already doing it (and has an "Add Dependency" command), so we include a code action to trigger the command for some kinds of diagnostics:

https://github.com/Dart-Code/Dart-Code/blob/master/src/extension/providers/add_dependency_code_action_provider.ts

The implementation is pretty basic so it's no surprise if there are edge cases. Perhaps moving it to server and fixing this issue together would make sense (because server has much better understanding of the projects than Dart-Code does).

[bwilkerson]

Yes, in general we should have as much support as possible in the server.

[FMorschel]

Perhaps moving it to server [...]

Yes, in general we should have as much support as possible in the server.

Should I open a new issue for us to reference this work? I was thinking of taking a look at these but with this on the way I figured it should be done first.

• New quick-fix to fix invalid_dependency in pubspec.yaml #56647
• Add a fix for the sort_pub_dependencies lint #47958

Also, to note. I've found in pkg\analysis_server\lib\src\services\correction\fix\pubspec\fix_kind.dart that we have pubspec.fix.add.name but I couldn't find anything to trigger it. So possibly taking a look at it too.

[DanTup]

The current fix provided by Dart-Code doesn't actually modify the pubspec, it runs pub add which will both update the pubspec and fetch the packages.

So to provide the same functionality in the server, we'd need a fix/assist that triggers a command (I'm not sure if we currently support commands except for on refactors, which go through a generic command.. this would need to be a fix that can provide its own command), and then a custom command that can invoke pub add (we have a PubCommand class used for some other things like pub outdated).

If we implement this here, we could update Dart-Code to only add its own actions if the new command is not registered by the server (so we don't show duplicates).

The legacy protocol doesn't support fixes/assists that call back to the server, so I think there are two options:

• implement this as a Refactor (which already supports calling commands), which could be used over the legacy protocol if it gets support for the new refactors (natively, or via LSP-over-Legacy)
• implement it in the LSP CodeActions handler, similar to how we insert the old refactorings as extra commands

Alternatively, we could add support to fixes/assists for commands, but then we'd need to ensure they didn't show up in the list for the legacy protocol because they wouldn't contain any source changes and instead have a command that can't be represented.

[DanTup]

Should I open a new issue for us to reference this work?

I think we could probably use this one to cover fixing this message by moving it to the server and updating it there?

[bwilkerson]

@DanTup already suggested this on Discord, but as I was about to say the same thing I'll add it here for the record.

It isn't clear to me that we want to execute pub as part of a quick fix. What if the user has just pasted some code and there's more than one package that needs to be added. If they add the dependencies one at a time (for example, to have better visibility of what dependencies they're adding), do they really want to run pub after each addition? I think it might be better to decouple these operations (adding dependencies and running pub).

Do we have other features (in either the server or in the plugin) that automatically run pub (other than to get information for code completion)? I don't think so, but I might be forgetting something.

As for the technical aspects ...

 So to provide the same functionality in the server, we'd need a fix/assist that triggers a command ...

We currently can't trigger commands in IntelliJ (and maybe some LSP clients), so we need to decide whether we're ok having different behaviors on different clients (assuming we want to pursue executing a command).

implement this as a Refactor ...
implement it in the LSP CodeActions handler ...

Both of these have the disadvantage that they'd only make the functionality available in a subset of clients.

I don't want to support functionality on a single client if we can find a way to make it available more generally, but the first of these options is better than the second because there's at least a path forward for other clients, even if it's non-trivial.

Alternatively, we could add support to fixes/assists for commands, but then we'd need to ensure they didn't show up in the list for the legacy protocol because they wouldn't contain any source changes and instead have a command that can't be represented.

We currently send a command (as well as an edit) with every quick fix whose only purpose is to allow us to gather analytics data (when we're authorized to do so). We could presumably add additional behavior for some fixes if we want to, but I don't think we can guarantee that the pubspec file has been saved before executing pub, so even with the ability to execute a command I don't think we can guarantee that we won't corrupt the file.

[DanTup]

Do we have other features (in either the server or in the plugin) that automatically run pub (other than to get information for code completion)? I don't think so, but I might be forgetting something.

In Dart-Code, we run pub get when you save pubspec.yaml, and we also inject this quick-fix for the diagnostic above which triggers pub add. The first one I think we should keep (and because it runs on-save, it's very likely things are consistent), and the second is ofc the one we're discussing here (and maybe the current behaviour is questionable).

but I don't think we can guarantee that the pubspec file has been saved before executing pub, so even with the ability to execute a command I don't think we can guarantee that we won't corrupt the file.

Correct (or at least, not with custom code in the client that could do this).

Another possibility (though I haven't completely thought it through) is we just make a normal fix that edits the pubspec, but in Dart-Code we detect when this is executed, and if the pubspec was not dirty before, then we auto-save it after the fixes edits are applied. That would retain the current behaviour in Dart-Code (and be possible to implement in other clients with a little code), but gives us the safety of not modifying files on disk that the user has open changes for.
(I'm not certain how simple/clean this would be, but if it seems like a good idea I can figure that out).

[FMorschel]

Here is a new issue that came up regarding this being currently handled by Dart-Code Dart-Code/Dart-Code#5378. The current method will stop working (for directories inside package) and it'll need to be handled in the future.

$ flutter pub add highlight/languages

Using a naked argument for directory is deprecated and will stop working in a future Flutter release.

Use --directory instead.
Expected to find project root in highlight/languages.
exit code 1

[DanTup]

@FMorschel thanks - that issue was a bad regex that extracted the package name from the import string. I've pushed a fix for that (although based on the above, if/when this moves to server, we will probably stop calling pub add for this fix).