Missing playground results for init/deinit in a class wrapped in a struct#77498
Merged
abertelrud merged 1 commit intoDec 4, 2024
Merged
Conversation
abertelrud
requested review from
hborla,
slavapestov and
xedin
as code owners
abertelrud
requested review from
chrismiles and
etcwilde
and removed request for
hborla,
slavapestov and
xedin
…class wrapped in a struct) The problem here is when wrapping a type that has constructors and destructors inside another type. The reason is that the playground transform uses an `ASTWalker` to get to the top-level structure, but then once it’s inside a type, it does directly nested transformDecl() calls. And that inner check was for too narrow of a type. The problem in this case was that the `dyn_cast<FuncDecl>(D)` was too narrow and didn’t include constructors/destructors. We want `dyn_cast<AbstractFunctionDecl>(D)`. We should probably resolve this duality (using an `ASTWalker` to get to the top-level decls but then using directly nested calls below) at some point, but that’s a larger change, and so this is worth addressing with a safer short-term fix. Changes: - change a `dyn_cast<FuncDecl>` to a `dyn_cast<AbstractFunctionDecl>` in PlaygroundTransform.cpp - add a unit test nested init and deinit (this test also tests the unnested case) rdar://137316110
abertelrud
force-pushed
the
eng/anders/137316110-nested-init-playground-results
branch
from
34c18ec to
3db28c8
Compare
Contributor
Author
|
I hdd accidentally included some temporary cherry-picks of @chrismiles's now-landed branch, so this latest update removed them. This is now ready for review. |
abertelrud
changed the title
init and deinit in a class wrapped in a struct)init/deinit in a class wrapped in a struct
Contributor
Author
|
@swift-ci Please smoke test |
Contributor
Author
|
Adding @hborla and @slavapestov who are also listed as code owners in a second PR I'm opening for the same source file (they were not in this list when I opened this PR, IIRC). |
chrismiles
approved these changes
Nov 11, 2024
xedin
approved these changes
Dec 4, 2024
abertelrud
deleted the
eng/anders/137316110-nested-init-playground-results
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When a decl that has
initordeinitis nested inside another type, thoseinitordeinitimplementations aren't playground-transformed.Details
The reason is that the playground transform uses an
ASTWalkerto get to the top-level structure, but then once it’s inside a type, it does directly nestedtransformDecl()calls. And that inner check was for too narrow of a type.The problem in this case was that the
dyn_cast<FuncDecl>(D)was too narrow and didn’t include constructors/destructors. We wantdyn_cast<AbstractFunctionDecl>(D).Future work
It's unfortunate that the playground transform reaches decls in two different ways (using an
ASTWalkerto get to the top-level decls but then using directly nested calls below). This seems to be worth resolving at some point (perhaps by using theASTWalkerfor the whole traversal?), but that’s a larger change, and so this is worth addressing with a safer short-term fix.Also, there are still missing results with accessors that are associated with properties in nested types. The fix for that will follow in a separate PR with its own unit test. Edit: The follow-on fix is here: #77530
Changes
dyn_cast<FuncDecl>to adyn_cast<AbstractFunctionDecl>in PlaygroundTransform.cppinitanddeinit(this test also tests the unnested case)rdar://137316110