Skip to content

Commit

Permalink
Don't drop member mixins from multiple mixins
Browse files Browse the repository at this point in the history
This fixes a bug where a member that sources traits from multiple
mixins would have all but the last of those mixins dropped from its
list, *if* it also happened to apply new traits locally.
  • Loading branch information
JordonPhillips committed Feb 22, 2024
1 parent 698745a commit d0d4e07
Show file tree
Hide file tree
Showing 5 changed files with 72 additions and 17 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -99,22 +99,25 @@ public void modifyShape(
}
}
}
if (introducedMember == null && memberBuilders.containsKey(member.getMemberName())) {
MemberShape.Builder original = memberBuilders.get(member.getMemberName());
introducedMember = original.addMixin(member).build();
if (!introducedMember.getTarget().equals(member.getTarget())) {
mixinMemberConflict(original, member);

if (introducedMember == null) {
if (memberBuilders.containsKey(member.getMemberName())) {
MemberShape.Builder original = memberBuilders.get(member.getMemberName());
introducedMember = original.addMixin(member).build();
if (!introducedMember.getTarget().equals(member.getTarget())) {
mixinMemberConflict(original, member);
}
} else if (!introducedTraits.isEmpty()) {
// Build local member copies before adding mixins if traits
// were introduced to inherited mixin members.
introducedMember = MemberShape.builder()
.id(targetId)
.target(member.getTarget())
.source(member.getSourceLocation())
.addTraits(introducedTraits.values())
.addMixin(member)
.build();
}
} else if (!introducedTraits.isEmpty()) {
// Build local member copies before adding mixins if traits
// were introduced to inherited mixin members.
introducedMember = MemberShape.builder()
.id(targetId)
.target(member.getTarget())
.source(member.getSourceLocation())
.addTraits(introducedTraits.values())
.addMixin(member)
.build();
}

if (introducedMember != null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,11 @@ static Map<String, MemberShape> computeMixinMembers(
String name = member.getMemberName();
if (members.get().containsKey(name)) {
MemberShape localMember = members.get().get(name);
// Rebuild the member with the proper inherited mixin if needed.
// Rebuild the member with the proper inherited mixins if needed.
// This catches errant cases where a member is added to a structure/union
// but omits the mixin members of parent shapes. Arguably, that's way too
// nuanced and error-prone to _not_ try to smooth over.
if (localMember.getMixins().isEmpty() || !mixins.containsKey(member.getId())) {
if (localMember.getMixins().isEmpty() || !localMember.getMixins().contains(member.getId())) {
localMember = localMember.toBuilder().clearMixins().addMixin(member).build();
}
computedMembers.put(name, localMember);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -491,4 +491,11 @@ public void throwsWhenTooNested() {
assertThat(e.getMessage(),
startsWith("Syntax error at line 1, column 1: Parser exceeded maximum allowed depth of 64"));
}

@Test
public void handlesMultipleMemberInheritance() {
ValidatedResult<Model> result = Model.assembler()
.addImport(getClass().getResource("error-recovery/to-dollar.smithy"))
.assemble();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.contains;
import static org.hamcrest.Matchers.containsInAnyOrder;
import static org.hamcrest.Matchers.containsInRelativeOrder;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.empty;
import static org.hamcrest.Matchers.equalTo;
Expand All @@ -44,6 +45,7 @@
import java.util.Collections;
import java.util.Comparator;
import java.util.HashMap;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Optional;
Expand Down Expand Up @@ -77,6 +79,8 @@
import software.amazon.smithy.model.traits.InternalTrait;
import software.amazon.smithy.model.traits.MediaTypeTrait;
import software.amazon.smithy.model.traits.MixinTrait;
import software.amazon.smithy.model.traits.PatternTrait;
import software.amazon.smithy.model.traits.RequiredTrait;
import software.amazon.smithy.model.traits.SensitiveTrait;
import software.amazon.smithy.model.traits.SuppressTrait;
import software.amazon.smithy.model.traits.TagsTrait;
Expand Down Expand Up @@ -1368,4 +1372,23 @@ public void resolvesDuplicateTraitApplicationsToSameMixedInMember() throws Excep
.assemble()
.unwrap();
}

@Test
public void handlesMultipleInheritanceForMixinMembers() {
Model model = Model.assembler()
.addImport(getClass().getResource("mixins/multiple-inheritance-with-introduction.smithy"))
.assemble()
.unwrap();

MemberShape shape = model.expectShape(ShapeId.from("com.example#FinalStructure$member"), MemberShape.class);

assertThat(shape.getMixins(), contains(
ShapeId.from("com.example#MixinA$member"),
ShapeId.from("com.example#MixinB$member")
));
assertThat(shape.getAllTraits().keySet(),
containsInAnyOrder(PatternTrait.ID, RequiredTrait.ID, InternalTrait.ID));
String actualPattern = shape.expectTrait(PatternTrait.class).getValue();
assertThat(actualPattern, equalTo("baz"));
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
$version: "2"

namespace com.example

@mixin
structure MixinA {
@pattern("foo")
@required
member: String
}

@mixin
structure MixinB {
@pattern("bar")
@internal
member: String
}

structure FinalStructure with [MixinA, MixinB] {
@pattern("baz")
member: String
}

0 comments on commit d0d4e07

Please sign in to comment.