[SILOptimizer] Fixes the ValueDefUseWalker to handle potential unchecked_ref_cast between optional and non-optional class types. #79872
Conversation
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
WeZZard
requested review from
a team,
hborla,
slavapestov,
xedin,
eeckstein,
rjmccall,
ahoppen,
bnbarham,
rintaro,
hamishknight,
shahmishal,
mikeash,
al45tair,
compnerd,
ktoso,
AnthonyLatsis,
xymus,
artemcm,
kavon,
jckarter,
CodaFi,
DougGregor,
zoecarver,
hyp,
egorzhdan,
Xazax-hun,
j-hui,
fahadnayyar,
tshortli and
adrian-prantl
as code owners
WeZZard
requested review from
ian-twilightcoder,
etcwilde,
edymtt and
justice-adams-apple
as code owners
|
Relative issue: #79871 |
WeZZard
force-pushed
the
bugfix/unchecked-ref-casting-with-optional-any-object
branch
from
2c5219f to
21757d0
Compare
|
@swift-ci please test |
…ked_ref_cast between optional and non-optional class types. The `unchecked_ref_cast` is designed to be able to cast between `Optional<ClassType>` and `ClassType`. We need to handle these cases by checking if the type is optional and adjust the path accordingly.
WeZZard
force-pushed
the
bugfix/unchecked-ref-casting-with-optional-any-object
branch
from
21757d0 to
35f82ba
Compare
|
Hi, folks. I've just learned that the practice of submitting pull requests in the Swift repository is to merge to the main branch instead of a specific releasing branch. I've updated the targeting branch of the PR and fixed a build issue introduced in the tidying up of the commit history. Could anyone help to trigger the @swift-ci to test my commit? |
|
@swift-ci please test |
eeckstein
requested changes
Mar 10, 2025
…Optional casting of unchecked_ref_cast in ValueDefUseWalker.
|
@swift-ci test |
|
Hi folks! The tests have completed. I think it's time to complete the code review. Do you have any further advice for this pull request? Plus, since the reviewers list is incredibly long, I wanna make sure that if this pull request really need to involve so much reviewers? Or we just need to involve those reviewers with the shield icon? Reviewers without the shield icon might be mis-involved because I was originally base with release/6.1 but targeted to the main branch. Now the base and the target branch are both fixed to the |
It's fine if a single reviewer (who understands the code) gives her/his thumbs up. |
beccadax
added a commit
to beccadax/swift
that referenced
this pull request
Apr 5, 2025
When a C enum has multiple constants with the same value, ClangImporter selects one of them to import as an `EnumElementDecl` and imports the others as `VarDecl` “aliases” of that one. This helps preserve the invariant that each case of an enum has a unique raw value and is distinct for exhaustiveness checking. However, a bug in that logic could sometimes cause the canonical case to be imported *twice*—once as an `EnumElementDecl` and again as a `VarDecl` alias. In this situation, the `EnumElementDecl` was not added to the enum’s member list, resulting in a malformed AST. This bug has apparently been present since early 2017 (!), but it seems to have been harmless until recently, when the `ComputeSideEffects` SIL pass recently became sensitive to it (probably because of either swiftlang#79872 or swiftlang#80263). Correct this problem by modifying the memoization logic to retrieve the canonical case’s clang-side constant so the subsequent check will succeed. Additionally change a variable name and add comments to help clarify this code for future maintainers. In lieu of adding new unit tests, this commit adds a (slightly expensive) conditional assertion to catch this kind of AST malformation. There are actually about twenty tests that will fail with just the assertion and not the fix, but I’ve updated one of them to enable the assertion even in release mode. Fixes rdar://148213237. Followup to swiftlang#80487, which added related assertions to the SIL layer.
beccadax
added a commit
to beccadax/swift
that referenced
this pull request
Apr 11, 2025
When a C enum has multiple constants with the same value, ClangImporter selects one of them to import as an `EnumElementDecl` and imports the others as `VarDecl` “aliases” of that one. This helps preserve the invariant that each case of an enum has a unique raw value and is distinct for exhaustiveness checking. However, a bug in that logic could sometimes cause the canonical case to be imported *twice*—once as an `EnumElementDecl` and again as a `VarDecl` alias. In this situation, the `EnumElementDecl` was not added to the enum’s member list, resulting in a malformed AST. This bug has apparently been present since early 2017 (!), but it seems to have been harmless until recently, when the `ComputeSideEffects` SIL pass recently became sensitive to it (probably because of either swiftlang#79872 or swiftlang#80263). Correct this problem by modifying the memoization logic to retrieve the canonical case’s clang-side constant so the subsequent check will succeed. Additionally change a variable name and add comments to help clarify this code for future maintainers. In lieu of adding new unit tests, this commit adds a (slightly expensive) conditional assertion to catch this kind of AST malformation. There are actually about twenty tests that will fail with just the assertion and not the fix, but I’ve updated one of them to enable the assertion even in release mode. Fixes rdar://148213237. Followup to swiftlang#80487, which added related assertions to the SIL layer.
beccadax
added a commit
to beccadax/swift
that referenced
this pull request
Apr 15, 2025
When a C enum has multiple constants with the same value, ClangImporter selects one of them to import as an `EnumElementDecl` and imports the others as `VarDecl` “aliases” of that one. This helps preserve the invariant that each case of an enum has a unique raw value and is distinct for exhaustiveness checking. However, a bug in that logic could sometimes cause the canonical case to be imported *twice*—once as an `EnumElementDecl` and again as a `VarDecl` alias. In this situation, the `EnumElementDecl` was not added to the enum’s member list, resulting in a malformed AST. This bug has apparently been present since early 2017 (!), but it seems to have been harmless until recently, when the `ComputeSideEffects` SIL pass recently became sensitive to it (probably because of either swiftlang#79872 or swiftlang#80263). Correct this problem by modifying the memoization logic to retrieve the canonical case’s clang-side constant so the subsequent check will succeed. Additionally change a variable name and add comments to help clarify this code for future maintainers. In lieu of adding new unit tests, this commit adds a (slightly expensive) conditional assertion to catch this kind of AST malformation. There are actually about twenty tests that will fail with just the assertion and not the fix, but I’ve updated one of them to enable the assertion even in release mode. Fixes rdar://148213237. Followup to swiftlang#80487, which added related assertions to the SIL layer.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The
unchecked_ref_castis designed to be able to cast betweenOptional<ClassType>andClassType. We need to handle these cases by checking if the type is optional and adjust the path accordingly.The absence of this process causes the escape analysis for the operand of the
loadinstruction returns non-escaping if there is aunchecked_ref_castcasting from an optional class type to a non-optional class type. Since this the implicit optional-wrapping does not reflect on thepath.This further makes the redundant load elimination pass to eliminate this
loadthough there is a function with side-effects applied right before theloadinstruction. Since the function apply effect computation in the alias analysis only returns the effects of the function itself if the operand of theloadinstruction is not escaped.