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

Analyzer doesn't properly handle parts generated using build_to:cache #51087

Closed
greglittlefield-wf opened this issue Jan 20, 2023 · 16 comments
Closed
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P1 A high priority bug; for example, a single project is unusable or has many test failures

Comments

@greglittlefield-wf
Copy link
Contributor

greglittlefield-wf commented Jan 20, 2023

When a builder generates parts using build_to:cache instead of build_to:source, and the parts use URIs in their part of directives, the part files don't get recognized correctly and you get a part_of_different_library error.

This issue seems to be specific to the analyzer, and does not seem to impact running in the VM or compiling to JS files that depend on those generated parts.

For example, if you have a file foo.dart which pulls in a generated file:

part 'foo.g.dart';

// ...

and the generated file looks like:

part of 'foo.dart';

// ...

Then you get an error on the part URI in the foo.dart file that looks like:

Expected this library to be part of
'file:///<path_to_package>/example/foo.dart', not
'file:///<path_to_package>/.dart_tool/build/generated/foo_package/example/foo.g.dart'.
Try including a different part, or changing the name of the library in the part's part-of directive.

I put together a package with a simple builder that reproduces this issue, which can be seen on this line.

However, if the generated part points to the main library using its library name, there's no error. For example, if you have a file foo.dart which pulls in a generated file:

library foo;

part 'foo.g.dart';

// ...

and the generated file looks like:

part of foo;

// ...

In that case, everything seems to work fine. Example of that behavior here.

Impact

This impacts consumers of over_react, which uses build_to:cache for its generated code.

It'd be ideal if this issue were fixed in the Dart SDK, and released in a 2.x version. I'd be happy to attempt to contribute a fix if that would be helpful!

If that's not feasible, then the following workarounds come to mind:

  1. switching to build_to:source
  2. generating parts using library names if they're available, and encouraging consumers to declare names on source libraries

While switching to build_to:source would be possible, it'd be no small mount of work to migrate consumers. I think ideally we'd stick with build_to:cache until Dart macros are available :).

Dart SDKs

I tested this issue in different stable Dart SDKs until I could narrow down where behavior changed.

In Dart <=2.15.1, this worked as expected.

In Dart >=2.16.0 <=2.17.7, part directives in libraries that point to generated parts have uri_has_not_been_generated errors, regardless of whether a part uses a URI or library.

In Dart >=2.18.0, part directives in libraries that point to generated parts have part_of_different_library errors, but only if those parts uses URI in their part of directives, and not if they use library names.

This issue is still present the beta (2.19.0-444.4.beta) and dev (3.0.0-128.0.dev) channels as well.

You can see CI runs of that reduced test case package on different Dart SDKs here.

@bwilkerson bwilkerson added P1 A high priority bug; for example, a single project is unusable or has many test failures area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. labels Jan 20, 2023
@asashour

This comment was marked as outdated.

@asashour
Copy link
Contributor

sdk/279395 contains a failing test.

Please note that if the folder is lib, then the test passes.

I wonder whether the files outside lib should be resolved or not. Currently both analyzer and CFE seem to not handle them.

@bwilkerson
Copy link
Member

I'm confused. The original post says that the part file contains:

part of 'foo.g.dart';

Is that a typo, or does the part actually claim to be a part of itself? I would have expected the generated file to contain

part of 'foo.dart';

@greglittlefield-wf
Copy link
Contributor Author

@asashour Oh good catch! I didn't realize the issue didn't occur for files inside lib, sine when I first found the issue it was in a package where the builder was only applied outside of lib.


Sorry about that @bwilkerson, that is indeed a typo; it should be:

part of 'foo.dart';

I'll upate the original post

@bwilkerson
Copy link
Member

Does the same error message get generated? I ask because the error message indicates that analyzer thinks foo.g.dart is a part of foo.g.dart, which shouldn't be the case if the file declares itself to be a part of foo.dart.

@greglittlefield-wf
Copy link
Contributor Author

Yes, it does. Yeah, I was confused by that as well

@bwilkerson
Copy link
Member

@scheglov Does the analyzer know how to handle files in .dart_tool/build/generated?

(I haven't been able to reproduce the original issue. I get a different error message suggesting that my environment isn't complete.)

@greglittlefield-wf
Copy link
Contributor Author

I haven't been able to reproduce the original issue. I get a different error message suggesting that my environment isn't complete.

Oh sorry, I forgot to mention that sometimes I would have to run "Reanalyze Dart Sources" in my IDE to get it to pick up on the changes after running a build. I can't recall exactly, but there may have been a few times where even that didn't work. That's one of the reasons I put up the CI runs in that reduced test case package

@scheglov
Copy link
Contributor

Thank you for the reproduction as a unit test, @asashour!

Yes, it looks inconsistent that we can resolve part 'foo.g.dart'; to a path in .dart_tool/build/generated, but then cannot convert back path inside .dart_tool/build/generated into the "canonical" file URI. We do this for package URIs and paths though.

@scheglov
Copy link
Contributor

copybara-service bot pushed a commit that referenced this issue Jan 23, 2023
Bug #51087

Change-Id: I9ee382bb5abe05624e7b61783bf4f50214d67ab0
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/279395
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
copybara-service bot pushed a commit that referenced this issue Jan 23, 2023
…hToUri()

Bug: #51087
Change-Id: I5f56dc5c82e22a71b3d871ae2ef51f2acfa5eee1
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/279562
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
@bwilkerson
Copy link
Member

The fix is on bleeding edge. If you have a chance to try using an SDK built from bleeding edge, please let us know whether the problem still occurs. If we know that it's fixed we can investigate cherry picking into 2.19. Otherwise the change won't reach users until 3.0.

@asashour
Copy link
Contributor

I tested it with the package in the OP and it works with the analyzer. However, CFE ignores both scenarios, please see #51105

@robbecker-wf
Copy link

robbecker-wf commented Jan 23, 2023

Could I request this be patched to a 2.18.8 release as well as 2.19 🙏 ?

@asashour
Copy link
Contributor

The CFE works fine, my testing for it was incorrect. So if this seems important, it should be probably cherry picked.

@greglittlefield-wf
Copy link
Contributor Author

In addition to @asashour's testing, I reran a build on that test package with a dev build of the Dart SDK that included this fix, and it passed! https://github.com/greglittlefield-wf/generated_part_issue_repro/actions/runs/3991654264/jobs/6902940045

Thanks for fixing the issue so quickly! If it could be cherry-picked into 2.19, that would be awesome. Like @robbecker-wf mentioned, 2.18 would've been ideal but we'll take what we can get 🙂.

For more context on why 2.18, since our build fails on 2.19 due to our dependency graph still being on 1.x of the analyzer package, which throws when analysis hits constructor tearoffs in the Dart 2.19 core library. We'll just need to upgrade analyzer further.

@kevmoo
Copy link
Member

kevmoo commented Jan 26, 2023

A 2.19 cherry pick for a dot release is absolutely on the table.

copybara-service bot pushed a commit that referenced this issue Feb 7, 2023
…olver.pathToUri()

Bug: #51087
Change-Id: I1d141ef918fa53a86f31d331f6586367dfb99953
Cherry-pick: https://dart-review.googlesource.com/c/sdk/+/279562
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/280045
Commit-Queue: Kevin Chisholm <kevinjchisholm@google.com>
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P1 A high priority bug; for example, a single project is unusable or has many test failures
Projects
None yet
Development

No branches or pull requests

6 participants