Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix solution application for keypath function subtype conversions #71737

Merged
merged 1 commit into from
Feb 21, 2024

Conversation

Jumhyn
Copy link
Collaborator

@Jumhyn Jumhyn commented Feb 19, 2024

In #39612 we added subtyping for keypath-function conversions. However the implementation naively coerced the keypath expression itself to the inferred supertype, resulting in erroneous results. This patch updates the solution application logic to build the keypath-function conversion expression based entirely on the 'natural' keypath type, only converting to the inferred supertype at the end via the usual coerceToType machinery for function conversions.

Resolves #71423

In apple#39612 we added subtyping for keypaths-as-functions, but during application
the implementation naively coerced the keypath expression itself to the
inferred supertype, resulting in erroneous results. This patch updates the
solution application logic to build the keypath-function conversion expression
based entirely on the 'natural' keypath type, only converting to the inferred
supertype at the end via the usual coerceToType machinery for function
conversions.
@Jumhyn
Copy link
Collaborator Author

Jumhyn commented Feb 19, 2024

@swift-ci please test

@Jumhyn
Copy link
Collaborator Author

Jumhyn commented Feb 19, 2024

@swift-ci please test source compatibility

@Jumhyn
Copy link
Collaborator Author

Jumhyn commented Feb 19, 2024

@xedin would love your eyes here when you have a chance!

Copy link
Member

@xedin xedin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me!

struct Test {
var codingStack: [CodingStackEntry]
var codingPath: [any CodingKey] { codingStack.map(\.key) }
// CHECK: keypath $KeyPath<CodingStackEntry, URICoderCodingKey>, (root $CodingStackEntry; stored_property #CodingStackEntry.key : $URICoderCodingKey)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a test-case with optional promotion as well for completeness.

@xedin
Copy link
Member

xedin commented Feb 20, 2024

@swift-ci please build toolchain macOS platform

@xedin
Copy link
Member

xedin commented Feb 20, 2024

@swift-ci please test macOS platform

@xedin
Copy link
Member

xedin commented Feb 20, 2024

@swift-ci please test Linux platform

@xedin
Copy link
Member

xedin commented Feb 20, 2024

All of the source compatibility failures are macro related, including penny-bot

@Jumhyn
Copy link
Collaborator Author

Jumhyn commented Feb 20, 2024

@xedin as in, there's an unrelated macro issue here? Or you suspect this patch is somehow interfering with macro logic?

@xedin
Copy link
Member

xedin commented Feb 20, 2024

Macro issue is unrelated, I saw this happen in other PRs.

@xedin
Copy link
Member

xedin commented Feb 20, 2024

@swift-ci please smoke test macOS platform

@xedin xedin merged commit 26a4077 into apple:main Feb 21, 2024
5 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Source compatibility suite] penny-bot failing to build with main branch
2 participants