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

Comments

Projects
None yet
6 participants
@pjebs

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

This comment has been minimized.

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 Oct 6, 2018

@pjebs

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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.

@pjebs pjebs changed the title from Bug fix/inconsistency for literal assignment to Proposal: Go 2 : "Fix up" or remove literal assignments Oct 6, 2018

@gopherbot gopherbot added this to the Proposal milestone Oct 6, 2018

@gopherbot gopherbot added the Proposal label Oct 6, 2018

@dpinela

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Contributor

ianlancetaylor commented Oct 8, 2018

@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 Go2 and removed NeedsDecision labels Oct 8, 2018

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Oct 8, 2018

@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

This comment has been minimized.

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

This comment has been minimized.

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 from Proposal: Go 2 : "Fix up" or remove literal assignments to Proposal: Go 2: "fix up" or remove literal assignments Oct 8, 2018

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

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Oct 30, 2018

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 from proposal: Go 2: change handling of composite literals with unexported fields from other packages to cmd/compile: clearer error message for unkeyed composite literal with unexported field 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