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

proposal: don't export methods of non-exported anonymous fields for the embedding type #21892

Closed
go101 opened this issue Sep 14, 2017 · 13 comments

Comments

@go101
Copy link

go101 commented Sep 14, 2017

This is a Go 2 proposal.

Now, the exported methods of sync.RWMutex will be exported for types embedded its non-exported alias.

type rwMutex = sync.RWMutex

type SyncedInitMap struct {
  rwMutex // to avoid being exported
  internal map[int]int
}

To make the methods of rwMutex not exported, rwMutex mustn't be embedded anonymously now.

As SyncedInitMap.rwMutex is not exported, it is more logical that its methods are also not exported.

@gopherbot gopherbot added this to the Proposal milestone Sep 14, 2017
@go101 go101 changed the title proposal: don't export methods of non-exported embedding types proposal: don't export methods of non-exported anonymous fields Sep 14, 2017
@go101 go101 changed the title proposal: don't export methods of non-exported anonymous fields proposal: don't export methods of non-exported anonymous fields for the embedding type Sep 14, 2017
@jimmyfrasche
Copy link
Member

If you do this then SyncedInitMap satisfies sync.Locker in the defining package but not in other packages.

If you have

func Exported() sync.Locker {
  return &SyncedInitMap{internal: map[int]int{}}
}

what happens when another package calls that?

@go101
Copy link
Author

go101 commented Sep 14, 2017

sync.Locker has methods of Lock and Unlock. So it is ok for the return value to call the two methods.
For example, this is ok:

func Exported() sync.Locker {
  return &unexported{}
}

type unexported struct{sync.RWMutex}

@jimmyfrasche
Copy link
Member

Doesn't that defeat the purpose of hiding those methods from external packages?

@go101
Copy link
Author

go101 commented Sep 14, 2017

no, exposing non-exported methods through interface is a recommended pattern.

@ianlancetaylor
Copy link
Contributor

@go101 What is the purpose of this proposal? It doesn't seem to let Go programs do anything they can't do today, so what errors does it help them avoid?

It's already easy to not export the methods of an embedded field: don't embed the field. The local package can already call the methos on a named unexported field.

@go101
Copy link
Author

go101 commented Sep 15, 2017

It doesn't seem to let Go programs do anything they can't do today

Yes

It's already easy to not export the methods of an embedded field: don't embed the field.

Yes, it may be not hard, but still some inconvenient, it is just some verbose and not that beautiful. :)

@natefinch
Copy link
Contributor

The whole point of embedding a type rather than making it a field is to have the methods and fields of the type get lifted to the containing struct. You seem seem to be arguing that you don't want that to happen..... but then why embed the type?

@go101
Copy link
Author

go101 commented Sep 15, 2017

but then why embed the type

methods are still lifted in the current package internal, they are just not exported to external packages.

@ianlancetaylor
Copy link
Contributor

Every change to Go has benefits and drawbacks. Every language change starts with an automatic drawback: people have to learn something new. When proposing a language change, you need to clearly explain the benefits of the change. So far the only benefits I see are that this proposal is "more logical" and that the current approach is "verbose and not that beautiful." To me those benefits do not seem nearly large enough to overcome the drawback of changing the language.

@go101
Copy link
Author

go101 commented Sep 15, 2017

At least, they are some reasonable reasons. :)

And, for Go 2, for every case like the following one:

type ExportedT struct {
    nonExportedButWithSomeMethods
}

go fix can just add an alias declaration to keep the compatibility

type TnonExportedButWithSomeMethods = struct{nonExportedButWithSomeMethods}
type ExportedT struct {
    TnonExportedButWithSomeMethods
}

@niondir
Copy link

niondir commented Sep 15, 2017

Which make the code more "verbose and not that beautiful." IMHO - which is your argument to add the new behavior.

@go101
Copy link
Author

go101 commented Sep 15, 2017

v.embedded.method is more verbose than v.method. Not?

@rsc
Copy link
Contributor

rsc commented Sep 18, 2017

As I wrote in #18130, "restrictions add complexity" (see the Restrictions section). This proposal directly conflicts with the goal of type aliases, which is that the two types are essentially indistinguishable.

As SyncedInitMap.rwMutex is not exported, it is more logical that its methods are also not exported.

If the method is named Lock, the method is exported. That's just how it works. Note that if you do

type rwMutex struct{} // not an alias
func (rwMutex) Lock() {}

and embed that, then you still get an exported method Lock, so this has nothing to do with aliases. It would be very weird if aliases behaved differently from that example.

If you want Lock/Unlock exported, embed. If you don't, name the field (don't embed), like:

    m rwMutex

and then use foo.m.Lock instead of foo.Lock. It's slightly longer but it keeps the rules regular and predictable.

@rsc rsc closed this as completed Sep 18, 2017
@golang golang locked and limited conversation to collaborators Sep 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants