pkg/instancetype/instancetype.go: Avoid named returns

[0xFelix]

Is your feature request related to a problem? Please describe:

Named returns make the code harder to understand than necessary, therefore they should be avoided.

Describe the solution you'd like:

Remove named returns in pkg/instancetype/instancetype.go L814-908 and define the variables in the function bodies instead.

[0xFelix]

/sig code-quality

[alicefr]

/label good-first-issue

[sarthaksarthak9]

/assign

[iholder101]

Hey @0xFelix!

Named returns make the code harder to understand than necessary, therefore they should be avoided.

I'm sorry, but I disagree with this statement.
Like a lot of other language offerings, named parameters sometimes make the code easier to read, and sometimes harder to read. I don't agree with the statement that they're bad all across the board.

Let me give you an example of a situation I think you're right:

func doSomething() (err error) {...}

In this case the err naming is redundant, since it's absolutely obvious what the return value stands for.

However, here's a case when I think your statement is not correct:

func getPersonInformation(p Person) (string, string, bool, int, error) {...}

In this case I have absolutely no idea what the return values actually are or what they represent. I would have to dive into the implementation in order to figure it out. I think that this function is much easier to understand:

func getPersonInformation(p Person) (firstName, LastName string, hasChildren bool, age int, err error) {...}

This is only one case where named return values are valuable, but there are other cases, for example to avoid declaring a var inside the function body.

IMHO this should be reviewed on a case-to-case basis.

Looking at #11452 for example, I'm not sure that this PR actually improves code quality in any way.

[iholder101]

Remove named returns in pkg/instancetype/instancetype.go L814-908 and define the variables in the function bodies instead.

I've just now realized you were targeting something specific. Sorry for not noticing.
So let's look at your example specifically (I'll make it a bit shorter for simplicity so we can focus on return values):

func inferDefaultsFromVolumes(vm *virtv1.VirtualMachine) (defaultName, defaultKind string, err error) {

The PR turns it into:

func inferDefaultsFromVolumes(vm *virtv1.VirtualMachine) (string, string, error) {

This specific case proves my point from the comment above.
Now I can understand that the two strings that are being returned are default name and kind. But without these I would have to look at the implementation. However, the implementation delegates this operation to other functions:

	for _, volume := range vm.Spec.Template.Spec.Volumes {
		if volume.Name != inferFromVolumeName {
			continue
		}
		if volume.PersistentVolumeClaim != nil {
			return m.inferDefaultsFromPVC(volume.PersistentVolumeClaim.ClaimName, vm.Namespace, defaultNameLabel, defaultKindLabel)
		}
		if volume.DataVolume != nil {
			return m.inferDefaultsFromDataVolume(vm, volume.DataVolume.Name, defaultNameLabel, defaultKindLabel)
		}
		return "", "", NewIgnoreableInferenceError(fmt.Errorf("unable to infer defaults from volume %s as type is not supported", inferFromVolumeName))
	}
	return "", "", fmt.Errorf("unable to find volume %s to infer defaults", inferFromVolumeName)

So now I have to dive even deeper into other function's implementation.. all of that just to understand what values are being returned and what they represent.

[alicefr]

So now I have to dive even deeper into other function's implementation.. all of that just to understand what values are being returned and what they represent.

This could be solved with a good comment on what the function returns. Not always the variable names are clear either

[alicefr]

@iholder101 as already mentioned in the PR, I encourage you to write this comment in the coding best-practice issue: #11259

[kubevirt-bot]

@lyarwood: The label(s) /label instancetype cannot be applied. These labels are supported: good-first-issue. Is this label configured under labels -> additional_labels or labels -> restricted_labels in plugin.yaml?

In response to this:

/label instancetype
/cc

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[lyarwood]

/area instancetype

[lyarwood]

As the original author I obviously agree with @iholder101 here that in this specific case there's value in using named return variables when the implementation delegates the returns. That said the shared underlying function used by all paths does actually define these return variables within the function and should have its named return values dropped:

kubevirt/pkg/instancetype/instancetype.go

Lines 830 to 836 in ee87726

	func inferDefaultsFromLabels(labels map[string]string, defaultNameLabel, defaultKindLabel string) (defaultName, defaultKind string, err error) {
	defaultName, hasLabel := labels[defaultNameLabel]
	if !hasLabel {
	return "", "", NewIgnoreableInferenceError(fmt.Errorf("unable to find required %s label on the volume", defaultNameLabel))
	}
	return defaultName, labels[defaultKindLabel], nil
	}

I also agree with @alicefr that empty returns using named return variables should be avoided and should be documented as something to avoid.

[iholder101]

This could be solved with a good comment on what the function returns. Not always the variable names are clear either

@alicefr
I fundamentally and strongly disagree with this statement :)
Comments are one of the worst and dangerous code smells there is. Specifically because comments are not code, they can be outdated and provide false information very easily. While it's obviously true that the same can happen with variable names, since variable names are code, it is much less likely.

Let's look at a simple example again:

// First return value is first name, second is last name
func getPersonInformation(p Person) (string, string) {
	firstName := getFirstName()
	LastName := getLastName()
	// ...
	return firstName, lastName
}

Now, let's say that I'm refactoring the code and actually want to return the first name, and address. This can very easily happen:

// First return value is first name, second is last name
func getPersonInformation(p Person) (string, string) {
	firstName := getFirstName()
	address := getAddress()
	// ...
	return firstName, address
}

Now we have a comment which provide false information, and yet for some reasons (as often happens with comments) people trust it to be the source of truth. This is horrible.

I think the following mistake is MUCH harder to make:

func getPersonInformation(p Person) (firstName, lastName string) {
	firstName := getFirstName()
	lastName := getAddress()
	// ...
	return
}

In general - I think that comments should be avoided whenever we can document stuff with actual code.

[iholder101]

@lyarwood

That said the shared underlying function used by all paths does actually define these return variables within the function and should have its named return values dropped

IMHO I should not have to look at a function implementation to understand what it returns. That's what function signatures are for! so you would be able to reason about the basic purpose of the function without reading its implementation.

I also agree with @alicefr that empty returns using named return variables should be avoided and should be documented as something to avoid.

Why? :)
If I already know exactly the returned parameters from the function signature, why would we have to repeat them?
In addition, it's sometimes very useful to rely on the default type values.

