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.

Sign up for GitHub

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

Fix duplication of parameter name assignment #1589

Merged
merged 2 commits into from
Jun 24, 2021
Merged

Fix duplication of parameter name assignment #1589

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0d08954
Select parameter name exactly once
theunrepentantgeek Jun 22, 2021
3a9e234
Merge branch 'master' into fix/duplicate-parameter-names
matthchr Jun 24, 2021
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
36 changes: 15 additions & 21 deletions hack/generator/pkg/conversions/property_assignment_function.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@ type PropertyAssignmentFunction struct {
knownLocals *astmodel.KnownLocalsSet
// conversionContext is additional information about the context in which this conversion was made
conversionContext *PropertyConversionContext
// identifier to use for our receiver in generated code
receiverName string
// identifier to use for our parameter in generated code
parameterName string
}

// StoragePropertyConversion represents a function that generates the correct AST to convert a single property value
Expand All @@ -58,11 +62,10 @@ func NewPropertyAssignmentFromFunction(
direction: ConvertFrom,
conversions: make(map[string]StoragePropertyConversion),
knownLocals: astmodel.NewKnownLocalsSet(idFactory),
receiverName: idFactory.CreateIdentifier(receiver.Name().Name(), astmodel.NotExported),
parameterName: "source",
}

// TODO: Bevan will improve how this is done (avoid hard-coding "source")
result.knownLocals.Add("source")

result.conversionContext = conversionContext.WithFunctionName(result.Name()).
WithKnownLocals(result.knownLocals).
WithDirection(ConvertFrom)
Expand All @@ -88,11 +91,10 @@ func NewPropertyAssignmentToFunction(
direction: ConvertTo,
conversions: make(map[string]StoragePropertyConversion),
knownLocals: astmodel.NewKnownLocalsSet(idFactory),
receiverName: idFactory.CreateIdentifier(receiver.Name().Name(), astmodel.NotExported),
parameterName: "destination",
}

// TODO: Bevan will improve how this is done (avoid hard-coding "destination")
result.knownLocals.Add("destination")

result.conversionContext = conversionContext.WithFunctionName(result.Name()).
WithKnownLocals(result.knownLocals).
WithDirection(ConvertTo)
Expand Down Expand Up @@ -153,41 +155,32 @@ func (fn *PropertyAssignmentFunction) Equals(f astmodel.Function) bool {
// AsFunc renders this function as an AST for serialization to a Go source file
func (fn *PropertyAssignmentFunction) AsFunc(generationContext *astmodel.CodeGenerationContext, receiver astmodel.TypeName) *dst.FuncDecl {

var parameterName string
var description string
switch fn.direction {
case ConvertFrom:
parameterName = "source"
description = fmt.Sprintf("populates our %s from the provided source %s", receiver.Name(), fn.otherDefinition.Name().Name())
case ConvertTo:
parameterName = "destination"
description = fmt.Sprintf("populates the provided destination %s from our %s", fn.otherDefinition.Name().Name(), receiver.Name())
default:
panic(fmt.Sprintf("unexpected conversion direction %q", fn.direction))
}

// Create a sensible name for our receiver
receiverName := fn.idFactory.CreateIdentifier(receiver.Name(), astmodel.NotExported)

// We always use a pointer receiver so we can modify it
receiverType := astmodel.NewOptionalType(receiver).AsType(generationContext)

funcDetails := &astbuilder.FuncDetails{
ReceiverIdent: receiverName,
ReceiverIdent: fn.receiverName,
ReceiverType: receiverType,
Name: fn.Name(),
Body: fn.generateBody(receiverName, parameterName, generationContext),
Body: fn.generateBody(fn.receiverName, fn.parameterName, generationContext),
}

parameterPackage := generationContext.MustGetImportedPackageName(fn.otherDefinition.Name().PackageReference)

funcDetails.AddParameter(
parameterName,
fn.parameterName,
&dst.StarExpr{
X: &dst.SelectorExpr{
X: dst.NewIdent(parameterPackage),
Sel: dst.NewIdent(fn.otherDefinition.Name().Name()),
},
X: astbuilder.Selector(dst.NewIdent(parameterPackage), fn.otherDefinition.Name().Name()),
})

funcDetails.AddReturns("error")
Expand Down Expand Up @@ -291,8 +284,9 @@ func (fn *PropertyAssignmentFunction) createConversions(receiver astmodel.TypeDe
panic(fmt.Sprintf("unexpected conversion direction %q", fn.direction))
}

// Flag receiver name as used
fn.knownLocals.Add(receiver.Name().Name())
// Flag receiver and parameter names as used
fn.knownLocals.Add(fn.receiverName)
fn.knownLocals.Add(fn.parameterName)

for destinationName, destinationEndpoint := range destinationEndpoints {
sourceEndpoint, ok := sourceEndpoints[destinationName]
Expand Down