Fix capturing uses of NonCapturingLazyInitializer #23881
Conversation
roji
commented
Jan 14, 2021
roji
commented
- Fixed several cases where our initialization was capturing (so spurious memory allocations). Added C# 9 static everywhere to reduce the risk of this happening again.
- Fixed one case where a nullable value was being lazily generated, defeating the initialization mechanism (because NonCapturingLazyInitializer compares to null to see if a value has been generated). This meant every invocation would re-generate again and again. Discovered during NRT annotations - doing this won't compile any more after that's done.
roji
force-pushed
the
EvilInitializers
branch
2 times, most recently
from
ab42198
to
9f3fb63
Compare
| @@ -93,7 +93,7 @@ public CustomShaperCompilingExpressionVisitor(bool tracking) | |||
| { | |||
| if (entity is TIncludingEntity includingEntity) | |||
| { | |||
| var collectionAccessor = navigation.GetCollectionAccessor(); | |||
| var collectionAccessor = navigation.GetCollectionAccessor()!; | |||
There was a problem hiding this comment.
@AndriySvyryd @smitpatel note the bang here. For navigations and skip navigations, ClrCollectionAccessorFactory().Create() returns null when the property isn't a collection or is a shadow property. I guess this would be about implementing shadow navigations?
There was a problem hiding this comment.
We could also throw for these cases as they're not supported etc.
roji
force-pushed
the
EvilInitializers
branch
from
03ad246
to
3d83dd8
Compare
|
Hello @roji! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
|
Apologies, while this PR appears ready to be merged, I've been configured to only merge when all checks have explicitly passed. The following integrations have not reported any progress on their checks and are blocking auto-merge:
These integrations are possibly never going to report a check, and unblocking auto-merge likely requires a human being to update my configuration to exempt these integrations from requiring a passing check. Give feedback on thisFrom the bot dev teamWe've tried to tune the bot such that it posts a comment like this only when auto-merge is blocked for exceptional, non-intuitive reasons. When the bot's auto-merge capability is properly configured, auto-merge should operate as you would intuitively expect and you should not see any spurious comments. Please reach out to us at fabricbotservices@microsoft.com to provide feedback if you believe you're seeing this comment appear spuriously. Please note that we usually are unable to update your bot configuration on your team's behalf, but we're happy to help you identify your bot admin. |
ghost
merged commit 