[alicefr]

@iholder101 yes, I agree on what you said about the comments :). Don't get me wrong, I'm personally not strongly against named return values.

However, when I have tried to use them in the past, I got several reviews telling me to avoid it. What I personally dislike is having a mix of coding styles. For example in https://github.com/kubevirt/kubevirt/blob/main/pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter.go, we have a mixture of name returned values and not for the same variable type (causes).
This is one of the coding pattern which depend on who is going to review your code. I think, it shouldn't be the case.

[iholder101]

@iholder101 yes, I agree on what you said about the comments :). Don't get me wrong, I'm personally not strongly against named return values.

However, when I have tried to use them in the past, I got several reviews telling me to avoid it. What I personally dislike is having a mix of coding styles. For example in main/pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter.go, we have a mixture of name returned values and not for the same variable type (causes). This is one of the coding pattern which depend on who is going to review your code. I think, it shouldn't be the case.

100% agree.

I would say that, in general, unless there's a policy that defines code conventions (like @0xFelix is trying to create) then comments about matters of style can be given as nits, but they shouldn't block the PR or dictate a specific code style (expect for extreme cases).

In addition, while I think that uniformity sometimes has many advantages, in other times it's perfectly fine to use different styles on a case-to-case basis where it makes sense.

[lyarwood]

@iholder101

IMHO I should not have to look at a function implementation to understand what it returns. That's what function signatures are for! so you would be able to reason about the basic purpose of the function without reading its implementation.

Yeah that's fair. In that specific case the function is so short that I didn't think peeking at the implementation was a big issue but as a convention using named returns as a means to document the differences between multiple return variables of the same type in the signature is sound.

Why? :) If I already know exactly the returned parameters from the function signature, why would we have to repeat them? In addition, it's sometimes very useful to rely on the default type values.

I thought about this a little more and I think my issue with naked returns is that they are not explicit enough when the function is over a certain size and you end up having to lookup the signature again to regain context. The nakedret linter attempts to address this concern:

https://github.com/alexkohler/nakedret?tab=readme-ov-file#purpose

[iholder101]

I thought about this a little more and I think my issue with naked returns is that they are not explicit enough when the function is over a certain size and you end up having to lookup the signature again to regain context. The nakedret linter attempts to address this concern:

@lyarwood

To that I can agree :)
In general, I think that the discussions around these topics should be around "when it's good and when it's bad" rather than broadly saying "named return values are bad period".

[0xFelix]

I share @lyarwood's view about naked returns and I think that is what bugs me the most when thinking about named returns. What do you think about @EdDev suggestion in #11456?

@alicefr I also share your view about different coding styles in the same file. IMO we should stick to one coding style in a file to make reviews easier.

[0xFelix]

@lyarwood Is nakedret part of golangci-lint? Can we enable that?

[lyarwood]

@lyarwood Is nakedret part of golangci-lint? Can we enable that?

Yes it's part of golangci-lint and yes I think it would be valid to enable it:

https://golangci-lint.run/usage/linters/#nakedret

[EdDev]

/cc

[lyarwood]

@lyarwood Is nakedret part of golangci-lint? Can we enable that?

Yes it's part of golangci-lint and yes I think it would be valid to enable it:

https://golangci-lint.run/usage/linters/#nakedret

Helps if I actually check if it's enabled already....

kubevirt/.golangci.yml

Line 90 in a18b070

- nakedret

@0xFelix is there anything else to discuss within this issue or can we move the discussion back to the conventions PR?

[0xFelix]

@lyarwood Let's move this discussion back to #11456!

[lyarwood]

/close

Okay closing this out, thanks all.

[kubevirt-bot]

@lyarwood: Closing this issue.

In response to this:

/close

Okay closing this out, thanks all.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.