Skip to content

Conversation

jamieQ
Copy link
Contributor

@jamieQ jamieQ commented Sep 17, 2025

Issue

the {Render|Apply}Context types should not escape from their respective methods, as this can allow library internals to 'leak out' and cause various issues1. primarily this can result in lifetime discrepancies or surprising relationships in the object graph. historically we've enforced this invariant 'lazily' by setting a bit on the instances and checking that they are still 'valid' during access through their public API. however, it recently occurred to me that we could probably be a bit more proactive about this, and perform a unique reference check immediately after returning from the client callouts.

Description

  • adds logic to detect escaping context instances (debug mode only)
  • adds the 'IssueReporting' dependency to Workflow so we can better test the logic in our unit tests
  • minor incidental refactoring of the Package file
  • updates unit tests appropriately

Footnotes

  1. this is really more of an issue for RenderContext, as it doesn't yet zero-out its internal storage when invalidated. since ApplyContext is newer, we didn't make that mistake twice.

@jamieQ jamieQ marked this pull request as ready for review September 17, 2025 17:38
@jamieQ jamieQ requested a review from a team as a code owner September 17, 2025 17:38
dependencies: ["ReactiveSwift", "Workflow"],
dependencies: [
"Workflow",
.product(name: "ReactiveSwift", package: "ReactiveSwift"),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

standardized some of these so now all the 'package-level' dependencies are just strings, and the external ones are .product()

@jamieQ jamieQ force-pushed the jquadri/no-escape branch 2 times, most recently from ad9dec1 to 649c1ea Compare September 17, 2025 21:34
Copy link
Member

@amorde amorde left a comment

Choose a reason for hiding this comment

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

Very cool! Only question is if there's any concerns having the Workflow target now depend on the IssueReporting framework from the xctest overlay. I suppose it should be fine given we only use it in #if DEBUG, but perhaps there's some potential dependency conflicts for downstream consumers using SPM if they use a different version or something like that.

@jamieQ
Copy link
Contributor Author

jamieQ commented Sep 22, 2025

Only question is if there's any concerns having the Workflow target now depend on the IssueReporting framework from the xctest overlay. I suppose it should be fine given we only use it in #if DEBUG, but perhaps there's some potential dependency conflicts for downstream consumers using SPM if they use a different version or something like that.

yeah, i guess i'm unsure exactly what the theoretical risk is here... we have had the dependency in the package for a while, but i guess not as part of the Workflow target, so maybe that could cause problems? i think we're as permissive with the version as possible though, so maybe it's okay.

i guess i should probably also look into if the library gets built & linked in non-debug builds even if it's only referenced behind conditional compilation directives...


edit: i think since we've had the implicit dependency already we're okay and won't cause new issues for people due to dependency resolution (i set the dependency to what seemed like the actual minimum version we require 1.2.2). also seems there's not really a way to do non-debug build & link only, but also i think this is probably okay since it's effectively only used in debug and nothing is publicly re-exported.

when testing locally to see if the tests worked with older dependency
versions, discovered that some interaction with SwiftTesting macros
and older tests caused the ApplyContext escaping test to fail. did
not investigate deeply, but converting to an XCTestCase provides
consistent behavior.
struct ApplyContextTests {
@Test
func concreteApplyContextInvalidatedAfterUse() async throws {
final class ApplyContextTests: XCTestCase {
Copy link
Contributor Author

@jamieQ jamieQ Sep 23, 2025

Choose a reason for hiding this comment

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

converted this to XCTest b/c when testing against the oldest version of the IssueReporting dependency we support (1.2.2), for some reason the test seemed to behave differently and didn't appear to actually 'escape' the context. i assume that means there's a bug somewhere, but did not investigate.

@jamieQ jamieQ merged commit 15e8c02 into main Sep 23, 2025
12 checks passed
@jamieQ jamieQ deleted the jquadri/no-escape branch September 23, 2025 15:36
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.

2 participants