GSB: getConformanceAccessPath() doesn't need to use getMinimalConformanceSource() #36402
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
…anceSource() Previously we would look for a derived source before an explicit one, on account of the explicit one possibly being redundant. However, the presence of 'self-derived' sources meant that we had to call getMinimalConformanceSource() to ensure the derived sources were actually usable and would not produce an infinite conformance access path. I'd like to remove getMinimalConformanceSource() now that we have an alternate algorithm to identify redundant explicit requirements. Instead, we can handle the explicit case first, by checking for a conformance requirement in the generic signature -- its presence means it was not redundant, by construction. Then once we handle that case, we know we're going to use a derived source, and finding the shortest one seems to be good enough. This fixes the IRGen crash in https://bugs.swift.org/browse/SR-11153; the requirement signatures in that test still have unnecessary same-type requirements printed, so I added a separate RUN: line for those, and it's marked as known-failing with 'not %FileCheck'.
This test case was also posted by a developer in https://bugs.swift.org/browse/SR-11153, but it appears to be a different problem that was fixed in Swift 5.2.
Contributor
Author
|
@swift-ci Please test |
Contributor
Author
|
@swift-ci Please test source compatibility |
Contributor
|
Build failed |
Contributor
Author
|
@swift-ci Please test macOS |
Contributor
Author
|
@swift-ci Please smoke test macOS |
Contributor
|
Build failed |
Contributor
|
Hi @slavapestov, this appears to have caused a regression on my Fedora 33 (x86-64) box. Any theories? |
Contributor
|
Whoops, the regression was actually the other pull request you merged this morning: #36411 |
slavapestov
added a commit
to slavapestov/swift
that referenced
this pull request
Mar 12, 2021
I added this test case in swiftlang#36402; the change in swiftlang#36411 correctly flags some bogus same-type requirements in a minimized signature and asserts.
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.
Previously we would look for a derived source before an explicit one,
on account of the explicit one possibly being redundant. However, the
presence of 'self-derived' sources meant that we had to call
getMinimalConformanceSource() to ensure the derived sources
were actually usable and would not produce an infinite conformance
access path.
I'd like to remove getMinimalConformanceSource() now that we have
an alternate algorithm to identify redundant explicit requirements.
Instead, we can handle the explicit case first, by checking for a
conformance requirement in the generic signature -- its presence
means it was not redundant, by construction.
Then once we handle that case, we know we're going to use a derived
source, and finding the shortest one seems to be good enough.
This fixes the IRGen crash in https://bugs.swift.org/browse/SR-11153;
the requirement signatures in that test still have unnecessary
same-type requirements printed, so I added a separate RUN: line
for those, and it's marked as known-failing with 'not %FileCheck'.