dart-lang / language

Design of the Dart language
Other
2.66k stars 204 forks source link

Using element model instead of AST for declarations phase #3426

Open scheglov opened 11 months ago

scheglov commented 11 months ago

Is there a reason for the declarations phase to return superclass from AST node, not from the element? The difference is that for this code:

@IntrospectDeclarationsPhaseMacro()
class X {}

From ClassDeclaration we don't see any superclass, so see

class X

But if we use ClassElement, we see all type annotations resolved, so get

class X
  superclass
    class Object

Similarly for unresolved types - it makes sense for the types phase to see them, maybe the macro will create these types. But we don't get new types during the declarations phase, and the analyzer drops these from the element model, so for

@IntrospectDeclarationsPhaseMacro()
class X extends A {}

we would get

class X

...without and sign that there was A.

This view would be consistent with the view that we get for types from imported libraries, which are not part of the same library cycle. For these libraries we don't have AST view, only element model's one.

@jakemac53

jakemac53 commented 11 months ago

In general these APIs are "syntactic", and not "semantic", if that makes sense. This is primarily meant just to simplify things. It is easier to say that superclass is nullable and does not exist if there is no explicit extends, than it is to say that it is nullable in the types phase but non-nullable in the declarations phase (and will have the inferred Object if no extends is present).

I agree that it would be safe though, after the types phase, to specify that it is actually not nullable. But we couldn't easily enforce that statically without making the API much more complex (adding a new interface for ClassDeclaration in the declarations phase?).

This view would be consistent with the view that we get for types from imported libraries, which are not part of the same library cycle. For these libraries we don't have AST view, only element model's one.

It sounds like there is an implementation issue here, where summaries don't have enough information to distinguish between an "explicit" versus "implicit" superclass (when it is Object)?

We could potentially change how this works (in the macro spec/apis), or even possibly allow compilers to be a bit loose here, where we allow them to give Object as the superclass for dependencies (that are not in the current library cycle)?

scheglov commented 11 months ago

OK, I understand that we provide "syntactic" APIs.

Yeah, it is probably impractical to add DeclarationsClassDeclaration, so we will have to accept that it can be nullable. In the analyzer we still have to make superclass nullable, because it is in Object itself.

summaries don't have enough information

The element model describes the semantics of the code, and for class A {} without extends the specification says that extends Object is implied. It would be complicated to track unresolved types in implements, and if we don't do it there, then it probably does not make sense to do for implicit extends Object.

We could potentially change how this works (in the macro spec/apis), or even possibly allow compilers to be a bit loose here, where we allow them to give Object as the superclass for dependencies (that are not in the current library cycle)?

This would be nice. We don't have "syntactic" view for dependencies.

Currently we already use "syntactic" view for the current library cycle in the analyzer, for all phases. I just tried to explore if we have to, or can switch to "semantic". But if this is not what we want, I can leave it as is :-)

jakemac53 commented 11 months ago

Currently we already use "syntactic" view for the current library cycle in the analyzer, for all phases. I just tried to explore if we have to, or can switch to "semantic". But if this is not what we want, I can leave it as is :-)

I think we can't, because within the types phase an extends can be added (actually, I am not sure that the API exists for this yet, but it is intended to be supported, or at least augmentations allow it).

It is unfortunate that all of this means you have to support creating a ClassDeclaration from both the AST and Element models though, but I am not sure of a way to work around it.

This would be nice. We don't have "syntactic" view for dependencies.

I will work on figuring out a long term solution for you. For now I would just give Object as the superclass after the types phase, and maybe link a TODO with this bug so we can come back to it.

davidmorgan commented 3 months ago

Not sure if there is anything specific left to do on this issue? But we are going to have to answer all questions of this type for dart_model, if they are not already answered.

jakemac53 commented 3 months ago

Fwiw there is now a macro API for adding extends.

My main concern here is ensuring we have consistency across tools. Because of that I don't think we can say that when a class is loaded from a summary file you can see the implicit Object superclass, unless we can somehow express that behavior in a tool agnostic and compilation mode (modular vs incremental vs whole world) agnostic way.