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: cmd/doc: add "// Unstable:" prefix convention #34409

Open
arnottcr opened this issue Sep 19, 2019 · 84 comments
Open

proposal: cmd/doc: add "// Unstable:" prefix convention #34409

arnottcr opened this issue Sep 19, 2019 · 84 comments

Comments

@arnottcr
Copy link
Contributor

@arnottcr arnottcr commented Sep 19, 2019

What did you see today?

Many projects have different stability guarantees for the exported symbols in a package. Others rely on generated code that cannot, or will not, give stability guarantees. As such, most authors will document this pre-release or instability in doc strings, but the syntax and conventions are all over the place.

The standard library likes to reference the Go compatibility promise or Go 1 compatibility guidelines, since it is bound by them, however these do not work well for community packages. Other terms like non-portable and EXPERIMENTAL are descriptive and well explained in unsafe and syscall/js respectively.

Some community libraries have used terms like // Alpha: ..., // Beta: ..., and // Unstable: ..., which work as well. There could be an argument for not releasing pre-release features on a stable branch, but other times like with the proto client/server interfaces, instability is a guarantee that must be worked around.

What would you like to see?

Similar to // Deprecated: ..., I would like to see the stabilization of supported comment tags for unstable symbols.

They support the same three formats that same three formats that deprecation has.

These tags should also allow such symbols to be excluded from the gorelease tool.

A single tag should be sufficient:

// Unstable: ...

When interacting with released, finalized symbol that cannot or will not be stabilized, the description can provide stability workarounds, alternatives, or what guarantees should be expected.

When interacting with pre-release features, a proposed timeline can be given or alternatives for customers requiring stability guarantees.

What did not work?

The // Alpha: ... and // Beta: ... options looked promising, since they would only be used for temporary instability as part of the release process. The two terms overload one another (what is the difference between alpha, beta, and // PreRelease: ...?), leading to confusion. Furthermore, the programmatic benefits of knowing an API will stabilize in a future release is not that useful, "is it unstable now?" is more important.

The // Experimental: ... syntax used by the standard library implies the notion that the feature will eventually be stabilized. This further overloads it with alpha, beta, etc. and does not fit the needs of the above gRPC interfaces.

The // NonPortable: ... syntax is too domain specific to unsafe to make sense for purely semantic changes to packages. It makes sense for unsafe, but does not generalize

@gopherbot gopherbot added this to the Proposal milestone Sep 19, 2019
arnottcr added a commit to arnottcr/protobuf that referenced this issue Sep 28, 2019
Due to the lack of stability guarantees provided for service Server and
Client interfaces, as hightlighted in #2318 and #3024, this change adds
a godoc line that both warns users and suggests the canonical workaround.

The structure of the godoc text is styled after an open proposal,
golang/go#34409, thus may need to be changed, but attempts to play well
with the tooling ecosystem, as was done for deprecated with
golang/go#10909.
arnottcr added a commit to arnottcr/protobuf that referenced this issue Sep 28, 2019
Due to the lack of stability guarantees provided for service Server and
Client interfaces, as hightlighted in grpc/grpc-go#2318 and
grpc/grpc-go#3024, this change adds a godoc line that both warns users
and suggests the canonical workaround.

The structure of the godoc text is styled after an open proposal,
golang/go#34409, thus may need to be changed, but attempts to play well
with the tooling ecosystem, as was done for deprecated with
golang/go#10909.
arnottcr added a commit to arnottcr/protobuf that referenced this issue Sep 28, 2019
Due to the lack of stability guarantees provided for service Server and
Client interfaces, as hightlighted in grpc/grpc-go#2318 and
grpc/grpc-go#3024, this change adds a godoc line that both warns users
and suggests the canonical workaround.

The structure of the godoc text is styled after an open proposal,
golang/go#34409, thus may need to be changed, but attempts to play well
with the tooling ecosystem, as was done for deprecated with
golang/go#10909.
@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented Oct 2, 2019

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.

@carnott-snap
Copy link

@carnott-snap carnott-snap commented Nov 9, 2019

What are next steps for getting approval for this proposal?

@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented Nov 11, 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.

@carnott-snap
Copy link

