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

Dyno: Type constructors for inheriting generic classes #25778

Merged
merged 13 commits into from
Aug 23, 2024

Conversation

benharsh
Copy link
Member

@benharsh benharsh commented Aug 19, 2024

This PR implements type constructors for inheriting generic classes, which requires some substantial changes to the way type constructors are implemented in dyno.

Prior to this PR, we used a compiler-generated signature that re-used the uAST and IDs for fields in place of real formals. This breaks down when inheriting classes are involved because the parent fields will have different symbol paths from the child's fields, which can confuse the rest of dyno's infrastructure. This especially causes issues for the way we store resolution results, which cannot account for IDs from different symbol paths.

This PR addresses this problem by generating dedicated uAST for the type constructor, rather than trying to fake formals with the uAST of fields. Generating uAST on the fly like this is new for dyno, and has required some additional infrastructure. The notable changes are as follows:

  • Ability to copy uAST
  • Adds ID::FabricatedKind::Generated to indicate non-specific compiler-generated IDs
  • Storing/fetching a BuilderResult for compiler-generated uAST based on a symbol path of a compiler-generated module
  • Updating "id to ast" code to handle compiler-generated IDs

With these capabilities in place, typeConstructorInitialQuery now builds a small module to store a uAST Function that serves as the type constructor for a given type. This module pretends to be an included module living under the module in which the type was declared. For example, if a type 'R' was defined in module 'foo', the generated module would have the symbol path of foo.chpl__internal_foo_R.

This module also includes a 'use' statement for the original module, which is necessary in the event that fields' type/init expressions refer to other symbols.

Once finished building the uAST, we store the BuilderResult based on the module path.

This PR updates the implementation of IDs to treat postOrderId values less than -2 as compiler-generated IDs, but returning said IDs as a positive 0-based value so as not to confuse the rest of resolution. Internally, a value of -3 represents a compiler-generated symbol, and anything less than -3 is a child of a compiler-generated symbol, and will have its value translated into the proper positive value upon a call to ID::postOrderId(). This change was made to more easily support id-to-ast lookups in which we want to quickly determine whether an ID has an actual file behind it, or if it requires special handling because it was compiler-generated.

Other changes:

  • Updated CompositeType::stringify to print generic parent substitutions
  • A new resolution query findFieldIDByName is added to support these changes

Testing:

  • test-dyno

@benharsh benharsh requested a review from DanilaFe August 19, 2024 19:53
@benharsh
Copy link
Member Author

A potential alternative approach would be to generate this uAST at parse-time when we encounter a record or class, and instead treat it as some kind of "hidden" uAST associated with a specific file.

Copy link
Contributor

@DanilaFe DanilaFe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not all the way through just yet, but had some comments in the meantime

frontend/include/chpl/resolution/resolution-queries.h Outdated Show resolved Hide resolved
frontend/lib/resolution/resolution-queries.cpp Outdated Show resolved Hide resolved
frontend/lib/resolution/resolution-queries.cpp Outdated Show resolved Hide resolved
frontend/lib/resolution/resolution-queries.cpp Outdated Show resolved Hide resolved
frontend/lib/resolution/resolution-queries.cpp Outdated Show resolved Hide resolved
Comment on lines 208 to 218
UniqueString symbolPath = id.symbolPath();

while (!symbolPath.isEmpty()) {
auto tupleOfArgs = std::make_tuple(symbolPath);
auto got = context->hasCurrentResultForQuery(compilerGeneratedBuilderQuery, tupleOfArgs);
if (got) {
const BuilderResult& p = getCompilerGeneratedBuilder(context, symbolPath);
return &p;
}
symbolPath = ID::parentSymbolPath(context, symbolPath);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a lot of work (for deeply nested IDs, it will keep trying to check the query database for more results, even if the ID is not compiler generated). Don't you already know if the ID is compiler-generated from its postorder ID being negative? It seems like you can skip this entire path if it isn't negative.

Also, can compiler-generated modules like this be nested? Not currently, right? In that case, you may be able to do this way more efficiently by finding the outermost symbol and trying with it directly. I suspect you can use logic in parentSymbolPath as inspiration to do so; perhaps finding the first '.' (if any) and using that?

Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
typeConstructorInitialQuery now builds a small module to store a
uAST Function that serves as the type constructor for a given type. This
module pretends to be an included module living under the module in
which the type was declared. For example, if a type 'R' was defined in
module 'foo', the included module would have the symbol path of
``foo.chpl__internal_foo_R``.

This module also includes a 'use' statement for the original module,
which is necessary in the event that fields' type/init expressions refer
to other symbols defined in that module.

Once finished building the uAST, we store the BuilderResult based on the
module path.

Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
Previously we were storing field IDs in compiler-generated type
constructor signatures. This was relied upon in a couple of places in
resolution to easily deal with substitutions and CompositeTypes.

To support the required behavior, a new query ``findFieldIDByName`` is
introduced.

Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
We now represent a compiler-generated symbol differently than we do for
a normal symbol (e.g. -1), so that all fabricated IDs are <= -2.

Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
@benharsh benharsh merged commit 45bb1fd into chapel-lang:main Aug 23, 2024
7 checks passed
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

Successfully merging this pull request may close these issues.

2 participants