Fix multi-model, mixin member apply bug #2378
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JordonPhillips
approved these changes
Aug 23, 2024
kstich
force-pushed
the
mixin_apply_double_build_fix
branch
from
c23c172
to
90713dc
Compare
On first load, via sources, there's a `DefineShape` `LoadOperation` with an `ApplyMixin` modifier. There's also a pending `@apply` statement in the `LoaderTraitMap`. When the source is assembled, that's fully resolved, the mixin is merged into the shape, then traits applied to mixed content are applied. This is a valid source model that has the trait applied to the mixed member. The import is then broken out into `LoadOperation`s to be assembled. The shape's `LoadOperations` already exist in the map and a new `DefineShape` load operation is added that also has the `ApplyMixin` modifier. The first, already fully mixed applied and built - the second, unmixed and unapplied. When we go to `applyTraitsToNonMixinsInShapeMap` on the import pass, we detect if any of the load operations that `DefineShape` contain the member where the trait is applied and add it. However, because we have **two** `LoadOperations` - one with the member and one without - we apply the trait to the shape that was already mixed and applied. We do not add it to the second `LoadOperation`, because it does not have the member. But, because we applied it somewhere, the trait is no longer unclaimed. When then later resolving the `ApplyMixin` modifier, the trait is not applied since it has been claimed elsewhere. When building the second shape and comparing it to the first, the trait is then missing, failing out due to a conflict. This is resolved by sending traits for missing members directly in to the `ApplyMixin` modifier and merging them when apply the mixin.
kstich
force-pushed
the
mixin_apply_double_build_fix
branch
from
90713dc
to
b810ef8
Compare
JordonPhillips
approved these changes
Aug 26, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
On first load, via sources, there's a
DefineShapeLoadOperationwith anApplyMixinmodifier. There's also a pending@applystatement in theLoaderTraitMap. When the source is assembled, that's fully resolved, the mixin is merged into the shape, then traits applied to mixed content are applied. This is a valid source model that has the trait applied to the mixed member.The import is then broken out into
LoadOperations to be assembled. The shape'sLoadOperationsalready exist in the map and a newDefineShapeload operation is added that also has theApplyMixinmodifier. The first, already fully mixed applied and built - the second, unmixed and unapplied.When we go to
applyTraitsToNonMixinsInShapeMapon the import pass, we detect if any of the load operations thatDefineShapecontain the member where the trait is applied and add it. However, because we have twoLoadOperations- one with the member and one without - we apply the trait to the shape that was already mixed and applied. We do not add it to the secondLoadOperation, because it does not have the member. But, because we applied it somewhere, the trait is no longer unclaimed. When then later resolving theApplyMixinmodifier, the trait is not applied since it has been claimed elsewhere. When building the second shape and comparing it to the first, the trait is then missing, failing out due to a conflict.This is resolved by sending traits for missing members directly in to the
ApplyMixinmodifier and merging them when we apply the mixin.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.