Dyno: Refactor uAST generation #26145
Conversation
These do not fit well with the new way of representing generated uAST in the frontend. Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
Previously, generated uAST was being created given a particular type possibly with other information about how the call was being invoked. Generated uAST was then created as a fake 'included' module with a parent symbol path was that of the module in which the type was defined. This uAST was then stored and retrieved using some set/get queries in ``chpl::parsing``. This commit updates the implementation to instead rely on generated functions using the type's symbol path as its parent path. This allows ``parseFileContainingIdToBuilderResult`` to use a generated ID's symbol path and symbol name to query the uAST, as opposed to using the old getter method. Now, we rely on the ``buildDefaultFunction`` query in ``chpl::resolution``. For example, a default initializer will have a path like ``MyMod.MyType.init``. With the parent symbol path of ``MyMod.MyType`` we can query the type's uAST if needed, and with the name ``init`` we know which uAST-generating function to invoke. This allows us to get rid of the clunky 'chpl__generated' prefix to generated modules. We also now need to avoid generating an implicit module for generated uAST and instead have a Function as the top-level expression in a Builder. Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
…stem Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
This commit updates buildTypeConstructor to build a type constructor given only a type's ID. Now that the untyped signature is not pointing to non-existent uAST, we can also stop using the original type's ID in the untyped signature. This requires some minor modifications to return-type-inference now that 'idIsFunction' is returning true for type constructors. Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
Parent symbol IDs for generated functions now refer to actual non-generated uAST, meaning that the returned ID should not itself be considered 'generated'. Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
This commit updates Builder to store a boolean indicating whether the uAST is generated rather than storing the old 'generatedFrom' ID. The necessary information from this ID can and should be stored in the pre-existing ``startingSymbolPath_`` field. After some experimentation it turns out that BuilderResult no longer needs a ``generatedFrom`` ID to use for the id-to-parent map. This is because we are now correctly setting the parent path for generated IDs in ``parseFileContainingIdToBuilderResult``. Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
| @@ -142,7 +156,8 @@ ID ID::parentSymbolId(Context* context) const { | |||
| } | |||
|
|
|||
| if (this->isFabricatedId() && | |||
| this->fabricatedIdKind() == FabricatedIdKind::Generated) { | |||
| this->fabricatedIdKind() == FabricatedIdKind::Generated && | |||
| !isSymbolDefiningScope()) { | |||
There was a problem hiding this comment.
If I understand correctly, this assumes that it's not possible to have a generated ID within another generated ID. That's probably fine, but needs to be documented as a constraint somewhere. Potentially as a CHPL_ASSERT somewhere. Or as a comment.
There was a problem hiding this comment.
Technically the assumption is that generated uAST will not contain nested scope-defining symbols. I've pushed a commit containing both comments and an assertion to enforce this.
| if (postOrderId == -1) { | ||
| pid = ID_GEN_START; | ||
| } else { | ||
| pid = ID_GEN_START - 1 - postOrderId; |
There was a problem hiding this comment.
Could you add a check that we're packing and unpacking these correctly in to testIds.cpp ? by calling generatedId and checking that postOrderId comes out right?
There was a problem hiding this comment.
Great suggestion! I had introduced a bug in a previous PR that is now fixed.
| /** | ||
| Given a type's symbol path and the name of a default function, return a | ||
| BuilderResult storing the generated default function uAST. | ||
| */ |
There was a problem hiding this comment.
It is interesting. At first I would think that this function should return a Function* ? I see that it returns a BuilderResult* to integrate into parseFileContainingIdToBuilderResult. IMO it might be worth renaming the function to emphasize this -- but at least it would be good to put a comment explaining that this is intended to be called by parsing queries that need to handle a BuilderResult* for generality.
There was a problem hiding this comment.
Renamed to builderResultForDefaultFunction.
| const BuilderResult* | ||
| buildDefaultFunction(Context* context, | ||
| UniqueString typePath, | ||
| UniqueString name) { |
There was a problem hiding this comment.
IMO this function and all of the functions implementing it should be stored in default-functions.cpp. I know that is slightly out of pattern (since the prototype is in resolution-queries.h) but that can be addressed with a comment here or by moving these to a new public header (so that we have a public chpl/include/default-functions.h and a private one in lib/resolution/ with a different name).
Alternatively, you can keep buildDefaultFunction but move the various functions implementing it to default-functions.cpp.
Either way, I think it's important to move the helper functions implementing buildDefaultFunction to default-functions.cpp both for find-ability and because resolution-queries.cpp can get overly large because of it's grab-bag nature.
There was a problem hiding this comment.
I moved builderResultForDefaultFunction and buildTypeConstructor into default-functions.cpp.
Firstly, fixes a bug in parentSymbolId where getting the parentSymbolId of a non-scope-defining symbol (e.g. an Identifier) would incorrectly return a generated ID with the grandparent symbol path. This does not seem to be exposed in any testing at this time. Adds some comments explaining the assumption that there is only one scope-defining symbol in generated uAST. Also adds an assertion to enforce this rule, which was convenient to add in doAssignIds. Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
…arity Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
…ult-functions.cpp Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
Previously, generated uAST was being created given a particular type
possibly with other information about how the call was being invoked.
Generated uAST was then created as a fake 'included' module with a
parent symbol path was that of the module in which the type was defined.
This uAST was then stored and retrieved using some set/get queries in
chpl::parsing.This PR updates the implementation to instead rely on generated
functions using the type's symbol path as its parent path. This allows
parseFileContainingIdToBuilderResultto use a generated ID's symbolpath and symbol name to query the uAST, as opposed to using the old
getter method. Now, we rely on the
buildDefaultFunctionquery inchpl::resolution.For example, a default initializer will have a path like
MyMod.MyType.init. With the parent symbol path ofMyMod.MyTypewe can query the type's uAST if needed, and with the name
initweknow which uAST-generating function to invoke (e.g.
buildInitializer).This change requires that Builders of generated code be able to support a
non-module as a top-level expression. This is achieved by disabling implicit
module creation for generated uAST. Currently it is expected that this
top-level expression will be a
Function.The recently-added compiler-generated assignment operators do not fit in
well with this new representation because the primitive types do not have an
ID associated with them. For now, it is simpler to remove those generated
operators and go back to using manually-created operators in testing. Future
work is expected to eliminate the need for such operators by using either the
standard or minimal modules.
Cleanup:
Testing: