Skip to content

[5.3] Patch A Regression in Lookup for CodingKeys #33586

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 24, 2020
Merged

[5.3] Patch A Regression in Lookup for CodingKeys #33586

merged 1 commit into from
Aug 24, 2020

Conversation

CodaFi
Copy link
Contributor

@CodaFi CodaFi commented Aug 21, 2020 •
edited
Loading

Cherry pick of #33582

  • Explanation: As part of the name lookup requestification, the code that synthesized CodingKeys was moved to the typechecker’s semantic lookup entrypoints. This migration covered the qualified lookup cases, but did not cover unqualified lookup. This broke clients that attempted to reference CodingKeys without qualification in files that were not the primary file where Codable synthesis usually takes place. This patch restore the behavior of the previous compiler by allowing unqualified lookup for CodingKeys to succeed once again.
  • Scope: SR-13137 was reported by the maintainer of a very popular open source SQLite library - GRDB. That maintainer reports that a common mode of use of this library has been impacted by this regression. This library enjoys a large following, so the scope of the impact of this regression has the potential to be rather large.
  • Risk: Very Low. The fix to restore the old compiler’s behavior is extremely narrow.
  • Issue: rdar://problem/65088901
  • Testing: Added regression tests from SR
  • Reviewer: Doug Gregor

@CodaFi
Patch A Regression in Lookup for CodingKeys
53a38f9 
Codable's magic previously relied on the subject of every qualified lookup in an
unqualified lookup stack to force the synthesis of this member. This
allowed users to reference CodingKeys transitively through a non-primary input
without qualification. As part of the requestification of name lookup,
this synthesis was moved out of the normal qualified lookup path and into the
Type Checker's semantic lookup entrypoints in order to prevent wild
cycles caused by protocol conformance resolution. In the process, we
forget to restore the synthesis check at this entrypoint.

To patch up the source break this caused, we need to walk the context
stack again and force synthesis. Unfortunately, we're stuck with a hack like
this until we bring Codable's implementation back out of the realm of magic
once more. A future implementation of synthesizeSemanticMembersIfNeeded
should aim to just craft the AST for CodingKeys, but not actually run
any of the semantic checks until we check the conformance to CodingKey.

rdar://65088901, SR-13137
@CodaFi CodaFi added the r5.3 label Aug 21, 2020
@CodaFi CodaFi requested a review from DougGregor August 21, 2020 18:04
@CodaFi CodaFi requested a review from a team as a code owner August 21, 2020 18:04
@CodaFi
Copy link
Contributor Author

CodaFi commented Aug 21, 2020

@swift-ci test

@CodaFi
Copy link
Contributor Author

CodaFi commented Aug 21, 2020

@swift-ci test source compatibility

@CodaFi
Copy link
Contributor Author

CodaFi commented Aug 21, 2020

Source compat is complaining about unrelated ARM failures.

@CodaFi
Copy link
Contributor Author

CodaFi commented Aug 24, 2020

⛵

@CodaFi CodaFi merged commit 9f28f80 into swiftlang:release/5.3 Aug 24, 2020
@CodaFi CodaFi deleted the the-coding-keys-to-the-castle branch August 24, 2020 23:35
@groue groue mentioned this pull request Aug 28, 2020
Closed
@AnthonyLatsis AnthonyLatsis added swift 5.3 🍒 release cherry pick Flag: Release branch cherry picks labels Jan 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DougGregor DougGregor DougGregor approved these changes

Assignees
No one assigned
Labels
🍒 release cherry pick Flag: Release branch cherry picks swift 5.3
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@CodaFi @DougGregor @AnthonyLatsis