-
Notifications
You must be signed in to change notification settings - Fork 47
[refactor]: improve diagnostics for escaped context instances #380
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
Conversation
dependencies: ["ReactiveSwift", "Workflow"], | ||
dependencies: [ | ||
"Workflow", | ||
.product(name: "ReactiveSwift", package: "ReactiveSwift"), |
There was a problem hiding this comment.
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()
ad9dec1
to
649c1ea
Compare
There was a problem hiding this 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.
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 |
649c1ea
to
6ef3a8d
Compare
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 { |
There was a problem hiding this comment.
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.
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
Footnotes
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. ↩