[ClangImporter] Check for builtin conformance in importNumericLiteral
#85409
Conversation
hamishknight
requested review from
Xazax-hun,
beccadax,
egorzhdan,
hborla,
ian-twilightcoder,
j-hui,
slavapestov,
susmonteiro and
xedin
as code owners
hamishknight
force-pushed
the
fishers-landing
branch
4 times, most recently
from
1c8f747 to
17f5c11
Compare
| if (!ctx.getIntBuiltinInitDecl(constantTyNominal)) | ||
| return nullptr; | ||
|
|
||
| return createMacroConstant(Impl, MI, name, DC, constantType, |
There was a problem hiding this comment.
Does it make sense to pass down the result of the lookup to createMacroConstant() instead of re-doing the lookup inside?
There was a problem hiding this comment.
We cache the result on success, so it shouldn't matter much. There are also a bunch of different clients of createConstant, which would make it a bit awkward (although looking at them it's not entirely clear to me that we have the correct checks for them either)
There was a problem hiding this comment.
Interestingly the caching logic in getBuiltinInitDecl only uses the decl as the key, I guess we never ever conform a decl to more than 1 builtin literal kind? 🤔
There was a problem hiding this comment.
Hmm nope, Float conforms to both _ExpressibleByBuiltinIntegerLiteral & _ExpressibleByBuiltinFloatLiteral
There was a problem hiding this comment.
Updated to also include the kind in the key
|
|
||
| auto &ctx = DC->getASTContext(); | ||
| auto *constantTyNominal = constantType->getAnyNominal(); | ||
| if (!constantTyNominal) |
There was a problem hiding this comment.
Should we add an assert at the other call sites too then, to avoid propagating nulls?
There was a problem hiding this comment.
Hmm not sure I understand what you mean by this
Allow querying for types that may not conform. Failures here should be asserted downstream (e.g in `setBuiltinInitializer`).
Make sure we don't return the wrong witness if you query with 2 different builtin protocol kinds.
Make sure the destination type actually conforms to the corresponding builtin literal protocol before attempting to import it. We may want to consider allowing any type that conforms to the non-builtin literal protocol, but I want to keep this patch low risk and just ensure we at least don't crash for now. rdar://156524292
hamishknight
force-pushed
the
fishers-landing
branch
from
17f5c11 to
cdffd55
Compare
|
@swift-ci please test |
|
@swift-ci please test Windows |
2 participants
Make sure the destination type actually conforms to the corresponding builtin literal protocol before attempting to import it. We may want to consider allowing any type that conforms to the non-builtin literal protocol, but I want to keep this patch low risk and just ensure we at least don't crash for now.
rdar://156524292