Fix a few more callers of ProtocolConformanceRef::forAbstract() to pass in a subject type #80107
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
If any of the get*() methods are called on the wrong kind of ProtocolConformanceRef, we immediately cast a pointer to an incorrect type, which will most likely cause a crash.
The "abstract conformance is just a ProtocolDecl" assumption is pretty fundamental here, so we have to fudge a bit at the API boundary for now. Eventually, LocalTypeDataKind should just contain a ProtocolConformanceRef instead of duplicating the representation, however this would require fixing various calls to LocalTypeDataKind::forAbstractProtocolWitnessTable() which pass in a ProtocolDecl to pass in a ProtocolConformanceRef instead.
…eWitness The Protocol field isn't really necessary, because the conformance stores the protocol. But we do need the substituted subject type of the requirement, just temporarily, until an abstract conformance stores its own subject type too.
slavapestov
requested review from
eeckstein,
jckarter,
xymus,
AnthonyLatsis,
hborla,
xedin and
rjmccall
as code owners
|
@swift-ci Please smoke test |
|
@swift-ci Please test source compatibility |
eeckstein
reviewed
Mar 19, 2025
| @@ -37,6 +37,9 @@ public struct Conformance: CustomStringConvertible, NoReflectionChildren { | |||
| return Type(bridged: bridged.getType()) | |||
| } | |||
|
|
|||
| public var proto: ProtocolDecl { | |||
There was a problem hiding this comment.
Can you name this var `protocol`: ProtocolDecl. We try to avoid abbreviations in the swift APIs as good as possible.
Note that the backticks are not needed when using this property, e.g. p = c.protocol
drodriguez
reviewed
Mar 19, 2025
| SILWT->getConformance()->getAssociatedConformance(requirement.getAssociation(), | ||
| requirement.getAssociatedRequirement()); | ||
| llvm::Constant *witnessEntry = IGM.getAddrOfWitnessTable(assocConf.getConcrete()); | ||
| ProtocolConformance *assocConf = associatedWitness.Witness.getConcrete(); |
There was a problem hiding this comment.
associatedWithness comes from a variable which is part of that #ifndef NDEBUG above. Makes this a compilation failure when compiling a non-asserts toolchain.
Is there a fix already happening for this?
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.
I want to make abstract conformances store a subject type, eventually.
This PR is another small step in that direction, by making us pass down a subject type instead of Type() in most places.
I wanted to avoid this becoming too much of a yak shave, so a couple of bits are a little hacky. IRGen's LocalTypeDataKind duplicates ProtocolConformanceRef in an awkward way, and SILWitnessTable::AssociatedConformanceWitness needs a temporary hack. We can unwind these later once the core representation is fixed.
One more call to forAbstract() without a subject type remains. Once that's fixed, we can start storing the subject type that is passed in to forAbstract(). This will allow simplifying some APIs, which will make them more convenient to call from Swift -- something that @eeckstein ran into, which is why I decided to pick up this refactoring again.