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

Code navigation doesn't work correctly for conditional imports #3071

Closed
osaxma opened this issue Jan 15, 2021 · 4 comments
Closed

Code navigation doesn't work correctly for conditional imports #3071

osaxma opened this issue Jan 15, 2021 · 4 comments
Labels
in editor Relates to code editing or language features in lsp/analysis server Something to be fixed in the Dart analysis server is enhancement
Milestone

Comments

@osaxma
Copy link

osaxma commented Jan 15, 2021

Describe the bug

In VS code, conditional imports breaks code navigation (i.e. pressing cmd+click) and linting (i.e., showing error for invalid paths) as shown below:

import 'package:file_picker/file_picker.dart'; // cmd+click ✅  lint ✅  
import 'image_cropper/cropper_stub.dart' // cmd+click ❌  lint ✅  
    if (dart.library.io) 'image_cropper/cropper_mobile.dart' // cmd+click ❌  lint ❌
    if (dart.library.html) 'image_cropper/cropper_web.dart'; // cmd+click ❌  lint ❌

Versions (please complete the following information):

  • VS Code version: 1.52.1
  • Dart extension version: v3.18.1
  • Dart/Flutter SDK version: v3.18.1
@osaxma osaxma added the is bug label Jan 15, 2021
@DanTup DanTup added is enhancement in editor Relates to code editing or language features in lsp/analysis server Something to be fixed in the Dart analysis server and removed is bug labels Jan 18, 2021
@DanTup DanTup added this to the On Deck milestone Jan 18, 2021
@DanTup DanTup modified the milestones: On Deck, v3.24.0 May 24, 2021
@DanTup
Copy link
Member

DanTup commented Jun 8, 2021

@bwilkerson do you know what the expected behaviour is for the missing files here?

import 'invalid_1.dart'
    if (dart.library.io) 'invalid_2.dart'
    if (dart.library.html) 'invalid_3.dart';

The code in LibraryAnalyzer._validateUriBasedDirective looks like it will validate the "selected" one (the one whose condition is matched), although in my testing it seems to always only ever validate the main uri (in a test it seems like the selected URI is always the main one, although I'm not sure if that's just because something isn't set up for the test or that's also what's happening when I try it in a real project).

I suspect it's not difficult to additionally validate each conditional uri, though since the existing code appears to be trying to use the "selected" URI I'm not certain if that's the right route.

@bwilkerson
Copy link

The analyzer's support for conditional imports is not as complete as I'd like it to be. I suspect that the code you pointed to was an earlier attempt to conform to the specification before we realized that it didn't really make sense to do so in the analyzer (because there's no way for users to define the flags that control which URI would be selected).

We can, and should, both validate the conditional URIs and provide navigation from them to the referenced file. I don't think either of those should be tied to the "selected" URI.

@DanTup
Copy link
Member

DanTup commented Jun 8, 2021

Great - I'll take a look at validating each URI then.

For the navigation, it seemed to be working but was returning too many regions which caused VS Code to open a Peek window letting you pick between them (it would include all conditional uris instead of only the one at the requested location). I have a fix for that at https://dart-review.googlesource.com/c/sdk/+/202772/ (@osaxma if that wasn't what you were seeing, let me know and we can debug some more).

@DanTup DanTup changed the title code navigation and linting doesn't work with conditional imports Code navigation doesn't work correctly for conditional imports Jun 21, 2021
@DanTup
Copy link
Member

DanTup commented Jun 21, 2021

I'm splitting this issue because there are two problems and one is fixed, but the other has some complications.

#3422 relates to not showing errors, which I have a fix in progress for but there are some details to work out.

This issue is now just for navigation, which is fixed by dart-lang/sdk@eb28368 (if my reproduction of the issue is the same - which is that multiple ranges are returned and VS Code shows them all in the peek window, rather than just jumping to the correct one). The fix will be included in an upcoming (non-hotfix) SDK release.

@DanTup DanTup closed this as completed Jun 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in editor Relates to code editing or language features in lsp/analysis server Something to be fixed in the Dart analysis server is enhancement
Projects
None yet
Development

No branches or pull requests

3 participants