Add a lint for missing files in conditional imports #3080

DanTup · 2021-12-02T15:32:54Z

An issue came up in Dart-Code abut conditional imports not being validated:

import 'image_cropper/cropper_stub.dart'
    if (dart.library.io) 'image_cropper/cropper_mobile.dart' // no error/warning if file doesn't exist
    if (dart.library.html) 'image_cropper/cropper_web.dart';

Originally this was added to the analysis server (https://dart-review.googlesource.com/c/sdk/+/202924) but it triggered a lot of warnings internally for files that haven't been generated yet and it was agreed it would be better as a lint.

This is an attempt to move convert it to a lint, although I have a few outstanding questions:

what should it be called? I've used missing_conditional_import as a placeholder, but I suspect there are some conventions it should follow
is this tested correctly? I noticed a lot of lints have source in test_data but from reading ☂ Improve linter test reliability and maintainability sdk#58520 I got the impression that might be the old way, and this may be the new way
in the original analysis server version, the source was checked for existence differently (below), but as far as I can see I can't do that here, however calling configuration.uriSource.exists() seems to do what I want (the comment notes it won't include overlays, but I think that's probably fine - running the script through the VM also wouldn't find the overlay):
```
bool _isExistingSource(Source source) {
  if (source is InSummarySource) {
  	return true;
  }
  return _library.isExistingFilePath(source.fullName);
}
```

@pq @bwilkerson

coveralls · 2021-12-02T15:40:24Z

Coverage increased (+0.2%) to 94.31% when pulling aabc623 on DanTup:validation-conditional-import-uris into cc14c30 on dart-lang:master.

bwilkerson · 2021-12-02T16:31:30Z

lib/src/rules/missing_conditional_import.dart

+class MissingConditionalImport extends LintRule {
+  MissingConditionalImport()
+      : super(
+            name: 'missing_conditional_import',


As for naming, I might suggest that we use a name similar to the error code (URI_DOES_NOT_EXIST) such as conditional_uri_does_not_exist. There's a companion error code in the analyzer (URI_HAS_NOT_BEEN_GENERATED) and we might want to have two separate messages for this lint to distinguish between those cases, but that's less clear to me.

(The only convention that I'm aware of for naming lints is that we're now actively discouraging using 'avoid', 'prefer' and other such designations from the style guide because those tend to change over time leaving us with misleading lint names. Other than that, lints should be named to either express the condition they're trying to ensure or the condition they're trying to prevent.)

Thanks! I've changed it to conditional_uri_does_not_exist.

There's a companion error code in the analyzer (URI_HAS_NOT_BEEN_GENERATED) and we might want to have two separate messages for this lint to distinguish between those cases, but that's less clear to me.

I don't know much about that so have no opinion, but if you think it would also be useful here I'm happy to add it - it doesn't seem like a complicated change (just changing the code/error depending on whether the uri ends with these suffixes?).

Thinking about it a bit more, I'd propose that we leave it as is. If we get feedback from users that a different message would be helpful we can always update it later.

bwilkerson · 2021-12-02T16:32:35Z

lib/src/rules/missing_conditional_import.dart

+  static const LintCode code = LintCode('missing_conditional_import',
+      "The target of the conditional URI '{0}' doesn't exist.",
+      correctionMessage: 'Try creating the file referenced by the URI, or '
+          'Try using a URI for a file that does exist.');


nit: "Try" --> "try"

Done, thanks!

bwilkerson · 2021-12-02T16:41:56Z

lib/src/rules/missing_conditional_import.dart

+    String? uriContent = configuration.uri.stringValue;
+    if (uriContent != null) {
+      Source? source = configuration.uriSource;
+      if (!(source?.exists() ?? false)) {


As you noted, this will probably work most of the time, but it would be better if we could ask the resource provider. Unfortunately there doesn't appear to be any way to access it currently.

My personal opinion is that we need to update LinterContext to provide access to the resource provider and then pass that in when constructing the visitor. Unfortunately, that's a multi-step process, and I'm not sure we want to delay merging this rule while that happens.

I'm happy to go with your decision here. If you think it's worth doing (and I know enough to do it), I'm happy to. Although I'm not sure if having to support package: URIs complicates using the resource provider. This exists() call seemed to handle that (and dart: URIs) for me which I thought might have complicated things :-)

I'm fine with leaving it as-is. You might consider adding a comment for future reference, because the fact that it isn't checking for overlays might not occur to a future reader.

Done!

Would this lint be invalidated/updated correctly if the user saved a new file with the name that was used here? Would the server know if a file has import 'file_a.dart' but that file does not exist, that creating a file at that location may require this file to be re-analyzed/linted?

I don't know whether it is the case, but it ought to be re-analyzed when a new file is added, and we run lints as part of analyzing a file.

bwilkerson · 2021-12-02T16:45:52Z

test/rules/missing_conditional_import.dart

+}
+
+@reflectiveTest
+class MissingConditionalImportTest extends LintRuleTest {


Yes, this is the preferred style for tests, thanks!

bwilkerson · 2021-12-02T16:50:10Z

lib/src/rules/missing_conditional_import.dart

+  }
+}
+
+class _Visitor extends RecursiveAstVisitor<void> {


For performance reasons, the linter uses visitors as a dispatch mechanism rather than as a tree walking mechanism. Walking the tree is expensive, so we do it once and effectively run all of the lint rules simultaneously. As a result,

_Visitor should extend SimpleAstVisitor,

you should use addConfiguration rather than addCompilationUnit on line 43, and

you should remove the invocation of super

Ah, apparently the file I duplicated as a starting point was the only one using a RecursiveAstVisitor (missing_whitespace_between_adjacent_strings.dart) 😄

This makes sense and I've made those changes, thanks!

That rule should definitely be updated. (But obviously not an issue for this PR.)

Ah, wasn't sure if there was a reason. I'll file a PR, seems like a trivial change. Thanks!

lib/src/rules/missing_conditional_import.dart

pq

💎

Awesome!

DanTup · 2021-12-02T18:27:18Z

Should I update pubspec.yaml to 1.16.0 too, or is it done when ready to release?

pq · 2021-12-02T18:29:14Z

Should I update pubspec.yaml to 1.16.0 too, or is it done when ready to release?

No need. I'll do that when I get the release cooking.

DanTup · 2021-12-02T18:39:53Z

Ok, cool :)

I don't have permissions to merge here (or know the procedure) so I'll leave that to one of you two when appropriate. Thanks!

dart-lang/linter#3080

…#3080) * Add a lint for missing files in conditional imports * Rename missing_conditional_import -> conditiona_uri_does_not_exist * Switch to SimpleAstVisitor * Improve Good/Bad examples * Add a comment above server overlays

Add a lint for missing files in conditional imports

a5382e1

google-cla bot added the cla: yes label Dec 2, 2021

bwilkerson reviewed Dec 2, 2021

View reviewed changes

pq reviewed Dec 2, 2021

View reviewed changes

lib/src/rules/missing_conditional_import.dart Outdated Show resolved Hide resolved

DanTup added 3 commits December 2, 2021 17:59

Rename missing_conditional_import -> conditiona_uri_does_not_exist

a399507

Switch to SimpleAstVisitor

e850de7

Improve Good/Bad examples

e967a02

bwilkerson approved these changes Dec 2, 2021

View reviewed changes

Add a comment above server overlays

aabc623

pq approved these changes Dec 2, 2021

View reviewed changes

DanTup mentioned this pull request Dec 2, 2021

Change missing_whitespace_between_adjacent_strings to use SimpleAstVisitor #3081

Merged

pq merged commit af0f8c8 into dart-lang:master Dec 2, 2021

mockturtl added a commit to mockturtl/tidy that referenced this pull request Dec 19, 2021

feat: conditional_uri_does_not_exist

9d1b7f8

dart-lang/linter#3080

DanTup mentioned this pull request May 8, 2023

Conditional imports not checked Dart-Code/Dart-Code#4525

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a lint for missing files in conditional imports #3080

Add a lint for missing files in conditional imports #3080

DanTup commented Dec 2, 2021 •

edited

Loading

coveralls commented Dec 2, 2021 •

edited

Loading

bwilkerson Dec 2, 2021

DanTup Dec 2, 2021

bwilkerson Dec 2, 2021

bwilkerson Dec 2, 2021

DanTup Dec 2, 2021

bwilkerson Dec 2, 2021

DanTup Dec 2, 2021

bwilkerson Dec 2, 2021

DanTup Dec 2, 2021

bwilkerson Dec 2, 2021

bwilkerson Dec 2, 2021

bwilkerson Dec 2, 2021

DanTup Dec 2, 2021

bwilkerson Dec 2, 2021

DanTup Dec 2, 2021

pq left a comment

DanTup commented Dec 2, 2021

pq commented Dec 2, 2021

DanTup commented Dec 2, 2021

Add a lint for missing files in conditional imports #3080

Add a lint for missing files in conditional imports #3080

Conversation

DanTup commented Dec 2, 2021 • edited Loading

coveralls commented Dec 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pq left a comment

Choose a reason for hiding this comment

DanTup commented Dec 2, 2021

pq commented Dec 2, 2021

DanTup commented Dec 2, 2021

DanTup commented Dec 2, 2021 •

edited

Loading

coveralls commented Dec 2, 2021 •

edited

Loading