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: clearer error message for unkeyed composite literal with unexported field #28053

Open
pjebs opened this issue Oct 6, 2018 · 11 comments
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@pjebs
Copy link
Contributor

pjebs commented Oct 6, 2018

I have a struct:

package df

type SortKey struct {
	Key interface{}
	SortDesc bool
	seriesIndex int
}

From an outside package:

TRY 1

z := df.SortKey{"n", false}

Compiler error: too few values in df.SortKey literal

TRY 2

z := df.SortKey{"n", false, 0}

Compiler error: implicit assignment of unexported field 'seriesIndex' in df.SortKey literal

You can't win. Perhaps it should just be disallowed for Go 2 - It messes up backwards compatibility anyway amongst minor versions of packages (eg. net/http.Request and Context situation).

@mvdan
Copy link
Member

mvdan commented Oct 6, 2018

You cannot modify unexported fields from separate packages. This applies to composite literals too. The error seems clear enough to me.

@mvdan mvdan closed this as completed Oct 6, 2018
@pjebs
Copy link
Contributor Author

pjebs commented Oct 6, 2018

I'm not referring to the error description. I'm suggesting literal assignments should just be banned, or when an unexported field exists, initialised to zero value when attempting to do literal assignment (since you can't access the unexported field anyway).

Please reopen issue.

@mvdan
Copy link
Member

mvdan commented Oct 6, 2018

I don't understand what you mean, but I'm going to reopen this for now.

@mvdan mvdan reopened this Oct 6, 2018
@mvdan
Copy link
Member

mvdan commented Oct 6, 2018

Also, please note that language changes should be proposals, as they need careful consideration. You should try to make your proposal as detailed and clear as possible.

@mvdan mvdan added LanguageChange NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Oct 6, 2018
@pjebs pjebs changed the title Bug fix/inconsistency for literal assignment Proposal: Go 2 : "Fix up" or remove literal assignments Oct 6, 2018
@gopherbot gopherbot added this to the Proposal milestone Oct 6, 2018
@dpinela
Copy link
Contributor

dpinela commented Oct 6, 2018

In your example, the solution would be to use a keyed literal.

Note that using an unkeyed composite literal of a struct type defined in another package is already explicitly recommended against, to preserve compatibility when such types add fields - the Go 1 compatibility promise specifically excludes such unkeyed literals, for example. There's even a proposal to disallow them altogether, and they're already a warning in vet.

@go101
Copy link

go101 commented Oct 6, 2018

@pjebs
If you are the maintainer of the library package, then you can put a zero-size non-exported field in your struct type to avoid using non-keyed literals in user code.

@ianlancetaylor
Copy link
Contributor

@mvdan Minor point: please don't add a NeedsDecision label to Go 2 issues. We're using that as a marker to be applied only after review. Thanks.

@ianlancetaylor ianlancetaylor added v2 A language change or incompatible library change and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Oct 8, 2018
@ianlancetaylor
Copy link
Contributor

@pjebs Like @mvdan , I don't understand what you are proposing. You are describing how the language works today, and you say "perhaps it should just be disallowed," but I don't know what you mean. What should be disallowed? Thanks.

@pjebs
Copy link
Contributor Author

pjebs commented Oct 8, 2018

  1. I was suggesting that literal assignments cause more trouble than they are worth and should be disallowed.
    OR
  2. Literal assignments are allowed but if a field is not accessible (i.e. external pkg + unexported), then the literal assignment would operate as if the unexported fields are non-existent, but upon creation of object, they are zero-valued.

@pjebs
Copy link
Contributor Author

pjebs commented Oct 8, 2018

I showed the 2 different error messages to highlight that the status quo seemed inconsistent, since the first error message says too few values which explains the error perfectly but seems to also imply that the situation can be remedied by adding extra values. ....but adding extra values produces implicit assignment of unexported field, which also explains the situation perfectly. You simply can't win!

Hence the proposal. Remove or rectify.

@dmitshur dmitshur changed the title Proposal: Go 2 : "Fix up" or remove literal assignments Proposal: Go 2: "fix up" or remove literal assignments Oct 8, 2018
@ianlancetaylor ianlancetaylor changed the title Proposal: Go 2: "fix up" or remove literal assignments proposal: Go 2: change handling of composite literals with unexported fields from other packages Oct 8, 2018
@ianlancetaylor
Copy link
Contributor

The solution is to use a keyed literal. The vet -composites check can help with this.

We aren't going to require people to use keyed literals even for structs defined in the same package. And we don't want to have different rules for literals defined in the same package and literals defined in a different package. We could perhaps improve the compiler error messages for these cases. I'll repurpose this issue for that.

@ianlancetaylor ianlancetaylor changed the title proposal: Go 2: change handling of composite literals with unexported fields from other packages cmd/compile: clearer error message for unkeyed composite literal with unexported field Oct 30, 2018
@ianlancetaylor ianlancetaylor added help wanted NeedsFix The path to resolution is known, but the work has not been done. and removed v2 A language change or incompatible library change LanguageChange Proposal labels Oct 30, 2018
@ianlancetaylor ianlancetaylor modified the milestones: Proposal, Unplanned Oct 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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