@carnott-snap carnott-snap commented Nov 12, 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.

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:

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.

@rsc rsc changed the title proposal: stability documentation conventions proposal: cmd/doc: stability documentation conventions Nov 13, 2019
@rsc rsc changed the title proposal: cmd/doc: stability documentation conventions proposal: cmd/doc: add "// Unstable:" prefix convention Nov 13, 2019
@rsc
Copy link
Contributor

@rsc rsc commented Nov 13, 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.)

@bcmills
Copy link
Member

@bcmills bcmills commented Nov 13, 2019

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).
@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented Nov 13, 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
Copy link
Contributor

@jayconrod jayconrod commented Nov 13, 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?

@dsnet
Copy link
Member

@dsnet dsnet commented Nov 13, 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.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Nov 13, 2019

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

@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented Nov 13, 2019

@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.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Nov 13, 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/ .

@carnott-snap
Copy link

@carnott-snap carnott-snap commented Nov 14, 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/moby@v1.13.1/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.
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Nov 14, 2019

I didn't mean anything special about the exact word Experimental. I just meant, as in #34409 (comment), 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.

@bcmills
Copy link
Member

@bcmills bcmills commented Nov 14, 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.)

@cristaloleg
Copy link

@cristaloleg cristaloleg commented Nov 14, 2019

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.

@carnott-snap
Copy link

@carnott-snap carnott-snap commented Nov 14, 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)

  1. 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
  2. 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.
  3. 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.
@rsc
Copy link
Contributor

@rsc rsc commented Nov 20, 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.

@carnott-snap
Copy link

@carnott-snap carnott-snap commented Nov 20, 2019

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.

@maja42
Copy link

@maja42 maja42 commented Dec 7, 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.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Dec 7, 2020

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

@bcmills
Copy link
Member

@bcmills bcmills commented Dec 7, 2020

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
Copy link
Member

@bcmills bcmills commented Dec 7, 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.

@dfawley
Copy link

@dfawley dfawley commented Dec 8, 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.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Dec 8, 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.

@jba
Copy link
Contributor

@jba jba commented Dec 8, 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.

@ianthehat
Copy link

@ianthehat ianthehat commented Dec 8, 2020

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.

@neild
Copy link
Contributor

@neild neild commented Dec 8, 2020

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.

@jba
Copy link
Contributor

@jba jba commented Dec 8, 2020

@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?"

@codyoss
Copy link
Member

@codyoss codyoss commented Dec 8, 2020

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.

@carnott-snap
Copy link

@carnott-snap carnott-snap commented Dec 8, 2020

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.

@ianthehat
Copy link

@ianthehat ianthehat commented Dec 8, 2020

@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.

@dsnet
Copy link
Member

@dsnet dsnet commented Dec 8, 2020

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?

@carnott-snap
Copy link

@carnott-snap carnott-snap commented Dec 8, 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.

@dfawley
Copy link

@dfawley dfawley commented Dec 8, 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

@carnott-snap
Copy link

@carnott-snap carnott-snap commented Dec 9, 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.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Dec 9, 2020

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

@bcmills
Copy link
Member

@bcmills bcmills commented Dec 9, 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.)

@jba
Copy link
Contributor

@jba jba commented Dec 9, 2020

@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.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Dec 9, 2020

@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.

@jba
Copy link
Contributor

@jba jba commented Dec 10, 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.

@carnott-snap
Copy link

@carnott-snap carnott-snap commented Dec 10, 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.

@bcmills
Copy link
Member

@bcmills bcmills commented Dec 10, 2020

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.

@codyoss
Copy link
Member

@codyoss codyoss commented Dec 10, 2020

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?

@bcmills
Copy link
Member

@bcmills bcmills commented Dec 10, 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).

@codyoss
Copy link
Member

@codyoss codyoss commented Dec 10, 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.
  • 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.

@ianthehat
Copy link

@ianthehat ianthehat commented Dec 10, 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.

@codyoss
Copy link
Member

@codyoss codyoss commented Dec 10, 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.

@jba
Copy link
Contributor

@jba jba commented Dec 10, 2020

@ianthehat What forces the authors to surface the instability?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.