Sdk: check for valid paths in pubspec validation

[pq]

Note that this check is currently provided by IntelliJ:

but presumably not in VS Code (@DanTup?)

Needless to say, any clients that are providing this analysis currently will need updating once his lands in server.

/fyi @alexander-doroshko @jwren @bwilkerson @jonasfj

[pq]

I was initially thinking it would be awkward to land server support before IDEA stopped reporting but the UX isn't too bad.

Of note:

1. When you hover, you get the IDEA inspection (probably because it's an "error" not a warning) -- when it goes away, you'll see ours.
2. The IDEA inspection does not show up in the analysis window (so there are no duplicates).

https://dart-review.googlesource.com/c/sdk/+/168882

[alexander-doroshko]

@pq Thanks for the progress with this feature!

FYI, some info about existing path inspection in InteliJ:

• it checks that the path is valid (relative or absolute) and points to a directory, not to a file
• it checks that the path is not referring to the current project itself (e.g. not like "../this-project")
• it checks that the referenced folder contains a file named pubspec.yaml
• it doesn't check that the referenced folder contains lib subfolder but I guess this check would make sense as well
• IMPORTANT: IntelliJ is able to analyze unsaved (edited) pubspec content. AFAIK Analysis Server can work with unsaved Dart files but not with unsaved analysis_options.yaml or pubspec.yaml. IntelliJ sends AnalysisServer.analysis_updateContent() requests for all these files but currently server doesn't respect these requests. It would be great if it did. Otherwise paths may stay yellow in the editor (and in the Dart Analysis tool window) even if already fixed by a developer.

[bwilkerson]

AFAIK Analysis Server can work with unsaved Dart files but not with unsaved analysis_options.yaml or pubspec.yaml. IntelliJ sends AnalysisServer.analysis_updateContent() requests for all these files but currently server doesn't respect these requests.

That isn't quite true. When server receives an analysis_updateContent notification for a Dart file it creates an override in the resource provider (https://github.com/dart-lang/sdk/blob/master/pkg/analysis_server/lib/src/analysis_server.dart#L497). I don't see anything in the code to suggest that this only happens for Dart files, so I have every reason to believe that we are, in fact, capturing this information.

That same method is responsible for triggering any analysis that might be necessary as a result of the change (the resource provider doesn't generate a watch event for overlays, though I've wondered a few times whether that might not be the better design), but it only triggers re-analysis for Dart files. All we need to do is trigger analysis of known non-Dart files to make this work as expected.

[alexander-doroshko]

@bwilkerson Thanks for the clarification! I was under an incorrect impression after reading this comment in https://github.com/dart-lang/sdk/issues/25114#issuecomment-697397940

Looks like it's possible to fix this limitation for both pubspec.yaml and anaysis_options.yaml. It is already tracked as https://github.com/dart-lang/sdk/issues/31890

[pq]

Thanks for the feedback @alexander-doroshko !

@jonasfj, do these validations look right?

• it checks that the path is valid (relative or absolute) and points to a directory, not to a file
• it checks that the path is not referring to the current project itself (e.g. not like "../this-project")
• it checks that the referenced folder contains a file named pubspec.yaml

Is there a pub validator that checks paths on publish?

EDIT: also,

it doesn't check that the referenced folder contains lib subfolder but I guess this check would make sense as well

agreed?

[pq]

Re-opening to keep discussion alive.

[DanTup]

but presumably not in VS Code (@DanTup?)

Correct - VS Code doesn't produce any diagnostics besides those that come from the server.

Tested this with VS Code in the nightly build and it works - though it does have the limitation mentioned above (doesn't update until you save - I verified VS Code is sending the deltas as you type).

[jonasfj]

• it checks that the path is valid (relative or absolute) and points to a directory, not to a file

Yes,

• it checks that the path is not referring to the current project itself (e.g. not like "../this-project")

Yes, this makes no sense to do, so I think that's fine :D

• it checks that the referenced folder contains a file named pubspec.yaml

Yes, this is a strict requirement for pub get.

it doesn't check that the referenced folder contains lib subfolder but I guess this check would make sense as well

No, This is not a strict requirement. It will have false positives:

• asset packages, such as an icon set,
• tool packages, such as build_runner doesn't necessarily have to contain any lib/ folder.

This is not a requirement for pub get. Checking for a lib/ folder wouldn't be horrible, because it's very rare for a package to not have a lib/ folder, even if it's a tool or and asset collection.

But my two cents on whether to check for lib, is that we shouldn't.

Checking that the path referenced exists and contains a pubspec.yaml might be fine. Presence of a pubspec.yaml is a strong indicator that a folder is a package :D

But it might be best to not go deeper into validating that the referenced path in fact contains a useful or valid package, ie. that is has a lib/ folder or that it has SDK constraints that allows current SDK version, etc.

---

Is there a pub validator that checks paths on publish?

Yes, when publishing a package dependencies may not contain path or git dependencies (only hosted dependencies).
However, this might be a different lint. Because:

• it's only relevant when publishing the package,
• it makes sense that the same lint warning about path dependencies in packages aimed at publishing, should also warn about git dependencies.
• hence, it doesn't have to do with the path-dependency configuration, it has more to do with the fact that publishing with a path or git dependency means that consumers might not be able to get the same dependency.

[sigurdm]

We could also consider checking that the package name in the pubspec.yaml corresponds with the name of the package we are importing. I believe that is a necessity for everything to line up (we don't do package renaming).

[pq]

Thanks for all the feedback! With basic path checking implemented, I'll close this issue in favor of targeted follow-ups. Feel free to create more.

Cheers!

[alexander-doroshko]

@pq Thanks! #31890 is the most important I think :)