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

Guidance on how to compare Dart types from independent libraries? #740

Open
simolus3 opened this issue Jan 20, 2025 · 9 comments
Open

Guidance on how to compare Dart types from independent libraries? #740

simolus3 opened this issue Jan 20, 2025 · 9 comments

Comments

@simolus3
Copy link
Contributor

I have a builder that needs to know how types in its input relate to some known static types. For this, it currently runs two analysis steps:

  1. resolver.libraryFor('package:drift/src/drift_dev_helper.dart'): This library exports all the base types that my builder cares about.
  2. resolver.libraryFor(buildStep.inputId): To resolve the element model for the input. inputId does not transitively import the helper library from step 1.

Then, I use TypeChecker.fromStatic with the types I've received in step in 1 to check whether classes resolved in step 2 are subtypes.
This used to work quite well, but it got broken after upgrading to analyzer 7.0. Even for classes that are subtypes, the type checker sometimes reports no subtype relationship.

After some debugging, I've determined a likely root cause: The analysis session from steps 1 and 2 are different, so all the element models aren't equal as far as the analyzer is concerned. It makes sense that they might be different, because each libraryFor call might cause the analysis driver to discover new libraries that would make it start a new session.

In my builder, I really need an API that would be able to resolve multiple independent Dart libraries under a single coherent element model.
I've tried repeating step 1 again in the end in the hope that it wouldn't refresh the session another time, but I didn't have any luck with that.

As far as I know, there currently isn't an analyzer API that could surface unified information about multiple libraries safely. So I'm wondering if there should be one? Or am I doing something wrong here? Does anyone have suggestions for a better workaround?

@jakemac53
Copy link
Contributor

cc @scheglov as this is really an analyzer question I think

@scheglov
Copy link
Contributor

You need to defined yourself what it means to be equal for types based on elements from different analysis sessions.
For example

bool areEqualInterfaceTypes(InterfaceType left, InterfaceType right) {
  return left.element.library.source.uri == right.element.library.source.uri &&
      left.element.name == right.element.name;
}

@jakemac53
Copy link
Contributor

cc @kevmoo it looks like probably https://github.com/dart-lang/source_gen/blob/master/source_gen/lib/src/type_checker.dart#L225 needs to be updated to check via URI

@jakemac53 jakemac53 transferred this issue from dart-lang/build Jan 21, 2025
@kevmoo
Copy link
Member

kevmoo commented Jan 21, 2025

This is EXACTLY what I'm hitting here GoogleCloudPlatform/functions-framework-dart#464

I'd love a general solution here, too!

My problem: I want it to work for generics, too – which is really tricky.

@simolus3
Copy link
Contributor Author

You need to defined yourself what it means to be equal for types based on elements from different analysis sessions.

That's convenient for simple equality checks and what I ended up doing, but it's really just a stopgap solution.
Builders may have more complex type queries they need to run (I'm also using TypeSystem.isAssignableTo and InterfaceType.asInstanceOf for instance). These APIs are not compatible with that notion of equality and I hope it's not my responsibility as an analyzer user to re-implement a TypeSystem capable of comparing elements across sessions.

I guess another builder-level workaround might be to have a preparing builder emit a temporary Dart file that imports the two Dart files on which we need to run analysis. Resolving that file would give us a single model across the two libraries, but it's also rather annoying to set up since it requires another build step. I wonder if Resolver could pull some internal tricks to possibly make a Future<Map<AssetId, LibraryElement>> consistentLibrariesFor(Iterable<AssetId>) API happen.

@scheglov
Copy link
Contributor

I don't know how you get all these other elements, types, and checkers.

Let's consider a specific example. I don't write builder myself, so I will say potentially something wrong. Let's say you are in a builder, and you have a ClassElement named C that you want to check that it implements a specific ClassElement named A from the library L. Your C can give you access to AnalysisSession, which you can ask for L and then for A.

@simolus3
Copy link
Contributor Author

Your C can give you access to AnalysisSession, which you can ask for L and then for A.

If I remember correctly, using getLibraryByUri in builders is frowned upon because it might circumvent the asset tracking mechanism of the build system (all file reads need to happen through build APIs for correct rebuilds). There might even be active protection against this (from my understanding, build_resolvers uses a custom MemoryResourceProvider that only makes files visible to the driver after they have been requested through build APIs).

In my case, the paths are something like:

1. Class C in Library L' -> imports Library L providing class A

2. My builder resolves Library L'' -> exports A from Library L

So when I have an analysis session for L' and ask about L'' which the analysis driver has never seen before, it might invalidate the session (internally, build_resolver calls driver.changeFile() and driver.applyPendingFileChanges when a new asset should become visible to the analyzer through endorsed build APIs. It then uses driver.currentSession.getLibraryByUri() which has a new session).

@scheglov
Copy link
Contributor

I'm surprised that there is a need to invoke driver.changeFile() when the builder asks for L''.
If the builder is allowed to access L'', then it has always been visible to the builder.
And a general sense that it is weird when reading causes writing.

@jakemac53
Copy link
Contributor

jakemac53 commented Jan 22, 2025

If the builder is allowed to access L'', then it has always been visible to the builder.

The problem is that we have many builders running at the same time and they each have different visibility rules. We don't run the build phases in order, we "pull" from the outputs that were asked for, so that we only build the required inputs for those outputs.

Even within a single builder, if it emits a file it is then allowed to read that file, in the same build step, so its visibility rules can change as it runs (an asset can move from not visible to visible).

This is obviously highly problematic when it comes to the analyzer integration, and we actually do paper over a lot of this and accept some level of inconsistency - we will never "remove" a file in the middle of a build, only add new ones. So it is possible for a builder to actually get an element model for something it shouldn't be able to see, if some other builder already asked to resolve that library and did have access to it. But, this means any time a builder asks for an analysis context we have to check if we need to add any files that it can see, but no previous builder could see.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants