go 🚀 - proposal: cmd/doc: add "// Unstable:" prefix convention

cc @jba

We've talked about having apidiff ignore changes to definitions that are annotated like this. I imagine it would be useful for other tools as well.

jayconrod on 2 Oct 2019

What are next steps for getting approval for this proposal?

carnott-snap on 9 Nov 2019

@carnott-snap I don't think this really needs to go through the formal proposal process. It's something we agree on and discussed already as part of the apidiff and gorelease design.

I think the next step would be for this to be implemented in golang.org/x/exp/apidff. gorelease will use that.

This should be documented somewhere, but I'm not sure where yet. We should have documentation on preparing a release, linked from https://golang.org/doc/. That should basically be a checklist, and gorelease can refer to it in error messages. These stability comments could be mentioned there.

About the specifics on the comments: I agree that // Unstable: is the best single marker for this. I wouldn't mind having // Experimental: as a synonym, but it's probably better to have one word instead of two. We should require the word to be at the beginning of a paragraph, though it doesn't have to be (and usually should not be) the first paragraph.

jayconrod on 11 Nov 2019

👍1

@carnott-snap I don't think this really needs to go through the formal proposal process. It's something we agree on and discussed already as part of the apidiff and gorelease design.

Is it worth getting this closed/tagged as Proposal-Accepted, or should we start work to prove the benefit first?

I think the next step would be for this to be implemented in golang.org/x/exp/apidff. gorelease will use that.

Does Google want to own the dev work? If not, I am happy to contribute.

This should be documented somewhere, but I'm not sure where yet. We should have documentation on preparing a release, linked from https://golang.org/doc/. That should basically be a checklist, and gorelease can refer to it in error messages. These stability comments could be mentioned there.

Long term it would be nice to mentioning this as part of the module release docs item in #33637. But for now, I think we can follow the lead that // Deprecated: ... has set:

Create https://github.com/golang/go/wiki/Unstable.
Amend the godoc blog post: https://blog.golang.org/godoc-documenting-go-code.

About the specifics on the comments: I agree that // Unstable: is the best single marker for this. I wouldn't mind having // Experimental: as a synonym, but it's probably better to have one word instead of two.

Totally agree, may be worth getting a wider audience, but one acceptable word seems better than two perfect ones.

We should require the word to be at the beginning of a paragraph, though it doesn't have to be (and usually should not be) the first paragraph.

IIRC, doc comments rules will not allow it to be the first paragraph, though type unstable struct{} // Unstable: do not use. should be fine. I think we should follow the // Deprecated: ... conventions, and require the keyword be the first word of the paragraph.

carnott-snap on 12 Nov 2019

Adding "// Unstable:" has a much larger effect than "// Deprecated:".
"// Deprecated" is basically a courtesy: it says "this will keep working but just so you know there is a newer thing you might be happier with." If you don't see the comment, no big deal.

"// Unstable" is much more invasive. It says "even though you might think otherwise, I reserve the right to change the types/code here in the future or delete it entirely and break your uses." If you don't see the comment, it's a very big deal: you upgrade and your code breaks.

The big question is not what the comment syntax should be but whether we want to bless this kind of user-hurting behavior at all.

An alternative would be to develop experimental changes like this on explicitly experimental version tags (v1.6.1-unstable, for example), keeping the unstable code completely out of the stable tag.

Another alternative would be to put the name into the defined symbols, like "mypkg.UnstableFoo", like we did for types like testing.InternalBenchmark (before internal directories). It's impossible to miss the Unstable when it's in the name.

We should very carefully consider what the right way is to let package authors experiment without hurting users. A simple comment does not seem like enough. (I realize that the idea is tools would surface the comment etc but then that's just more mechanism on top, whereas versions or symbol names that explicitly say unstable reuse existing mechanism.)

rsc on 13 Nov 2019

👍8 ❤1

The big question is not what the comment syntax should be but whether we want to bless this kind of user-hurting behavior at all.

Note that there are already at least a few examples of unstable definitions in the standard library.

For example, consider:

unicode.Version (which changes every time the version is updated)
os.ModeType (which would change if a new type bit is added)
text/scanner.GoTokens (which will change if a new kind of token is added to the Go grammar, for example in order to handle generics).

bcmills on 13 Nov 2019

As another data point, there are also several symbols in the testing package: RegisterCover, Cover, CoverBlock, MainStart. These have comments like:

It is not meant to be called directly and is not subject to the Go 1 compatibility document. It may change signature from release to release.

These definitions are only meant to be called by generated code that is tightly coupled with the testing package, so in this case, it might be fine to name things Unstable. I imagine you might see the same pattern any time you have a code generator and a library that go together.

However, some definitions like the ones Bryan mentioned may have their own definition of compatibility. According to apidiff, changing the value of an integer constant is an incompatible change because it may be used as an array length. Using unicode.Version as an array length would be quite strange though, and I don't think there's a need to report a change to that constant as an error in gorelease. I'd like to have some annotation like // Unstable: that gives authors a chance to suppress false positives like this.

jayconrod on 13 Nov 2019

@rsc I'm not sure what the next step is. It doesn't sound like the proposal committee has accepted or rejected this. What information would be useful in making a decision?

jayconrod on 13 Nov 2019

whether we want to bless this kind of user-hurting behavior at all.

As a data point. In the protobuf module, there are several types or functions explicitly marked as being for internal use only. However, they must be exported in order for generated code to access them. We reserve the right to change those APIs with the caveat that we don't break existing usages that were properly generated by protoc-gen-go.

Whether this convention is adopted or not, this type of sharp edge already exists. In v1 the sharp edge is awful since the internal functions is placed alongside public functions in the proto package. In v2, the sharp edge is isolated to a protoimpl package which is explicitly documented as being unstable.

dsnet on 13 Nov 2019

@jayconrod I think that what might be helpful is a reason why you can't use names like ExperimentalFoo.

ianlancetaylor on 13 Nov 2019

👍1

@ianlancetaylor My expectation is that gorelease via apidiff will identify a number of changes as technically incompatible, but developers will want a way to annotate definitions to suppress false positives.

I'm planning to ship an experimental version of gorelease soon ("soon" depends on what other cmd/go issues need to be fixed before the 1.14 beta). Hopefully I'll have some useful feedback to share after that.

jayconrod on 13 Nov 2019

Sorry if this is an unhelpful comment, but the cmd/api tool in the standard library does have a way to suppress false positives, for the kinds of examples that @bcmills cites above. See the files in https://golang.org/api/ .

ianlancetaylor on 13 Nov 2019

I think that what might be helpful is a reason why you can't use names like ExperimentalFoo.

In the description I call out why Experimental is problematic for long term, intentional instability: is Unstable equally acceptable?
- If so, s/Experimental/Unstable/g.
Experimental is quite long to add to every unstable symbol.
Would const experimentalFoo = "bar" be stable?
Would this apply to functions retroactively?
- type ExperimentalRoute interface{} where the link hardware is experimental, not the Go api.
- github.com/moby/[email protected]/api/server/router.ExperimentalRoute is stable
What is the migration path for symbols that are partially stable? (read: custom compatibility guarantees)
- It would be a breaking change to convert gRPC code generator's logic fromtype XxxService interface into type ExperimentalXxxService.
Must Experimental be the first word, or simply prefix in the symbol?
- func ExperimentallyInvestigate() {}
- func GetExperimentalFoo() {}
How do I mark a whole package or module?
- Can we reserve exp or experimental, like we did internal?
- Would a lowercase prefix be clear?
- syscall/experimentaljs
- golang.org/x/exp/experimentalapidiff
- Is documentation sufficient?
- See the current state of syscall/js, golang.org/x/exp, etc.
- Currently apidiff and gorelease are not mindful of this.
- golang.org/x/exp/apidiff.ExperimentalChange has stutter.
- What protects you from accidentally committing to something: golang.org/x/exp/apidiff.Change.

carnott-snap on 14 Nov 2019

👎2

I didn't mean anything special about the exact word Experimental. I just meant, as in https://github.com/golang/go/issues/34409#issuecomment-553546180, to put the instability into the symbol name itself rather than a comment. A comment can be missed. It's much harder to miss the symbol name.

ianlancetaylor on 14 Nov 2019

The (IMO pretty major) downside of putting words like Experimental in the identifiers (or in the package path, for that matter) is that it imposes an additional breaking change if/when the API is promoted from experimental to stable.

(It's the same reason we don't distinguish between v0 and v1 in module paths.)

bcmills on 14 Nov 2019

👍3

It's much better to use branches for experimental features. This makes 2 worlds(stable and experimental) independent and reduces possible mistakes for the user.

As Russ mentioned above: comment is too weak to protect user from incorrect usage.

Extending previous comment by Bryan: v0 and v1 means that the module can be experimental, but in the same time we can use replace statement in go.mod to use a module from specific branch with experimental/unstable/ things.

In other words: comments or unstable api increases _code entropy_ which increases _code maintainability_.

cristaloleg on 14 Nov 2019

I built this proposal around documentation to make adoption less invasive. This was partially based on experience with the protobuf libraries, where they define custom compatibility guarantees. I agree condoning unstable interfaces is troublesome, but many projects require it, and I wanted a canonical way to label/identify it, both for users and tooling.

It may be helpful to isolate the use cases that exist for instability: (please correct me if I left anything out or if this is a false trichotomy)

We have experimental things and want to change/remove them.
- Call this the pre-release case (or alpha, beta).
- packages
  - golang.org/x/exp
  - syscall/js
- symbols
  - golang.org/x/tools/go/analysis.ObjectFact
We would like to define our own compatibility guarantees that differ from the apidiff/Go 1 compatibility.
- Call this the we know better than the language case.
- unicode.Version
  - This will not randomly become a func, but the value and length may change.
- Call site compatibility
  - Protobuf generated code type XxxServer interface must be implementable and allow adding methods.
- Considering the complexity of the apidiff spec, this will keep cropping up; sometimes it could be useful to break some guarantees to give extra features: e.g. change a constant's value when you know it will not be used as an array length.
We want to share an internal interface between two packages.
- Call this the protected api surface case.
- testing.RegisterCover
- gRPC generated methods against message structs: e.g. XXX_Unmarshal.
- Some of these are required because of the limited nature of the Go language, others are bad practice, frequently it is hard to tell the difference.

I think we all agree that we _want_ stable packages, but even the standard library is developing experimental or long term unstable features. Most examples use documentation to signal their stability, so this seemed intuitive and canonical. What emphasis should be placed on supporting existing patterns, as opposed to preventing undesirable behaviour?

Outstanding concerns:

Branches and tags will not work well with 2. or 3. Authors want the ability to ship official, (partially) unstable releases.
The Experimental proposal does not appear to be a specification, so much as a convention. This lack of tooling support makes it incompatible with my intentions.

carnott-snap on 14 Nov 2019

@rsc I'm not sure what the next step is. It doesn't sound like the proposal committee has accepted or rejected this. What information would be useful in making a decision?

The proposal committee's job is to help steer a discussion toward consensus, not to decide on their own. It sounds like there is no consensus here yet.

I wrote a lot about this topic at https://research.swtch.com/proposals-experiment. It's very important that people know they are using experimental features. Otherwise we run the risk of hitting the situation Rust did where everyone was on "unstable". I have the same concern about the "exp" build tag in #35704: it's too easy to lapse into where everything uses this.

Actually the build tag may be worse, since if any dependency super far down uses exp, you have to do the same. So basically everyone will have to use it.

rsc on 20 Nov 2019

👍3

I would like to be clear about the purposes of the two tickets being discussed:

#34409: documenting any form of feature instability
#35704: gating alpha, beta, testing, pre-release features that will be stabilised

The proposal committee's job is to help steer a discussion toward consensus, not to decide on their own. It sounds like there is no consensus here yet.

My concern is that the current state of things is not good. Major tools are actively developing and releasing unstable features, by apidiff's measure, that have a myriad of ways to signal their stability guarantees. This is hard on consumers, and may lead to more instability if we do nothing.

I wrote a lot about this topic at https://research.swtch.com/proposals-experiment. It's very important that people know they are using experimental features.

Would a go vet lint be sufficient? We already do this for // Deprecated:. What do you define as _know they are using_? This seems like a very tricky litmus test, e.g. dep was relied upon heavily by the community, despite never leaving experiment status.

Otherwise we run the risk of hitting the situation Rust did where everyone was on "unstable".

I have worked with Rust myself and do not see this today. Are there any lessons we can learn from their experiences? My understanding was that they had a lot of important features that needed to be stabilised, e.g. async, and that finally unlocking things from the nightly compiler was the fix.

I have the same concern about the "exp" build tag in #35704: it's too easy to lapse into where everything uses this.

Actually the build tag may be worse, since if any dependency super far down uses exp, you have to do the same. So basically everyone will have to use it.

Since #35704 is trying to solve a different problem, would you mind continuing the discussion there? I broke it out partially because I saw two heterogeneous problems (pre-release and custom compatibility) that felt like they may should be solved differently.

carnott-snap on 20 Nov 2019

👍1

Maybe this has already been suggested somewhere, but would it not be better to support all tags of the form // TagName:, and then allow filtering for including or excluding the items tagged with TagName, and by default, group tagged items together? Like that we can avoid this discussion on whether or not a specific tag is a good idea or not, and also avoid officially "blessing" any tag in particular.

beoran on 24 Jan 2020

👍1

@beoran, I think the point of this proposal is the semantics, not the syntax: if we want to know how to identify an unstable API we _must_ “bless” a specific tag as having that meaning, or else no one will know to add it.

bcmills on 24 Jan 2020

Is any additional information required, or is this proposal on Hold, but not correctly labeled? It appears to have missed the past few proposal review meetings: #33502.

carnott-snap on 5 Feb 2020

I am very uncomfortable with this proposal as written, for the reasons I outlined in November (https://github.com/golang/go/issues/34409#issuecomment-553546180). This is too big a thing to bury in a comment. I still believe it should be something else, most likely an "Unstable" prefix on the name itself.

rsc on 7 Feb 2020

But the whole point of unstable is that it's unstable. By using them you are accepting breakage. Better that than not realizing there's any breakage in the first place because that fact - which runs counter to all conventions in the ecosystem - is buried in a comment that you're likely not to notice without special tooling. And there can be an arbitrarily long transition in which the "final" Unstable version wraps the stable ones if the module author so chooses.

rsc on 7 Feb 2020

@rsc, how would you propose to address the unstable-to-stable transition (https://github.com/golang/go/issues/34409#issuecomment-553919623)?

We intentionally avoid a breaking change at that transition at the module level, and having an Unstable identifier as a permanent alias for an actually-stable API seems (a) noisy and (b) misleading.

Moreover, in some cases (like unicode.Version and text/scanner.GoTokens) the name of the unstable identifier is already well established, so adding a prefix to the name would itself be a breaking change — typically of a much larger magnitude than the usual changes to those identifiers.

bcmills on 7 Feb 2020

It has already been mentioned in the thread, but not detailed, so wouldn't a directory part "/unstable/" in the package name be enough to mark it as unstable. Migration from unstable to stable is then just a matter of removing the /unstable/ part. The goimports tool could be enhanced to do this.

beoran on 7 Feb 2020

Bumping this thread, since having no solution is still quite painful and it appears to have been skipped in the most recent proposal review meetings, #33144

Let me know if additional information, discussion, or something else is required.

arnottcr on 9 Mar 2020

More discussion needs to happen here - I haven't seen a solution that fits all the requirements people have yet. I hope to get some time to think about it outside of a proposal meeting.

rsc on 11 Mar 2020

It seems like this discussion has stalled to a degree, so it may be useful to take a step back. Can we get consensus that there are no more use cases to consider, see above, and if we want to support none, all, or just some of them?

In my opinion:

is required
seems like an anti-pattern.
is a nice to have

arnottcr on 11 Mar 2020

Furthermore, can we build a list of abstract requirements for any acceptable solution? I will begin based upon both my opinions and cited comments:

Consumption @rsc

The feature must be opt in, both for consumers and library authors. Much like, deprecation if you opt out and ignore it, things will not break.

Automation

The syntax must be programmatically accessible, so that tools like gorelease and go vet can apply semantic meaning to unstable symbols.

Stabilisation @bcmills

There must be a way to convert an unstable symbol into a stable one. This should not involve more breaking changes or expense for the consumer.

Migration

There should be a clear path to migrate existing symbols like unicode.Version. This need not be trivial or easy, but should be universal.

Application

The syntax should be applicable to modules, packages, and symbols.

arnottcr on 11 Mar 2020

Based upon these criteria, we can rank the proposed solutions:

Comment

This refers to the initial proposal that uses godoc comments: // Unstable: ....

Consumption

This is a hard fail. If you do not see the comment, you do not realise it could break. This exists today and is a known issue.

Automation

Godoc strings are part of the ast and trivial to parse.

Stabilisation

Removing the section from the comment stabilises the symbol.

Migration

The line can be applied to all symbols trivially.

Application

This can be applied to a package or symbol. With extension to go.mod, it could apply to modules too.

Example

gRPC

package service

// Unstable: interface only provides call site compatibility.
type MessageServer interface {
        Do(context.Context, *Message) (*Message, error)
}

type Message struct {
        Text             string   `protobuf:"bytes,1,opt,name=text,proto3" json:"text,omitempty"`
        NoUnkeyedLiteral struct{} `json:"-"` // Unstable: internal api do not use.
        Unrecognized     []byte   `json:"-"` // Unstable: internal api do not use.
        Sizecache        int32    `json:"-"` // Unstable: internal api do not use.
}

func (m *Message) GetText() string

// Unstable: internal api do not use.
func (m *Message) Size() int

stdlib

// Unstable: This package is EXPERIMENTAL. It is exempt from the Go compatibility promise.
package syscall/js

```go
package unicode

const Version = "12.0.0" // Unstable: string only provides call site compatibility.

```go
package testing

// RegisterCover records the coverage data accumulators for the tests.
//
// Unstable: this function is internal to the testing infrastructure and may
// change. It is not covered (yet) by the Go 1 compatibility guidelines.
func RegisterCover(c Cover)

Git tag

This refers to any git/module level release feature, such as custom v1.2.3-unsafe tags mentioned by @rsc or branches and replace as mentioned by @cristaloleg.

Consumption

While it could be argued that the cost of adoption is too high, this does meet requirements. For example, gRPC code generation would not allow authors to release unstable features with normal code because this happens at the module level.

Automation

Both module versions and go.mod files are trivially digestible.

Stabilisation

(Re)release the module with a valid, stable tag.

Migration

This is a hard fail, but only for the standard library, since it is not a module. It is possible that all unsafe behaviour could be extracted into the penumbra standard library.

Application

This can only be applied to a module.

Example

These are elided because they are identical to the reference example section below, just on a custom tag or separate branch.

Prefix

This refers to path, golang.org/x/unstable, package syscall/unstablejs, and symbol, unicode.UnstableVersion prefixes, as proposed by
@rsc and @ianlancetaylor. Note that path prefixes would apply recursively, like internal.

Consumption

While it could be argued that apidiff.Change does not clearly indicate that the symbol is unstable, all three cases embed the guarantee in the fully qualified import path.

Automation

Both import path and symbol name are available via the ast.

Stabilisation

This is a soft fail, because removing any character from a fully qualified symbol name is a breaking, however type aliases can help.

Migration

While it could be argued that it breaks go1 compatibility to convert to testing.UnstableRegisterCover or unstabletesting.RegisterCover, either a type alias and deprecation, or following through on the implied instability should be allowed.

Application

This can be applied to a module, e.g. golang.org/x/exp, package, or symbol.

Example

gRPC

package service

type UnstableMessageServer interface {
        Do(context.Context, *Message) (*Message, error)
}

type Message struct {
        Text                     string   `protobuf:"bytes,1,opt,name=text,proto3" json:"text,omitempty"`
        UnstableNoUnkeyedLiteral struct{} `json:"-"`
        UnstableUnrecognized     []byte   `json:"-"`
        UnstableSizecache        int32    `json:"-"`
}
func (m *Message) GetText() string
func (m *Message) UnstableSize() int

stdlib

// This package is EXPERIMENTAL. It is exempt from the Go compatibility promise.
package syscall/unstablejs

```go
package unicode

const (
UnstableVersion = "12.0.0"
Version = UnstableVersion // Deprecated: use UnstableVersion.
)

```go
package testing

// UnstableRegisterCover records the coverage data accumulators for the tests.
// NOTE: This function is internal to the testing infrastructure and may change.
// It is not covered (yet) by the Go 1 compatibility guidelines.
func UnstableRegisterCover(c Cover)

Build tag

This refers to the use of a custom build time compile tag: // +build unstable.

Consumption

While it could be argued that this solution is cancerous and too burdensome, it does meet requirements.

Automation

Tools could explicitly not pass this tag when generating the ast.

Stabilisation

Remove the build tag from source.

Migration

Even the standard library could add these tags to any symbol, though like above, it would involve following through on the implied instability.

Application

This can be applied to packages or symbols. With extension to go.mod, it could apply to modules too.

Example

gRPC

// +build unstable

package service

type MessageServer interface {
        Do(context.Context, *Message) (*Message, error)
}

type Message struct {
        Text                 string   `protobuf:"bytes,1,opt,name=text,proto3" json:"text,omitempty"`
        NoUnkeyedLiteral struct{} `json:"-"`
        Unrecognized     []byte   `json:"-"`
        Sizecache        int32    `json:"-"`
}

func (m *Message) Size() int

```go
// +build !unstable

package service

type Message struct {
Text string protobuf:"bytes,1,opt,name=text,proto3" json:"text,omitempty"
}

```go
package service

func (m *Message) GetText() string

stdlib

// +build unsable

package syscall/js

```go
// +build unstable

package unicode

const Version = "12.0.0"

```go
// +build unstable

package testing

// RegisterCover records the coverage data accumulators for the tests.
func RegisterCover(c Cover)

Reference example

gRPC

package service

type MessageServer interface {
        Do(context.Context, *Message) (*Message, error)
}

type Message struct {
        Text                 string   `protobuf:"bytes,1,opt,name=text,proto3" json:"text,omitempty"`
        XXX_NoUnkeyedLiteral struct{} `json:"-"`
        XXX_unrecognized     []byte   `json:"-"`
        XXX_sizecache        int32    `json:"-"`
}

func (m *Message) GetText() string
func (m *Message) XXX_Size() int

stdlib

// This package is EXPERIMENTAL. It is exempt from the Go compatibility promise.
package syscall/js

```go
package unicode

const Version = "12.0.0"

```go
package testing

// RegisterCover records the coverage data accumulators for the tests. NOTE:
// This function is internal to the testing infrastructure and may change. It
// is not covered (yet) by the Go 1 compatibility guidelines.
func RegisterCover(c Cover)

arnottcr on 11 Mar 2020

👍2

I don't like the scope of the proposal. Users should not have to deal with this complexity inside the package scope. The experiment should be contained by a package scope: Either the entire package is experimental, or none of it is. If you want an experimental function, import the experimental package.

The disadvantage of having the convention outlined in this proposal is that it will facilitate the introduction of experimental/unstable functions. I expect a package to have a smooth gradient of stability across all exported identifiers.

As a developer, you should not have to ask yourself, "How stable is this function, now that I've found it?".

as on 12 Mar 2020

I don't like the scope of the proposal. Users should not have to deal with this complexity inside the package scope.

I agree, unfortunately this is a pattern in use today, without safety mechanisms. I believe an official definition and tooling will make things better, not worse. Recall that even the standard library does this: unicode.Version, testing.RegisterCover.

There may also be a middle ground where we can make canonical some of the lessons and best practices that the gRPC team has made over times, since they have been managing and solving this without major incident or adoption loss.

The experiment should be contained by a package scope: Either the entire package is experimental, or none of it is. If you want an experimental function, import the experimental package.

Not all instability is caused by experimentation. Authors currently release "official" production code that is not compliant with go1 compatibility.

That being said, I like this restriction, just fear it does not work in all cases.

Stabilisation is now even more expensive than unicode.UnsafeVersion already is. How does a package publish a compatible experimental version, are they dependant, what about func init?
How do we migrate existing symbols when they loose access to private symbols?
How would one implement an unstable struct field or method?

arnottcr on 12 Mar 2020

For the “pre-release” use case, I think we already have a couple of reasonable alternatives:

Put the unstable identifiers on a branch, and don't tag any release versions on that branch. Then, anyone using the unstable identifiers ends up with a pre-release or pseudo-version in their dependencies (as reported by go list -m all and/or their own go.mod file).
Put the unstable identifiers in a separate package, and carve that package off into a nested module at a pre-release version. When the new API is promoted to stable, fold it into the main module and (optionally) tag one final version of the nested module: an empty module that requires the main module at the now-stable released version.

bcmills on 12 Mar 2020

For the “internal API hooks” use case, I don't believe it is ever actually _necessary_ to expose the unstable identifiers in exported fashion. (For example, note that the new protobuf reflection API eliminates the need for XXX_ fields.) Alternatives there include:

If the hooks share a common import-path prefix, use internal packages.
If the hooks expose methods for some reflection-based API, add a proper (stable) reflection API, and/or thread the methods through using unexported methods on some embeddable type (as described in golang/protobuf#276).
If you _really_ need to expose functionality only to _specific_ external packages, have the caller pass in an interface{} with an appropriate callback method and use reflect to verify that that interface is backed by an unexported type in one of the approved external packages. (If need be, you can do the reflect check in an init function or a function guarded by a sync.Once — all of the dispatch can avoid reflection after that point.)

bcmills on 12 Mar 2020

So for me, the compelling remaining use-case is really only the “exception to Go 1 compatibility” case.

I discussed that a bit more with @jayconrod and @matloob earlier this week, and one option we came up with is to encode the compatibility policy as a test function: if the test passes (without needing to be changed), then the identifier is deemed to still be “compatible”.

For unicode.Version, that might consist of something like:

package unicode

const Version = "12.0.0"  // Unstable: see TestVersionCompatibilityPolicy.

package unicode_test
…

func TestVersionCompatibilityPolicy(t *testing.T) {
    // unicode.Version must be an untyped string constant.
    const _ = unicode.Version + ""
    var _ string = unicode.Version
    type untypedString string
    var _ untypedString = unicode.Version

    // unicode.Version must consist of three dot-separated numbers.
    parts := strings.Split(unicode.Version, ".")
    if len(parts) != 3 {
        t.Errorf("must contain 3 dot-separated parts")
    }
    for i, p := range parts {
        n, err := strconv.Atoi(p)
        if err != nil || n < 0 {
            t.Errorf("element %d (%s) must be a non-negative number", i, p)
        }
        if i == 0 && n == 0 {
            t.Errorf("first element %s must be strictly positive")
        }
    }
}

But I'm not sure exactly how to fit that in to a declaration-site annotation for tools like gorelease or gopls to consume.

bcmills on 12 Mar 2020

For the “pre-release” use case, I think we already have a couple of reasonable alternatives:

Many projects are not following this advice today and instead using some other signalling mechanism (frequently comments), should we work with them to migrate to these alternatives: e.g. grpc.UnaryClientInterceptor. What would you say to authors that want to release unstable symbols as part of a stable release?

For the “internal API hooks” use case, I don't believe it is ever actually necessary to expose the unstable identifiers in exported fashion.

While I agree with internal, are we really suggesting that reflection be a canonical solution? I get that it makes for a clean interface, but using it everywhere has real costs. Currently encoding/json parsing speed is reduced to the level of interpreted languages, and code complexity is inflated from having to deal with type safety at runtime. I hope there are not severe performance implications to the new Protocol Buffers API.

So for me, the compelling remaining use-case is really only the “exception to Go 1 compatibility” case.

I both like this approach and am daunted by the implications. It reads a lot like the old generics contract proposal. This is good, since it may be familiar, but it is effectively a Go language DSL, and having two duelling formats so close seems folly. We could try and leverage contracts logic for implementation, but the new syntax is more about describing types than behaviours. Other notes:

I think this justifies a function prefix like Benchmark or Example: e.g. func CompatibilityVersion().
- We could add a testing.C to accommodate.
- We should use panic as the failure mode.
- We would need to define a spec like with examples for mapping func XxxYyy to a given symbol type Zzz, or did you want to use the comment to do this?
How do we describe this compatibility policy to humans?
- This format is bespoke and it may be unclear what is and is not guaranteed.
Is it compatible to modify custom definition:
- Less restrictive seems fine, but may have some thorny edge cases.
- More restrictive seems breaking: how do you verify this did not happen?
I would almost prefer a custom DSL that allows is based around the primitives described in apidiff.

carnott-snap on 17 Mar 2020

If the hooks expose methods for some reflection-based API, add a proper (stable) reflection API, and/or thread the methods through using unexported methods on some embeddable type (as described in golang/protobuf#276).

I don't understand how this helps with compatibility. In your snippet (https://play.golang.org/p/yDjZZtGmB6), SomeGeneratedMessage.XXX_InternalUnrecognized is visible to clients, just as if it were written directly in the struct. (The only difference is that clients can't set the field in a struct literal.) apidiff would report removal of, or a change in, XXX_InternalUnrecognized as a breaking change.

jba on 18 Mar 2020

I don't understand how this helps with compatibility. In your snippet (https://play.golang.org/p/yDjZZtGmB6), SomeGeneratedMessage.XXX_InternalUnrecognized is visible to clients, just as if it were written directly in the struct. (The only difference is that clients can't set the field in a struct literal.) apidiff would report removal of, or a change in, XXX_InternalUnrecognized as a breaking change.

Yeah, this can be seconded by the fact that google.golang.org/protobuf/runtime/protoimpl calls out its instability. impl is just a way to re-export /internal/ symbols fromimport "google.golang.org/protobuf/internal/impl", thus complicating the interface, but not changing the compatibility.

package protoimpl // import "google.golang.org/protobuf/runtime/protoimpl"

Package protoimpl contains the default implementation for messages generated
by protoc-gen-go.

WARNING: This package should only ever be imported by generated messages.
The compatibility agreement covers nothing except for functionality needed
to keep existing generated messages operational. Breakages that occur due to
unauthorized usages of this package are not the author's responsibility.

carnott-snap on 18 Mar 2020

What would you say to authors that want to release unstable symbols as part of a stable release?

I would say: “Don't do that. If it goes into a release, users will start depending on it no matter what you tell them about stability.”
(See also https://www.hyrumslaw.com/.)

While I agree with internal, are we really suggesting that reflection be a canonical solution?

I am suggesting that reflection can be used as a handshake mechanism. The reflective handshake does not need to be in the critical path.

(I'm happy to give more specific detail for specific code examples, but that seems like a digression for this proposal.)

[This approach] is effectively a Go language DSL, and having two duelling formats so close seems folly.

To be clear, the test function is just a test function: in order to evaluate it, you compile and run the test. (There is no second-order effect as in the generics draft.)

bcmills on 18 Mar 2020

For google.golang.org/protobuf/runtime/protoimpl in particular, the last draft I saw of the design of that package (which was admittedly a couple of years ago) was substantially different, and was not specifically called out as unstable.

So I'm not sure why that package ended up the way it did. (@dsnet and @neild might be able to shed some light on what alternatives were considered, and why the exported-but-unstable approach was taken instead.)

bcmills on 18 Mar 2020

I would say: “Don't do that. If it goes into a release, users will start depending on it no matter what you tell them about stability.”

What does that say about google.golang.org/grcp.UnaryClientInterceptor, google.golang.org/protobuf/runtime/protoimpl, or syscall/js? There are times where I am not given a choice if I integrate with Protocol Buffers or gRPC.

I am suggesting that reflection can be used as a handshake mechanism. The reflective handshake does not need to be in the critical path.

That seems fair, net/rpc does walk a nice line between using reflection for an ergonomic interface and being performant, however what about cases where you need critical path, shared, internal interfaces? Part of what I want to do is create canonical suggestions for interfaces and stability.

To be clear, the test function is just a test function: in order to evaluate it, you compile and run the test. (There is no second-order effect as in the generics draft.)

This means that a programmatic interface is either infeasible or impossible, violating the primary reason for submitting this proposal. Plus, manually detecting changes from release to release gets harder: can I add a new check to the validation function.

carnott-snap on 18 Mar 2020

So I'm not sure why that package ended up the way it did. (@dsnet and @neild might be able to shed some light on what alternatives were considered, and why the exported-but-unstable approach was taken instead.)

The protoimpl package is due to a design limitation which is almost unique to protobufs: The protobuf ecosystem contains:

Support packages in the google.golang.org/protobuf module.
User-written packages in third-party modules.
Generated packages that contain code "authored" by the code generator but that live in third-party modules.

There are three edges between these packages:

User-written packages use the stable, exported API of the support packages.
User-written packages use the stable, exported API of the generated packages.
Generated packages use the protoimpl package.

The protoimpl package is unusual in that generated packages need to interface with the support packages via an exported API, but we do not want to provide this API as a stable, supported interface for user code. Obviously, the protoimpl API needs to be somewhat stable, in that we wish to retain compatibility with existing generated code. What we don't want is to field support questions for that API, or to provide compatibility for anything other than generated packages produced by our own code generator.

In addition, we have a more limited compatibility promise when it comes to generated code--the google.golang.org/protobuf runtime will support generated packages produced by a code generator up to one year older than it. In practice, I expect we'll underpromise and overdeliver on that guarantee, but this is another way in which the protoimpl package is unusual--we reserve the right to change it in ways that will break older generated packages.

There are other ways we could have structured this, but we feel that it strikes a good balance between providing stable, supported, user-facing APIs while preserving our ability to make improvements in the non-user-facing aspects of the generated code.

neild on 18 Mar 2020

The visibility model for Go packages is simplistic where internal packages can only be visible to other packages under the same root path. Generated protobufs need a different form of visibility where the protobuf runtime implementation (which is exactly what protoimpl is) is only visible to generated packages. However, no such feature in the build system exists that enables us to express that visibility constraint. The package documentation in protoimpl is a soft signal of that expectation.

As @neild said, it is incorrect to say that protoimpl is "unstable". It is not. It is stable only to degree that is sufficient to enable code generated protoc-gen-go to continue working.

I don't think protoimpl is a good example of the definition of "unstable" that this issue is trying to argue for. It is certainly less stable than most other user-facing APIs, but it is not "unstable" either. Trying to include it's definition of "unstable" into your definition of "unstable" would just lead to confusion.

What does that say about google.golang.org/grcp.UnaryClientInterceptor, google.golang.org/protobuf/runtime/protoimpl, or syscall/js? There are times where I am not given a choice if I integrate with Protocol Buffers or gRPC.

As stated earlier, you should _never_ depend directly on protoimpl. Only generated .pb.go files should ever do that. Tampering with .pb.go files after generation or producing your own outside of the officially released protoc-gen-go is outside of our support story.

dsnet on 18 Mar 2020

👍1

Simpler protoimpl, upon reflection: It's a super weird corner case because generated protobuf code doesn't follow the usual visibility rules. Just ignore it, it's not a point for or against anything.

neild on 18 Mar 2020

Simpler protoimpl, upon reflection: It's a super weird corner case because generated protobuf code doesn't follow the usual visibility rules. Just ignore it, it's not a point for or against anything.

Sorry if it feels like I called out protobuf, if anything I am impressed at the claims and guarantees the authors have kept. That being said, I have seen these patterns in other IDL/SerDe/RPC frameworks leveraging code generation, so I think carving out a solution for internal APIs is important. See cap'n proto, flatbuffers, net/rpc, thrift, messagepack, and even grpc.

carnott-snap on 19 Mar 2020

I reread this issue and I find that I don't understand what problem this is trying to solve.

Names like those in the protoimpl package or testing.InternalTest and friends should only be used by generated code. They are not unstable in any useful sense; they simply should not be called. The comments on these names can indicate without needing an "unstable" convention. They are not unstable, they are internal.

A package that has not been released is unstable by definition. Comments in the package can indicate what may change and what may not, but I don't see any benefit to a comment convention.

Names like unicode.Version may be unstable in the apidiff sense, but I don't think these names pose any trouble for any actual users. unicode.Version is not unstable; it just doesn't happen to have the same value in every Go release.

So I think we must be talking about experimental features in released versions of released packages. And we're talking about cases where the author isn't sure whether the name is experimental or not; it might be the final version or it might not. And any future changes might not be backward compatible; it's not something like changing the value of unicode.Version, it's something like adding an argument to a function.

Is this really a common case? And if it is, does an "unstable" comment help more than just an ordinary comment? I'm not saying that an ordinary comment solves a problem here, but I'm wondering how an "unstable" comment is a better solution.

What is an example of a problem that would be avoided by this comment convention?

Thanks, and sorry if I'm missing something.

ianlancetaylor on 2 May 2020

Names like those in the protoimpl package or testing.InternalTest ... are not unstable in any useful sense; they simply should not be called.

Sure, but these symbols are available to consumers and, like protoimpl, may not be obviously internal or could have changed since one last read the documentation. Can we build a specification that will allow for automated detection? (Say // Internal: only for developers over four feet tall.) This would allow liners to warn users about what is going on.

I had rolled this into the greater // Experimental: xxx spec to reduce the _api surface_, but if it is a large use case and different, I agree we should not overload things.

Names like unicode.Version may be unstable in the apidiff sense, but I don't think these names pose any trouble for any actual users. unicode.Version is not unstable; it just doesn't happen to have the same value in every Go release.

For unicode.Version, I can write code that will break when the value changes, e.g. var array [len(unicode.Version)]uint. This is an extreme example, and I used it mostly because it existed in the standard library, and you all are much better about strict compatibility. However, many authors do want to provide limited, e.g. call site, compatibility, and as consumers start testing with mocks, using first class functions, and extending interfaces, things will break.

I am also fine calling out that this is non-canonical and not really supported, since the only solutions I have seen are ad-hoc and largely godoc driven. But either it needs to documented or supported. The weak middle ground we have now is hard on users.

Is this really a common case?

I do not know how to automate such a query, so I investigated three places I am familiar with: google.golang.org/grpc, golang.org/x, and stdlib

Stable packages with experimental symbols:

google.golang.org/grpc: 40
google.golang.org/grpc/credentials: 1
google.golang.org/grpc/grpclog: 1
google.golang.org/grpc/resolver: 1
golang.org/x/tools/go/analysis: 11

Experimental packages: (While considered acceptable, there is no detect them automatically; see below.)

google.golang.org/grpc/attributes
google.golang.org/grpc/encoding
google.golang.org/grpc/profiling
google.golang.org/grpc/serviceconfig
google.golang.org/grpc/tap
golang.org/x/tools/go/ssa/
golang.org/x/tools/refactor/satisfy
syscall/js

And if it is, does an "unstable" comment help more than just an ordinary comment? I'm not saying that an ordinary comment solves a problem here, but I'm wondering how an "unstable" comment is a better solution.

What is an example of a problem that would be avoided by this comment convention?

The big win would be for automated consumers like linters and gorelease. I want tooling that can warn users about experimental symbols, or even an integration with the toolchain to hide experimental symbols from go doc.

I will concede, for this use case I really like the idea of using build tags, since it makes everything clear and opt in. It also integrates well into the toolchain, since go doc would hide them automatically, but pointed out they trend toward cancerous. Build tags also do nothing to assist with internal or custom compatibility symbols.

arnottcr on 2 May 2020

I reread this issue and I find that I don't understand what problem this is trying to solve.

The problem here is that mature projects have no way of evolving without:

Bump major versions of an entire module whenever a change in course in one part of the API is needed.
Creating separate modules for experimental features.
Long-term development of experimental features on branches.

(1) causes pain for users to migrate and manage their dependencies.
(2) is infeasible for many reasons - e.g. no way to experiment with an API that adds a function to an existing package where it would naturally belong.
(3) has a huge maintenance burden and likely results in nobody trying the experimental feature, due to being undiscoverable. Also, where stable features are built on top of experimental features, this is even more difficult to manage.

So I think we must be talking about experimental features in released versions of released packages. And we're talking about cases where the author isn't sure whether the name is experimental or not; it might be the final version or it might not. And any future changes might not be backward compatible; it's not something like changing the value of unicode.Version, it's something like adding an argument to a function.

Is this really a common case?

I can't say whether it's common, but it happens.

And if it is, does an "unstable" comment help more than just an ordinary comment? I'm not saying that an ordinary comment solves a problem here, but I'm wondering how an "unstable" comment is a better solution.

What is an example of a problem that would be avoided by this comment convention?

Apparently, many developers do not read or abide by advisory docstring comments.

For an example, see: https://github.com/etcd-io/etcd/issues/12124

etcd made use of APIs explicitly marked in grpc's documentation as experimental. In fact, it made some of these part of its _own_ API. There are comments in the issue with many positive responses arguing that gRPC's documentation is, in fact, not sufficient for this purpose.

Having a standard for calling out portions of an API as unstable, legitimizing API experimentation within a stable release version, and providing a way for the build system to allow/reject the use of these symbols would be a big help for allowing a mature project to grow and evolve in a responsible way.

dfawley on 20 Nov 2020

👍4

etcd made use of APIs explicitly marked in grpc's documentation as experimental. In fact, it made some of these part of its own API.

If this proposal is accepted, this could be easily prevented with a go vet warning.
If an exported API is based on unstable features from another package, it must be marked as unstable as well. Otherwise vet should complain.

maja42 on 22 Nov 2020

We should talk more about etcd-io/etcd#12124. Here we have a real-world example that is causing actual pain. To save you the trouble of reading through the issue, here's what happened:

grpc had an experimental package called naming.
etcd chose to use it in a package of their own. And not merely use it internally; the API of the etcd package mentions symbols in the grpc package. For instance, the etcd package has the method func (*GRPCResolver) Resolve(target string) (naming.Watcher, error), where naming refers to the experimental grpc package.
In a minor release, grpc removed the naming package.

According to semver rules, grpc should not have made a breaking change in a minor release. But their versioning policy makes it clear that this is deliberate and principled. As argued above, they don't see a better way to deal with experimentation. I'd like to better understand their reluctance to use major versioning. I would think that if they kept backwards compatibility and did a major release every six months to a year to remove or change experimental API, that would work for both developers and users. Maybe @dfawley can explain why grpc rejected that approach, or why they don't use nested modules when the entire package is experimental.

It's possible that etcd was unaware that naming was experimental when they chose to bake it into their own API, but it seems unlikely. When etcd introduced their API in January 2017, the entire overview section of the grpc package doc read "Package naming defines the naming API and related data structures for gRPC. The interface is EXPERIMENTAL and may be su[b]ject to change." I don't think I could have used that package to build correct software without reading its documentation, and I don't think I could have missed that all-caps EXPERIMENTAL when I did. It's possible that engineers with other workflows, for example an IDE that pops up the doc for a specific symbol, might never have seen the package doc, but it still a stretch to think that none of etcd's developers noticed this in over three years. A more likely explanation is that they discounted the experimental nature of the API because they wanted to add a feature to their own product, and knew from experience, as this comment explains, that experimental grpc features can stay around for years (as this one in fact did) and often become stable.

If I'm right, then nothing proposed here would have helped etcd avoid an experimental API, because they already knew they were using one.

The real victims here are etcd's users. Nothing in the etcd doc (linked in bullet point 2 above) indicates that its API relies on an unstable API and so is itself unstable.

Could a Deprecated: comment on the experimental package have helped? pkg.go.dev doesn't yet propagate that marker, but even if it did, it would not have been good enough. When it was introduced in July 2016, grpc's naming package was an experiment with a chance of success, not yet a failed API. It should not have been marked as deprecated. That didn't happen until almost two years later, in May 2018. Users of the etcd package would have had 17 warning-free months to incorporate the etcd API in their own systems and move on to something else, never thinking to re-check the etcd docs.

What if grpc had named their package experimentalnaming instead? By hypothesis, that wouldn't have helped etcd because they knew the package was experimental already, but would it have helped their users? If etcd engineers had done nothing, then yes: that package name would have appeared in etcd's documentation. But if they had used an import identifier, as in

import naming "google.golang.org/grpc/experimentalnaming"

then the docs would show naming and the experimental nature of the package would be hidden. As this demo package shows, godoc uses import identifiers and alias names in place of the original names. For imports, doing so is necessary to avoid ambiguity, since two imported packages might have the same name. For type aliases, it makes sense to show the name that the developer explicitly chose for the type. So changing godoc's behavior to avoid these sorts of hiding wouldn't be a good idea.

I don't mean to imply that etcd would have done this with intent to deceive. They might reasonably have wanted to shorten references in the code. It is a deliberate act, however, so it provides an opportunity for a developer to think about what they are doing.

Reading the etcd package's source would reveal the "experimental," but it's perfectly possible to use a package successfully without ever reading its code. Certainly there's rarely a need to read import statements.

My conclusions:

The people most in need of help here are the downstream users of the experimental feature, not the immediate users.
If downstream users are unlikely to read the code of the packages they depend on, or even the doc of the twice-removed packages, then explicit naming isn't always going to help them.
Any solution should propagate, so the immediate importer of an experimental feature is marked as experimental to its own users.

jba on 5 Dec 2020

👍6

A bit more on what would happen if an experimental package had "experimental" in its name, like the hypothetical google.golang.org/grpc/experimentalnaming. As I discussed above, a package p importing that and including some of its symbols in its own API would have "experimental" in its code, but not in its godoc if the import was renamed.

But what about a package q importing p? Wouldn't the full path of the experimental package show up in the import declarations of one ofq's files?

Not necessarily. To be concrete, say p declares func Resolve() experimentalnaming.Watcher. If q's code looks like

w := p.Resolve()

then it never mentions the experimental package by name, so the import declarations at the top of the file would not include it.

What about the go.mod file? Two problems there: it lists modules, not packages, and the module as a whole might not be experimental; and it doesn't always show indirect dependencies, which is what the module containing experimentalnaming is to q.

The experimental package _would_ appear in the list of all packages, which you can get by running go list -deps. So one solution that could work with existing Go tooling is:

Every experimental package should have "experimental" in its path.
go list -deps should be be run on every PR, and should post new experimental packages to the PR.

That will call reviewers' attention to any new experimental package, whether or not its API is exposed.

Step 2, of course, involves significant tooling, just not in the Go toolchain.

This also doesn't handle the case of experimental API in an otherwise non-experimental package. Sometimes that's necessary because the new API needs access to unexported identifiers.

jba on 7 Dec 2020

👍2

I'd like to better understand their reluctance to use major versioning.

The gRPC team's goal is to never release new major versions unless there are very compelling reasons. The rationale behind this is that major version upgrades are often major inconveniences for users, and we do not wish to cause any friction that would prevent users from upgrading. Major releases lead to fragmentation and cause their own compatibility problems.

Would things really be much better if etcd had an API based on gRPC v1.2 (and they were continuing to use a 4+ year old version with no security updates/etc) when gRPC v5.3 was out? What would etcd even do here? They need newer features from gRPC, yet they wouldn't be able to use them, because they would be stuck with our old experimental resolver API that they chose to standardize on.

Maybe @dfawley can explain why grpc rejected that approach, or why they don't use nested modules when the entire package is experimental.

Modules weren't even a thing when this package was created. Even the internal directory feature was still reasonably new. However, this strategy just doesn't work for anything that is not a separate package, as I mentioned in my most recent comment above. Also, versioning for an experimental module integrated into a non-experimental module seems tricky/impossible to get right.

knew from experience, as this comment explains, that experimental grpc features can stay around for years (as this one in fact did) and often become stable.

Now they know from experience that all-caps EXPERIMENTAL APIs can be deleted.

"Backward compatibility", in the absence of documentation, is impossible to attain. Otherwise, your "bug fix" is my "behavior change".

Documentation must be considered part of the API and part of the Go backward-compatibility promise.

The feature request here is to codify a documentation convention so that tooling can surface it more noticeably to users (and indirect/transitive users).

dfawley on 7 Dec 2020

👍2

Would things really be much better if etcd had an API based on gRPC v1.2 (and they were continuing to use a 4+ year old version with no security updates/etc) when gRPC v5.3 was out? What would etcd even do here? They need newer features from gRPC, yet they wouldn't be able to use them, because they would be stuck with our old experimental resolver API that they chose to standardize on.

I'm going to claim that yes, things would be better in that scenario. They certainly would not be perfect, as you describe. But it would mean that users of etcd would not be broken by changes to gRPC.

ianlancetaylor on 7 Dec 2020

Yes, it would be slightly better. I'm a user of both etcd and grpc, and if this would have been the case, my projects could just have imported grpc in two different major versions. An old one for etcd and a new one for grpc.
But is this desirable? And what if I depend on a third library that also needs grpc, in yet another major version?
This might happen if grpc needs a lot of "experimentation" for it's experimental API. But also if there are multiple independent experiments in the same module. They might need to make several major version bumps when changing their unstable api, fragmenting their userbase.

Though, an "experimental" tag would only increase the visibility. It wouldn't keep etcd from using such an API voluntarily. And if such a tag doesn't propagate, user's of etcd wont see the problem with the library they're importing.

maja42 on 7 Dec 2020

👍1

Nobody has any perfect answers here, at least not so far. The choices are "bad" and "less bad".

ianlancetaylor on 7 Dec 2020

❤1

Would things really be much better if etcd had an API based on gRPC v1.2 (and they were continuing to use a 4+ year old version with no security updates/etc) when gRPC v5.3 was out? What would etcd even do here? They need newer features from gRPC, yet they wouldn't be able to use them, because they would be stuck with our old experimental resolver API that they chose to standardize on.

Why would an API compatible with gRPC v1.2 necessarily imply an _implementation_ older than v5.3?

I think the protobuf migration is a good example here: the old _API_ (github.com/golang/protobuf) was rewritten to use the new _implementation_ (google.golang.org/protobuf), so even users on the old API continue to get bug fixes and performance improvements. That does require more care and effort when designing both the new API and the migration path, but that extra effort pays substantial dividends for downstream users.

bcmills on 7 Dec 2020

@maja42

Though, an "experimental" tag would only increase the visibility. It wouldn't keep etcd from using such an API voluntarily. And if such a tag doesn't propagate, user's of etcd wont see the problem with the library they're importing.

An experimental build constraint would at least keep etcd from building (or keep the features that depend on that tag from building) when the tag is not set.

It's true that build tags used in that way become viral, in that anyone who wants to use an API guarded by the tag _also_ has to set the tag, but that's an intrinsic property of unstable APIs: depending on an unstable API _is necessarily viral_.

bcmills on 7 Dec 2020

I'm going to claim that yes, things would be better in that scenario. They certainly would not be perfect, as you describe. But it would mean that users of etcd would not be broken by changes to gRPC.

That's fair, but etcd users also wouldn't have been broken by changes to gRPC if the etcd authors had read and abided by the gRPC godocs, which very clearly labeled this package as unreliable. This is where things went wrong, and what this FR is hoping can be prevented in the future. You had asked:

I'm wondering how an "unstable" comment is a better solution.
What is an example of a problem that would be avoided by this comment convention?

...and the above was my example for you.

dfawley on 8 Dec 2020

@dfawley How would an explicit "unstable" comment convention be any better than the comment that was already there? That is, you have a clear example that relying on comments doesn't work. But "unstable", as proposed here, is just another comment.

ianlancetaylor on 8 Dec 2020

@ianlancetaylor My answer to your question is that "unstable" can be propagated to downstream users by tools that recognize the convention.

It's important to be clear what you mean by "work" in "relying on comments doesn't work." If you mean "prevent etcd from relying on experimental API," I've argued that nothing would work since etcd made a deliberate choice to use an API they knew was experimental. The best we can do is warn downstream users that they are indirectly consuming experimental API, and a machine-readable convention can help with that.

jba on 8 Dec 2020

👍1

Using auto completion in editors or cut'n'paste from web sites people end up depending on APIs without ever actually reading the documentation for them. An experimental tag that is tool readable could both be used to downrank experimental completions as well as warn users when they start depending on experimental features.
I personally still lean towards using build tags as the right technology to hide experimental APIs, but tool support in general for build tags is not ergonomic right now.

ianthehat on 8 Dec 2020

👍1

I'd strongly encourage naming experimental APIs in a fashion that clearly conveys the experimental status, exactly to make it the experimental nature clear in contextless code snippets.

ExperimentalFoo or experimental_package.Foo is clearly and unambiguously experimental without depending on tool support. If and when the experimental feature stabilizes, a stable name can be added and the experimental one removed after a delay for migration.

neild on 8 Dec 2020

😕1

@neild Names don't necessarily propagate, and propagation is the harder problem. Please see my comment above, the paragraph starting "What if grpc had named their package experimentalnaming instead?"

jba on 8 Dec 2020

👍1

I really like the idea of have this comment based vs symbol based. If I created an experimental API and that surface turned out to work just as well as intended I think it is great for users that they don't have to make any code changes to continue using the feature. Whereas if I had named my function ExperimentalFoo and stabilized it all users of the api will need to update their code when they upgrade the dependency.

codyoss on 8 Dec 2020

👍2

If we feel that propagation (read: virality) is important, the best option we have today is build tags. Sure, this will require some tooling support, like js, so that symbols will be surfaced even if you do not provide the correct tags, but the alternative is that we have to do a lot of tooling support to ensure that a symbol prefix, or comment propagates through code paths. I would rather trust the compiler for this.

I also personally prefer an exp flag for this, as it is easy to use and stabilisation is trivial: just remove the flag.

EDIT: Since only the comments of this ticket talk to the build flag approach, feel free to review #35704 for my proposal.

carnott-snap on 8 Dec 2020

👍1

@neild Naming does not work, because there are lots of ways that function and type names are hidden from you in normal code, even without transitive dependancies being considered. If we made the naming an actual convention that tools can detect, it might be useful, otherwise it is barely better than the plain documentation form. This also comes into play if you start talking about gorelease, which is supposed to tell you if you are making breaking changes, which would also need to make a decision about experimental APIs.
Right now, without any extra tooling, build tags are the only sensible way to have an experimental API on your main tagged branch. I do think we could keep doing that but pushing it further with better tooling around build tags.

@codyoss I don't think the experimental naming thing is a big hurdle as you stabilize, you can leave the experimental symbol there aliasing the now real symbol anyway, and ideally we will have good migration tools for this kind of thing. Keeping the name different also has some advantages (eg try out two signatures at the same time as different experiment names and pick a winner based on usage). One real downside is if you are trying to see if an API is truly ergonomic, which it might not be with a long experimental name.

ianthehat on 8 Dec 2020

👍1

If there is a tool that understands the concept of "unstable", would all APIs under a v0 module automatically be considered unstable since it not stable according to semver?

Major version zero (0.y.z) is for initial development. Anything MAY change at any time. The public API SHOULD NOT be considered stable.

Since a vast number of modules are still at v0, wouldn't this functionally make nearly every module essentially "unstable" since it depends transitively on a v0 module somewhere?

dsnet on 8 Dec 2020

That is not a consideration that I put into this proposal, but you make a good claim: one should not not rely upon 0.x.z symbols any more than the naming package. Unfortunately, that nuance is going to be really hard to tease out of older (non module) packages, and probably not worth the hassle, but if we do implement virality via godoc tooling, it could be a (relatively) easy extension.

carnott-snap on 9 Dec 2020

"unstable", as proposed here, is just another comment.

As others have said, the difference is visibility and propagation. If there is a standard, tooling can detect it, which has value.

There are other options, which may be worth fleshing out.

review #35704 for my proposal.

IMO there needs to be granularity if a build tag is used. Otherwise using one experimental/unstable feature for your own purposes opens the door to all unstable features used by anyone.

One aspect the proposal here may not fully appreciate about this situation is that the "poisoning" of using an unstable API from a foreign package happens at the package level. I.e. if the unstable API is removed, the entire package which references it breaks, meaning a transitive declaration needs to happen at the package level (or conditional compilation is required).

There are also module-level scope considerations. If I call an unstable function in another package within the same module, that shouldn't make the package that uses it unstable, since, as the maintainer of the module, I can ensure it always continues to work, even if the unstable function is removed. However, if the unstable function in the other package is unstable because it calls an unstable API from a foreign module, then it would be transitive.

Since a vast number of modules are still at v0, wouldn't this functionally make nearly every module essentially "unstable" since it depends transitively on a v0 module somewhere?

This is a tangent, but this is a problem that has concerned me for some time. We should really get v1 releases for at least the Go-owned packages that everyone relies upon, e.g. "x": https://github.com/golang/sys/releases

dfawley on 9 Dec 2020

IMO there needs to be granularity if a build tag is used. Otherwise using one experimental/unstable feature for your own purposes opens the door to all unstable features used by anyone.

I actually see this as a feature. Presumably tooling, godoc/pkgsite, could highlight any experimental tooling, but if you look to the rust community, they have a thriving nightly compiler that allows people to test out and prototype pre-release libraries. This even involves alpha libraries, using other alpha libraries in addition to alpha language features. It does lock people into this ecosystem until all the features they need are stabilised, but it is clear that you are running experimental. Back in Go, you could even write an adaptor for the interface you want and tag that +build !exp, thus maintaining compatibility.

Are you suggesting a convention like exp_xxx, where xxx is my module/pkg/scope? Looking through the standard library there seem to be three groups of tags. System level tags: GOOS/GOARCH. Short names: ignore libfuzzer gofuzz notacomment toolate testgo notmytag abc foo-bar tools missingtag tag1 tag2 exclude a b extra i tagtest buggy. Fully qualified names: disabled_see_issue_18589 goexperiment.staticlockranking nethttpomithttp2 cmd_go_bootstrap goexperiment.fieldtrack goexperiment.framepointer. The first is unrelated, the second fall victim to name squatting and collisions, and the third are really verbose.

One aspect the proposal here may not fully appreciate about this situation is that the "poisoning" of using an unstable API from a foreign package happens at the _package_ level. I.e. if the unstable API is removed, the _entire package_ which references it breaks, meaning a transitive declaration needs to happen at the _package_ level (or conditional compilation is required).

IMO, this is actually something that build tags solve. If we hide all the experimental code behind +build exp, removing an API only breaks when you run go build -tags exp, otherwise that whole file is ignored.

There are also module-level scope considerations. If I call an unstable function in another package within the same module, that shouldn't make the package that uses it unstable, since, as the maintainer of the module, I can ensure it always continues to work, even if the unstable function is removed. However, if the unstable function in the other package is unstable because it calls an unstable API from a foreign module, then it would be transitive.

This smells like an opportunity to use /internal/ and re-export symbols experimentally. E.g. path/to/pkg/internal.Foo is stable, but not publicly accessible, where as the type aliased path/to/pkg.Foo (or path/to/experimentalpkg.Foo, or path/to/pkg.ExperimentalFoo) is dangerous. Regardless of the experimental stamping mechanism, type aliasing should work around the root problem.

This is a tangent, but this is a problem that has concerned me for some time. We should really get v1 releases for at least the Go-owned packages that everyone relies upon, e.g. "x": https://github.com/golang/sys/releases

Yeah, tagging the extended standard library has been a long time coming, see #21324. That being said, there is a graceful way to encourage this: we could define a Go version, say 1.17, after which v0.x.y modules are treated as unstable by the toolchain/godoc/pkgsite/etc. This would balance the pain of treating all historical code as experimental; however, I agree it is a tangent and am happy to fork off a ticket to track it separately.

carnott-snap on 9 Dec 2020

👍1

@jba I'm not seeing how an "unstable" comment propagates in a way that is actually useful. Can you give an example? Thanks.

ianlancetaylor on 9 Dec 2020

@dsnet

If there is a tool that understands the concept of "unstable", would all APIs under a v0 module automatically be considered unstable since it not stable according to semver?

Sort of? But you can still have a stable API even if the implementation is unstable, as long as you promise that you will respond to breaking changes in your dependencies by reimplementing the functionality without changing your own exported API.

(Note that you _cannot_ fix a breaking change in that manner if your API directly returns or accepts an unstable type.)

But tools today can already use go list to identify packages loaded from v0 modules, and can (theoretically) propagate information about which types are unstable and which packages transitively import unstable dependencies. And downstream consumers of your module will see v0 modules in their own go.sum files, so at least the existence of the unstable dependency is somewhat visible.

Since a vast number of modules are still at v0, wouldn't this functionally make nearly every module essentially "unstable" since it depends transitively on a v0 module somewhere?

Yes. Nearly every module _really is_ unstable in that sense, especially given #21324.

(This check is called out explicitly as the second bullet-point in #26420.)

bcmills on 9 Dec 2020

👀1

@ianlancetaylor Imagine that the experimental grpc package had a tool-aware "unstable" comment, and etcd did exactly what they did, using that package without any mention of its being experimental. Now someone comes along and wants to use the etcd package. They look at its documentation and see an indication that the package imports an unstable package. Perhaps when they look at the doc for func Resolve() naming.Watcher, they see naming.Watcher in red, with a tooltip that says it's unstable. If they miss the doc but they run go vet on their own package that consumes etcd's, then they see a message about using an unstable API.

How is any of that useful? That user has information to make a decision. They may go ahead anyway, but avoid releasing a v1 version of their product. Or they may look around for an alternative, stable implementation of the same functionality. Or they may choose to ignore the problem, but at least _their_ importers will have that information.

jba on 9 Dec 2020

👍1

@jba It sounds like you are saying this:

someone goes to look at etcd in pkg.go.dev
pkg.go.dev will look at the etcd source code
it will look at each case where an imported name appears in the exported API
it will look at the packages that provide those names
if any of those names are marked "unstable" in the providing package, it will add a highlight for the name

Does that sound right?

How deep should it look? If a package exports a type with methods, some of those methods can refer to unstable APIs. Should any use of that type be considered unstable? Is this a plausible check to run?

I don't think we could do anything similar in "go vet", except perhaps as an optional check that people would have to opt into.

ianlancetaylor on 9 Dec 2020

@ianlancetaylor That's about right, but you make it sound like we'd do all that at serving time. When we process a module M (in the background), we'd check our DB for unstable packages on its build list and store that information with M. We'd also have to go in the other direction, updating modules that depended on M. But that will be a very small number because we check the index every minute or so, so not too many modules can have downloaded M in that time.

If a package exports a type with methods, some of those methods can refer to unstable APIs. Should any use of that type be considered unstable?

I think I'm convinced that instability is always a property of packages, as argued above. So yes. But that's not really a question that an informational tool like pkg.go.dev has to answer. We just have to point out that the package you're looking at depends on unstable packages or symbols, with as fine a grain as we think helpful and can reasonably compute, and let you decide what that means for your project.

jba on 10 Dec 2020

I want to call out that this seems like a lot of work, and we have to do it for pkgsite, godoc, and gorelease, plus the community has to implement (or integrate) it for every ide/linter.

I would also like to call out that even grpc has non-package level instability: e.g. google.golang.org/grpc.UnaryClientInterceptor.

carnott-snap on 10 Dec 2020

👍1

I think the tooling work is also an argument in favor of using a build tag. We _already_ need to do some work to support common build tags (GOOS and GOARCH), and generalizing that to an unsafe or experimental build tag seems like a lot less work overall than building out a lot of tooling to support another orthogonal mechanism.

bcmills on 10 Dec 2020

👍1

I think the tooling work is also an argument in favor of using a build tag.

@bcmills How does pkg.go.dev handle methods/functions that only appear in a file with a build tag today?

codyoss on 10 Dec 2020

@codyoss, it doesn't (see #37232). But it should, and it seems easier to solve one problem (surfacing tag-constrained APIs) than two problems (surfacing tag-constrained APIs and, separately, surfacing unstable APIs).

bcmills on 10 Dec 2020

I just wanted to throw in my 2 cents from what is being done in some of the cloud client library packages today.

We do method level experimentation. I can only think of one package where this happens today, examples:
We do package level experimentation. The best example I can think of this is non-GA apis. We add a small warning to the package level docs as protos the clients are being generated from have not hardened yet.
- asset/apiv1beta1
We provide an externalized internal package. This is needed so packages in another repository we own can use the code, but only those packages should: internaloption

I will note that we do float a v0.X.0 so this is not a huge issue for us, but even so we try our best to treat the code as if it were a v1.X.X.

I believe that experimentation is vital for any long tail project that wants to keep evolving. Excited to see how this and some of the other mentioned proposals evolve.

codyoss on 10 Dec 2020

I was envisioning it very differently, I would make a simple checker that complains if you reference an unstable symbol from a stable one.
This makes unstable viral, and means you need no other tooling, the authors will be forced to surface the instabilit. I think this is a fairly easy checker to write, very cheap to run, and if integrated into vet, would mean no other tools need to change at all. It would also be very easy to try it out as a simple experiment if anyone wants to do so.
I still don't think we should do this, but I don't think complexity of tooling would be an argument against it.

ianthehat on 10 Dec 2020

it doesn't (see #37232). But it should, and it seems easier to solve one problem (surfacing tag-constrained APIs) than two problems (surfacing tag-constrained APIs and, separately, surfacing unstable APIs).

@bcmills That issue seems somewhat related, but should evolve to include custom constraints if it is to be considered a viable alternative. I think an option like this seems fine as long as tooling like auto-complete and godoc support it is well.

codyoss on 10 Dec 2020

👍1

@ianthehat What forces the authors to surface the instability?

jba on 10 Dec 2020

Go: proposal: cmd/doc: add "// Unstable:" prefix convention

What did you see today?

What would you like to see?

What did not work?

Most helpful comment

All 84 comments

Consumption @rsc

Automation

Stabilisation @bcmills

Migration

Application

Comment

Consumption

Automation

Stabilisation

Migration

Application

Example

gRPC

stdlib

Git tag

Consumption

Automation

Stabilisation

Migration

Application

Example

Prefix

Consumption

Automation

Stabilisation

Migration

Application

Example

gRPC

stdlib

Build tag

Consumption

Automation

Stabilisation

Migration

Application

Example

gRPC

stdlib

Reference example

gRPC

stdlib

Related issues