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

cmd/compile: change "multiple-value .. in single-value context" error message to be consistent with >1 case #26616

Closed
techtonik opened this issue Jul 26, 2018 · 10 comments
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@techtonik
Copy link

What version of Go are you using (go version)?

1.10

What did you do?

bar := bufio.NewReader(...)
line := bar.ReadString()

What did you expect to see?

1 of 2 return values of ReadString() is not handled

What did you see instead?

multiple-value bar.ReadString() in single-value context

The expected error text should be more clear for people without a university degree. =)

@meirf
Copy link
Contributor

meirf commented Jul 26, 2018

It looks like 1 vs >1 is a special case. For example, this code:

package main

func main() {
	x, y := foo()
	println(x, y)
}

func foo() (int, int, int) {
	return 0, 0, 0
}

results in "assignment mismatch: 2 variables but 3 values", which is close to what you expect.

@meirf meirf changed the title Simplify "multiple-value bar.ReadString() in single-value context" error message cmd/compile: simplify "multiple-value .. in single-value context" error message Jul 26, 2018
@techtonik
Copy link
Author

I would still insist on 1 of 3 return values of foo() is not handled.

@agnivade
Copy link
Contributor

The error message is the same vein of how we write failure messages in test - Got- %v, Expected- %v. It says - Got 2 variables, but expected 3 values. It clearly mentions what was expected, and what it received.

I am not sure why the single-value context is considered a special case.

/cc @josharian @mdempsky

@andybons andybons added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 26, 2018
@andybons andybons added this to the Unplanned milestone Jul 26, 2018
@meirf
Copy link
Contributor

meirf commented Jul 27, 2018

@techtonik:

Regardless of whether the 1 vs >1 special case is changed, it sounds like you prefer an alt error message for the general case. I have a couple concerns with your approach:

I would still insist on 1 of 3 return values of foo() is not handled.

  • "1 of 3 return values": This sounds like an ordering. People might think this means that the first of the three returned values doesn't have a variable to be assigned to but the others do. Whereas "1 variables but 3 values" is just about quantity.
  • "is not handled": The word "handle" is a bit vague. Whereas "assignment mismatch" is more precise.

@mdempsky
Copy link
Member

I think it's very unlikely we'd adopt the wording 1 of 2 return values of ReadString() is not handled, for the reasons @meirf explained.

I'm sympathetic to changing the error message to assignment mismatch: 1 variable but 2 values though. I think the current error message can be attributed to an artifact of the compiler having OAS as a special case of the more general OAS2 node, rather than being intentionally inconsistent.

If anyone wants to dig into this, look at typecheckas and typecheckas2 in typecheck.go. In particular, Efnstruct (to suppress the "multi-value in single-value context" error) and IsFuncArgStruct (to distinguish a multi-value expression).

@agnivade agnivade changed the title cmd/compile: simplify "multiple-value .. in single-value context" error message cmd/compile: change "multiple-value .. in single-value context" error message to be consistent with >1 case Aug 12, 2018
@agnivade agnivade added help wanted NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Aug 12, 2018
@gopherbot
Copy link

Change https://golang.org/cl/129065 mentions this issue: cmd/compile: single value error matches multiple value error

@gopherbot
Copy link

Change https://golang.org/cl/131280 mentions this issue: cmd/compile: use "N variables but M values" error for OAS

@techtonik
Copy link
Author

techtonik commented Sep 1, 2018

"1 of 3 return values": This sounds like an ordering. People might think this means that the first of the three returned values doesn't have a variable to be assigned to but the others do.

For ordering it should be 1st of 3 return values, no? Also, does go syntax permit not to assign a variable to returned value? Because if it doesn't, ordering doesn't make sense.

assignment mismatch: 1 variable but 2 values

This one is good. I would only specify that these are "2 return values".

@gopherbot
Copy link

Change https://golang.org/cl/133335 mentions this issue: cmd/compile: use "N variables but M values" error for OAS

@mdempsky
Copy link
Member

mdempsky commented Sep 4, 2018

would only specify that these are "2 return values".

The current wording for x, y = three() is "2 variables but 3 values," so the consistent wording for x = three() is "1 variable but 3 values."

As I mentioned on the CL, I'm open to discussion about changing both instances to "3 return values" (or more precisely "3 result values," per the Go spec), but let's open a new issue for that. Personally, I think the current wording (as of f7a633a) is unambiguous.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

6 participants