dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.24k stars 1.58k forks source link

Analyzer: Optionally expose the unresolved type #47853

Open rrousselGit opened 2 years ago

rrousselGit commented 2 years ago

Hello!

Problem

As the author of the code-generator Freezed, I'm facing a common problem: It is difficult to handle cases where generated code depends on another generated code

For example, a user may use a code-generator to generate the class Generated. Then, use that Generated class in the input code that a generator will use, such as:

@annotation
class Something {
  final Generated property;
}

The problem is, since Generated doesn't exist when the code-generator that will handle the Something class runs, then the analyzer will send "dynamic" to that generator instead of infos on Generated.

That's understandable. The problem is, this will cause the generator to generate incorrect code. For example, if we wanted to generate a copyWith for Something, then instead of:

Something copyWith({Generated? property}) {...}

the generated code would likely be:

Something copyWith({dynamic property}) {...}

Proposal

To help reduce this issue, could we expose the unresolved type? Of course, obtaining things like superType & all would not be possible. But there are other methods on DartType/Element that would still likely be usable, such as

bwilkerson commented 2 years ago

While I'm not necessarily arguing against adding this information to the element model, I'll point out that this information is already available in the AST, and hence there is a workaround for not having it in the element model.

I'll also point out that adding this information to the element model might not be as trivial as it sounds. While the element model is intended to reflect the semantics of the code, there is no clear semantics for invalid code. So, how do we decide what kind of element model to produce for invalid code? The primary requirement we have is for the analyzer to produce the best UX possible, and one aspect of that is to recover gracefully from invalid code.

Consider the following:

@annotation
class Something {
  final Generated property; // 1
}
void f(Something s) {
  g(s.property); // 2
}
void g(SuperclassOfGenerated x) { ... }

The analyzer handles this by reporting at 1 that the type Generated isn't defined, as you'd expect. By treating the type of property as dynamic it then doesn't report anything at 2, which seems better than reporting that the type Generated can't be assigned to the type SuperclassOfGenerated given the fact that the analyzer can't actually know at this point whether that claim is true or not.

We could certainly change the element model and report the type of property as being an invalid type named Generated, test at 2 to see whether the type being assigned is an invalid type, and use that information to suppress the generation of a diagnostic. That would preserve the current UX.

I could even argue that this would be a better representation because we'd then be able to know whether something was intentionally or accidentally dynamic in some cases. (We'd probably still need to fall back to dynamic for an expression like s.property.value because we'd have no way of knowing which invalid type to use at that point.)

But whether it's a better representation or not, the analyzer code base, and everything written on top of it, is currently getting this kind of recovery essentially for free. We'd need to find all of the places where that would no longer be the case and add handling for this new concept of an invalid type. Doing so would not be a small amount of work, either for us or for any other packages that use this behavior from the analyzer.

I'm not certain whether the benefits would justify the cost, especially given that

rrousselGit commented 2 years ago

Note that I wasn't asking about changing the current behavior. It definitely should still fallback to dynamic by default.

My thought was that this would be purely additive. Such as:

DartTye type; // some type obtained from somewhere
print(type.getDisplayString()); // dynamic

print(type.unresolvedType!.getDisplayString()); // Generated<int>

This would not break any existing code, yet the possibility would be there.

static metaprogramming, if it happens, would solve the underlying problem (of generators depending on generated code) in a much more complete way. (And yes, I understand that even if the proposal is approved it will be a long time before the feature is available, and hence something like this might be a useful stop-gap measure.)

This is debatable. Some cases would certainly go away, but I doubt that all will.

For example with Freezed, folks can write:

@freezed
class Example<T> with _$Example<T> {
  factory Example.first() = A;
  factory Example.second(A<int> value ) = B;
}

This snippet will generate two classes: Aand B. The thing is, B depends on A.
That case wouldn't be caught by static metaprogramming since both classes are generated at the same time.

The only solution would be to maybe use the AST here, but there are talks about preventing metaprogramming from accessing the AST, so it may not be reliable

bwilkerson commented 2 years ago

My thought was that this would be purely additive.

Thanks for the clarification. That makes a big difference.

... but there are talks about preventing metaprogramming from accessing the AST, so it may not be reliable

To the best of my knowledge, the APIs for metaprogramming will not be the analyzer APIs, no matter what the metaprogramming APIs provide access to, so changes to the analyzer APIs won't help in that case anyway.