Skip to content
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

Put short-circuiting code in one place #1635

Merged
merged 3 commits into from
Jul 14, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions hack/generator/pkg/astmodel/array_type.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,19 @@ func (array *ArrayType) Element() Type {
return array.element
}

// WithElement returns an ArrayType with the specified element.
// the benefit of this is it allows reusing the same value if the
// element type is the same
func (array *ArrayType) WithElement(t Type) *ArrayType {
if array.element.Equals(t) {
return array
}

result := *array
result.element = t
return &result
}

// assert we implemented Type correctly
var _ Type = (*ArrayType)(nil)

Expand Down
4 changes: 4 additions & 0 deletions hack/generator/pkg/astmodel/flagged_type.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,10 @@ func (ft *FlaggedType) WithoutFlag(flag TypeFlag) Type {

// WithElement returns the flagged type with the same flags but a different element
func (ft *FlaggedType) WithElement(t Type) *FlaggedType {
if ft.element.Equals(t) {
return ft // short-circuit
}

var flags []TypeFlag
for f := range ft.flags {
flags = append(flags, f)
Expand Down
20 changes: 20 additions & 0 deletions hack/generator/pkg/astmodel/map_type.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,26 @@ func (m *MapType) ValueType() Type {
return m.value
}

func (m *MapType) WithKeyType(t Type) *MapType {
if m.key.Equals(t) {
return m
}

result := *m
result.key = t
return &result
}

func (m *MapType) WithValueType(t Type) *MapType {
if m.value.Equals(t) {
return m
}

result := *m
result.value = t
return &result
}

// NewMapType creates a new map with the specified key and value types
func NewMapType(key Type, value Type) *MapType {
return &MapType{key, value}
Expand Down
38 changes: 36 additions & 2 deletions hack/generator/pkg/astmodel/optional_type.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,32 @@ type OptionalType struct {
element Type
}

var _ Type = &OptionalType{}
// type assertions
var (
_ Type = &OptionalType{}
_ MetaType = &OptionalType{}
)

var _ MetaType = &OptionalType{}
// cache of commonly-used values
var cachedOptionals = map[Type]*OptionalType{
BoolType: {BoolType},
FloatType: {FloatType},
IntType: {IntType},
StringType: {StringType},
UInt32Type: {UInt32Type},
UInt64Type: {UInt64Type},
}
theunrepentantgeek marked this conversation as resolved.
Show resolved Hide resolved

// NewOptionalType creates a new optional type that may or may not have the specified 'element' type
func NewOptionalType(element Type) Type {
if isTypeOptional(element) {
return element
}

if result, ok := cachedOptionals[element]; ok {
return result
}

return &OptionalType{element}
}

Expand All @@ -52,6 +68,24 @@ func (optional *OptionalType) Element() Type {
return optional.element
}

func (optional *OptionalType) WithElement(t Type) Type {
if optional.element.Equals(t) {
return optional
}

if isTypeOptional(t) {
return t
}

if cached, ok := cachedOptionals[t]; ok {
return cached
}

result := *optional
result.element = t
return &result
}

func (optional *OptionalType) AsDeclarations(codeGenerationContext *CodeGenerationContext, declContext DeclarationContext) []dst.Decl {
return AsSimpleDeclarations(codeGenerationContext, declContext, optional)
}
Expand Down
8 changes: 8 additions & 0 deletions hack/generator/pkg/astmodel/resource_type.go
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,10 @@ func (resource *ResourceType) IsStorageVersion() bool {

// WithSpec returns a new resource that has the specified spec type
func (resource *ResourceType) WithSpec(specType Type) *ResourceType {
if TypeEquals(resource.spec, specType) {
return resource // short-circuit
}

if specResource, ok := specType.(*ResourceType); ok {
// type is a resource, take its SpecType instead
// so we don't nest resources
Expand All @@ -166,6 +170,10 @@ func (resource *ResourceType) WithSpec(specType Type) *ResourceType {

// WithStatus returns a new resource that has the specified status type
func (resource *ResourceType) WithStatus(statusType Type) *ResourceType {
if TypeEquals(resource.status, statusType) {
return resource // short-circuit
}

if specResource, ok := statusType.(*ResourceType); ok {
// type is a resource, take its StatusType instead
// so we don't nest resources
Expand Down
20 changes: 3 additions & 17 deletions hack/generator/pkg/astmodel/type_visitor.go
Original file line number Diff line number Diff line change
Expand Up @@ -121,11 +121,7 @@ func IdentityVisitOfArrayType(this *TypeVisitor, it *ArrayType, ctx interface{})
return nil, errors.Wrap(err, "failed to visit type of array")
}

if newElement == it.element {
return it, nil // short-circuit
}

return NewArrayType(newElement), nil
return it.WithElement(newElement), nil
}

func IdentityVisitOfPrimitiveType(_ *TypeVisitor, it *PrimitiveType, _ interface{}) (Type, error) {
Expand Down Expand Up @@ -186,11 +182,7 @@ func IdentityVisitOfMapType(this *TypeVisitor, it *MapType, ctx interface{}) (Ty
return nil, errors.Wrapf(err, "failed to visit map value type %q", it.value)
}

if visitedKey == it.key && visitedValue == it.value {
return it, nil // short-circuit
}

return NewMapType(visitedKey, visitedValue), nil
return it.WithKeyType(visitedKey).WithValueType(visitedValue), nil
}

func IdentityVisitOfEnumType(_ *TypeVisitor, it *EnumType, _ interface{}) (Type, error) {
Expand All @@ -205,11 +197,7 @@ func IdentityVisitOfOptionalType(this *TypeVisitor, it *OptionalType, ctx interf
return nil, errors.Wrapf(err, "failed to visit optional element type %q", it.element)
}

if visitedElement == it.element {
return it, nil // short-circuit
}

return NewOptionalType(visitedElement), nil
return it.WithElement(visitedElement), nil
}

func IdentityVisitOfResourceType(this *TypeVisitor, it *ResourceType, ctx interface{}) (Type, error) {
Expand Down Expand Up @@ -241,7 +229,6 @@ func IdentityVisitOfOneOfType(this *TypeVisitor, it *OneOfType, ctx interface{})
newTypes = append(newTypes, newType)
return nil
})

if err != nil {
return nil, err
}
Expand All @@ -264,7 +251,6 @@ func IdentityVisitOfAllOfType(this *TypeVisitor, it *AllOfType, ctx interface{})
newTypes = append(newTypes, newType)
return nil
})

if err != nil {
return nil, err
}
Expand Down
10 changes: 10 additions & 0 deletions hack/generator/pkg/astmodel/validated_type.go
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,16 @@ func (v *ValidatedType) ElementType() Type {
return v.element
}

func (v *ValidatedType) WithElement(t Type) Type {
if v.element.Equals(t) {
return v
}

result := *v
result.element = t
return &result
}
theunrepentantgeek marked this conversation as resolved.
Show resolved Hide resolved

func (v *ValidatedType) Validations() Validations {
return v.validations
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,7 @@ func (s synthesizer) handleArrayArray(leftArray *astmodel.ArrayType, rightArray
return nil, err
}

return astmodel.NewArrayType(intersected), nil
return leftArray.WithElement(intersected), nil
}

func (s synthesizer) handleObjectObject(leftObj *astmodel.ObjectType, rightObj *astmodel.ObjectType) (astmodel.Type, error) {
Expand Down
34 changes: 7 additions & 27 deletions hack/generator/pkg/codegen/pipeline/status_augment.go
Original file line number Diff line number Diff line change
Expand Up @@ -160,17 +160,12 @@ func flattenAugmenter(allTypes astmodel.ReadonlyTypes) augmenter {
merger.Add(func(spec *astmodel.OptionalType, status astmodel.Type) (astmodel.Type, error) {
// we ignore optionality when matching things up, since there are
// discordances between the JSON Schema/Swagger as some teams have handcrafted them
newSpecElement, err := merger.Merge(spec.Element(), status)
newElement, err := merger.Merge(spec.Element(), status)
if err != nil {
return nil, err
}

// return original type if not changed
if newSpecElement.Equals(spec.Element()) {
return spec, nil
}

return astmodel.NewOptionalType(newSpecElement), nil
return spec.WithElement(newElement), nil
})

// handle (Type, Optional)
Expand All @@ -183,17 +178,12 @@ func flattenAugmenter(allTypes astmodel.ReadonlyTypes) augmenter {
// handle (FlaggedType, Type); there is no need to handle the opposite direction
// as the swagger types won’t be flagged
merger.Add(func(spec *astmodel.FlaggedType, status astmodel.Type) (astmodel.Type, error) {
newSpec, err := merger.Merge(spec.Element(), status)
newElement, err := merger.Merge(spec.Element(), status)
if err != nil {
return nil, err
}

// return original type if not changed
if newSpec.Equals(spec.Element()) {
return spec, nil
}

return spec.WithElement(newSpec), nil
return spec.WithElement(newElement), nil
})

// handle (Map, Map) by matching up the keys and values
Expand All @@ -208,27 +198,17 @@ func flattenAugmenter(allTypes astmodel.ReadonlyTypes) augmenter {
return nil, err
}

// return original type if not changed
if keyResult.Equals(spec.KeyType()) && valueResult.Equals(spec.ValueType()) {
return spec, nil
}

return astmodel.NewMapType(keyResult, valueResult), nil
return spec.WithKeyType(keyResult).WithValueType(valueResult), nil
})

// handle (Array, Array) by matching up the inner types
merger.Add(func(spec, status *astmodel.ArrayType) (astmodel.Type, error) {
newSpecElement, err := merger.Merge(spec.Element(), status.Element())
newElement, err := merger.Merge(spec.Element(), status.Element())
if err != nil {
return nil, err
}

// return original type if not changed
if newSpecElement.Equals(spec.Element()) {
return spec, nil
}

return astmodel.NewArrayType(newSpecElement), nil
return spec.WithElement(newElement), nil
})

// safety check, (AllOf, AllOf) should not occur
Expand Down