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: Go 2: add mechanism to remove API as a function of caller's go.mod "go 1.xx" version #30639

Open
bradfitz opened this issue Mar 6, 2019 · 36 comments
Labels
Milestone

Comments

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Mar 6, 2019

When reviewing a number of the Go2 issues, we keep finding that we're basically unable to clean up past mistakes.

On the language side, the go.mod file's "go 1.xxx" declaration lets users declare their expected Go language semantics, which lets us remove Go language features over time, but we have nothing equivalent for removing standard library symbols, short of simply making new major versions of all packages, which gets contagiously invasive very quickly with the dependencies between the std packages. For redesigns, new major versions works, but it doesn't work well for cleanups.

It would be nice to have a mechanism that's similar (but different) to +build go1.x build tags, but are added implicitly by the caller module module's expected language version.

That'd let a package have different exports depending on who was using it.

Rather than propose an exact solution, this bug is more generally about tracking how (and whether) we can remove things from a package over time.

/cc @ianlancetaylor @griesemer @bcmills

@gopherbot gopherbot added this to the Proposal milestone Mar 6, 2019
@bcmills
Copy link
Member

@bcmills bcmills commented Mar 6, 2019

I don't think this is a problem that actually needs to be solved for package-level identifiers (types, constants, and functions). For those, we still have to provide an implementation anyway, and given that we have that implementation, we may as well just add a // Deprecated comment, and make lint and godoc warn everyone not to use it.

The interesting problem, then, is how to remove methods, because they aren't necessarily visible at the point where they are used.

Suppose I have:

package mine

type Thing struct{ […] }

// Deprecated: use NewThing instead.
func (t *Thing) OldThing() {}

func (t *Thing) NewThing() {}
package mediator

import (
	[…]
)

func […] {
	// This looks like it avoids the deprecated OldThing method...
	var t theirs.NewThinger = new(mine.Thing)
	theirs.Oops(t)
}
package theirs

type NewThinger interface { NewThing() }

func Oops(t NewThinger) {
	if x, ok := t.(interface { OldThing() }); ok {
		// ...but we would need whole-program analysis
		// to realize that OldThing is still in use.
		x.OldThing()
	} else {
		t.NewThing()
	}
}
@bcmills
Copy link
Member

@bcmills bcmills commented Mar 6, 2019

I have some ideas about how to address this, but I don't have the time to write them up this week. 😕

@ianlancetaylor ianlancetaylor changed the title proposal: add mechanism to remove API as a function of caller's go.mod "go 1.xx" version proposal: Go 2: add mechanism to remove API as a function of caller's go.mod "go 1.xx" version Mar 26, 2019
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Apr 16, 2019

@bcmills Any ideas on mechanism? Thanks.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented May 7, 2019

If we use our standard fallback of a magic comment, then it could look like:

//go:lang < 1.14
type ClientConn struct {
}

This means that ClientConn is normally exported, but it can not be seen when compiling at Go language level 1.14 or higher. This can also apply to struct fields.

type Request struct {
    ...
    //go:lang < 1.14
    Cancel <-chan struct{}
    ...
}

The type Request is visible at all language levels, but it can not be seen at language level 1.14 or higher. (The struct field would still exist at run time and would still show up when using the reflect package.)

The magic comment could be used with package scope types, consts, variables, functions, and methods, and also with fields of struct types and methods of interface types.

It could be used to cover an entire set of types, consts, or variables.

//go:lang < 1.14
const (
    SEEK_SET int = 0
    SEEK_CUR int = 1
    ...
)

In this example SEEK_SET and SEEK_CUR (and any other constants in the group) can not be seen at language level 1.14 or above.

@bcmills
Copy link
Member

@bcmills bcmills commented May 7, 2019

Yes, that's the thing that works for everything but methods — but it's also mostly redundant with // Deprecated comments.

The problem is, what happens if you do a type-assertion that checks for the removed method in a pre-1.14 module, and then pass the value as an interface{} to some other package that has since been upgraded? When the second package does the same type-assertion it will presumably fail, unless that method has somehow been marked as contaminated by the pre-1.14 package.

@bradfitz
Copy link
Contributor Author

@bradfitz bradfitz commented May 7, 2019

but it's also mostly redundant with // Deprecated comments.

Deprecated comments have no machine-readable structure, though. Parsing English sentences sounds super gross.

@bcmills
Copy link
Member

@bcmills bcmills commented May 7, 2019

Why is machine-readable structure important? Either way, the content is merely informational: it's nearly trivial to construct a “backdated” forwarding module to access the removed definitions.

@bradfitz
Copy link
Contributor Author

@bradfitz bradfitz commented May 7, 2019

Either way, the content is merely informational

That's not true. The whole point of this bug is to remove access to symbols from people. It's not to write English at them. We can already write English at them in comments.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented May 7, 2019

The problem is, what happens if you do a type-assertion that checks for the removed method in a pre-1.14 module, and then pass the value as an interface{} to some other package that has since been upgraded? When the second package does the same type-assertion it will presumably fail, unless that method has somehow been marked as contaminated by the pre-1.14 package.

The suggestion is that, if T.M is marked as removed in 1.14, then direct references to T.M will fail. And storing a value of type T in an interface that requires method M will fail. However, storing a value of type T in an interface, and then asserting to a different interface that checks for M will succeed. It may be that this inconsistency is enough to decide against this proposal, or at least to decide against permitting it to be used for methods, but I don't think there is any other plausible approach.

@bcmills
Copy link
Member

@bcmills bcmills commented May 8, 2019

The whole point of this bug is to remove access to symbols from people. It's not to write English at them.

The go directive doesn't tell us when the code was written, only which version it was written against. Given that, it seems like there is a fundamental tradeoff: we must either make breaking changes (presumably following a deprecation period), or use a mechanism that can be trivially defeated by a backdated intermediary.

Given that we don't want to take the breaking-change approach, I don't see why the defeat mechanism of “ignoring the linter” is all that much worse than “using a backdated trampoline module” — and the comment-and-lint approach has the advantage of already being in wide use.

@bcmills
Copy link
Member

@bcmills bcmills commented May 8, 2019

Thinking about the linting approach some more: we do have one interesting option, at least. We run vet for tests, but not lint — so perhaps we could adopt some sort of convention that promotes use of a // Deprecated: identifier from a mere lint warning to a stronger vet warning.

For example:
// Deprecated (go 1.14): […]

could be a lint warning if used in a pre-1.14 module, but a vet warning if used in a module with a later go version.

@bradfitz
Copy link
Contributor Author

@bradfitz bradfitz commented May 8, 2019

The go directive doesn't tell us when the code was written, only which version it was written against.

Yes, but that's fine. We wouldn't do this until modules were on by default for enough time that most the community has go.mod files with a go directive. So future symbol hiding wouldn't affect existing code; only new code, but new code not compiling isn't really a breaking change if it never worked in the first place.

For example:
// Deprecated (go 1.14): […]

We're talking about the language version, not the Go version, though, which that comment doesn't make clear, if that was your intention. That's why the directive Ian wrote above said //go:lang.

@bcmills
Copy link
Member

@bcmills bcmills commented May 8, 2019

So future symbol hiding wouldn't affect existing code; only new code, but new code not compiling isn't really a breaking change if it never worked in the first place.

Right; my point is that it also wouldn't affect new code written with a backdated go directive. (As long as the older language version continues to work, users can continue to write against it — and they can use packages written against it to defeat whatever symbol-removal we implement. In that sense, it's no more effective than a vet check, albeit a bit more annoying to hack around.)

@bradfitz
Copy link
Contributor Author

@bradfitz bradfitz commented May 8, 2019

Right; my point is that it also wouldn't affect new code written with a backdated go directive.

That's fine. That's the whole point, actually.

This mechanism says: "You want to to play with the new toys? (e.g. generics) Then first stop using the old deprecated stuff. You can't have both. Choose."

It's not about finding some fool-proof mechanism to stop people from accessing the symbols. (If so, we'd just delete them.) It's a casual nudge, and a way to let us remove godoc clutter. (godoc would stop showing these things, at least on golang.org's default view.)

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented May 8, 2019

Given that we don't want to take the breaking-change approach

The point of this proposal is to permit breaking changes, without actually breaking existing code. It's adapting the same mechanism that we've decided to use to permit breaking changes in the language.

@bcmills
Copy link
Member

@bcmills bcmills commented May 8, 2019

This mechanism says: "You want to to play with the new toys? (e.g. generics) Then first stop using the old deprecated stuff. You can't have both. Choose."

Right. The problem is that — unlike language changes — the API check doesn't actually make them choose. A library cannot provide a language feature by definition, but it can easily provide access to a deprecated/removed part of a regular library API.

If it's just a casual nudge either way, then it really seems like a refinement of the comment convention is the way to go — compiler support isn't needed. We can have vet warn about usage, and have go doc (and godoc.org and wherever else) hide the hard-deprecated parts, and so on, without actually removing the symbols at compile time.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented May 14, 2019

In order to have vet warn about usage of names that we want to remove, we need a machine parseable comment that clearly says the language versions that should and should not support the name. I don't think we can just use our loosely specified "Deprecated" convention for this. We could use go:lang as suggested above, or something else. Any other suggestions?

@dmitshur
Copy link
Member

@dmitshur dmitshur commented May 15, 2019

The go:lang examples above use exported fields/types. I think it'd be helpful to include their documentation to get a better sense of how the go:lang directive would fit in. Does it go on top or below the package comment? To use the httputil.ClientConn type as the example, would it be as follows?

// ClientConn is an artifact of Go's early HTTP implementation.
// It is low-level, old, and unused by Go's current HTTP stack.
// We should have deleted it before Go 1.
//
// Deprecated: Use Client or Transport in package net/http instead.
//go:lang < 1.14
type ClientConn struct { ... }

Or would the //go:lang < 1.14 go on top of the documentation?

The above is not compatible with the current godoc; it doesn't know to not include the //go directive in the documentation. But there is some precedent for this format for unexported identifiers, e.g., see here.

All of the go:lang examples above used < and a Go language version x.y. Is < included because there are expected to be other variants, or is it just for clarity? If the latter...

I don't think we can just use our loosely specified "Deprecated" convention for this.

I agree we can't just use it as it's currently defined:

To signal that an identifier should not be used, add a paragraph to its doc comment that begins with Deprecated: followed by some information about the deprecation, and a recommendation on what to use instead, if applicable.

But perhaps we can use it if it's modified. For example, the equivalent of //go:lang < 1.14 can be expressed as "Deprecated in 1.14: <human readable text>". A full example with ClientConn:

// ClientConn is an artifact of Go's early HTTP implementation.
// It is low-level, old, and unused by Go's current HTTP stack.
// We should have deleted it before Go 1.
//
// Deprecated in 1.14: Use Client or Transport in package net/http instead.
type ClientConn struct { ... }

There are existing tools that parse the current "Deprecated:" convention, so I suspect parsing "Deprecated in x.y:" may also be viable. Having just one place that to tell both humans and machines that an identifier is deprecated has the advantage that they can't fall out of sync.

/cc @FiloSottile @dominikh

Edit: Something I realized just now is that this proposed convention is about removing an identifier, something that may happen after it's been deprecated, not at the same time. An identifier may be deprecated in 1.8, and removed in 1.14. In that case, "Deprecated in 1.14:" phrasing won't work.

Maybe "Deprecated; removed in 1.14:" then. But that's not as clean.

@Xe
Copy link

@Xe Xe commented May 30, 2019

A thought: comments like //go:lang < 1.14 are more human-language agnostic than writing English at people. If you mandate that the deprecation comments be prose in English for field, method or other kinds of commentary, it basically precludes the use of non-English human-languages to write these comments.

@docmerlin
Copy link

@docmerlin docmerlin commented May 30, 2019

Can't you add a v2 folder for that package and just let modules handle it?

@bradfitz
Copy link
Contributor Author

@bradfitz bradfitz commented May 30, 2019

@docmerlin, the whole point of this proposal is to give us a lighter weight mechanism than the v2 hammer. Especially for the standard library, where v2 packages are proving to be very complicated.

@docmerlin
Copy link

@docmerlin docmerlin commented May 30, 2019

Thanks, @bradfitz

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 23, 2020

If we provide a machine readable comment, we don't need an expression. We just need to specify the Go version that will no longer provide the functionality:

//go:removedin 1.15
func F() { ... }
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 23, 2020

When compiling a program, the go tool can in principle know the language version of every module used in the program. If we let the comment, or whatever mechanism we use, affect the compiler, then we can pass a "minimally support version" to the compiler. In that case we don't need to compile the exported function/variable/type at all.

This seems like a reason to use this in the tool rather than just in vet.

We might have to always compile methods, even if they are marked. New packages wouldn't be able to refer to the methods directly, but perhaps they could via a type assertion. It's hard to see how to remove methods from the program while still keeping a consistent view of the type independent of language version of the modules in the program.

@prattmic
Copy link
Member

@prattmic prattmic commented Jun 24, 2020

It is not clear to me from the docs, is the module language version private to the module or subject to minimal version selection in the whole program (i.e., all modules are compiled with the minimal language version required by all packages)?

From the discussion it seems it is the former, in which case it does seem that methods must remain. e.g., if a standard library type removes a method, but it is then accessed from a different module with a different version it could still be accessed via an interface.

If it is the latter, then I wonder if keeping a consistent view of the type is something we need to require?

There, this feature looks almost equivalent to syntactic sugar for redefining the entire type without removed fields/methods in a // +build go1.1n file. With the primary difference that this is based on the minimum version selection from go.mod rather than the toolchain version as used in the build tag. Since the whole program has the same view of the type, I'm not seeing where we'd run into problems.

@networkimprov
Copy link

@networkimprov networkimprov commented Jun 24, 2020

See #28221. IIRC, the module go directive applies only to the Go language spec, not the stdlib, runtime, compiler, etc.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 25, 2020

@prattmic The language version that appears in the go.mod file is specific to the module.

Yes, this feature is similar to using a build tag with the Go release version. The difference is that the comment name is available for modules using older language versions, but not for modules using newer ones.

@danp
Copy link
Contributor

@danp danp commented Jun 25, 2020

Checking: does this mean that a name can only ever be used once, across all versions?

Would this ever be valid (or desired)?

package http

// Server is the completely overhauled net/http.Server.
type Server struct { ... }

// Server is the original net/http.Server.
//go:removedin 1.15
type Server struct { ... }

This issue is mainly about removing/hiding things, but the description does say "let a package have different exports depending on who was using it." So perhaps there should be consideration for adding new things with previously removed/hidden names?

@bcmills
Copy link
Member

@bcmills bcmills commented Jun 25, 2020

@danp, see https://golang.org/design/28221-go2-transitions#language-redefinitions:

I think the only feasible safe approach is to not permit language redefinitions.

@networkimprov
Copy link

@networkimprov networkimprov commented Jun 25, 2020

@bcmills, I think Dan is asking about stdlib redefinitions. The doc you linked pertains to the language spec.

@bcmills
Copy link
Member

@bcmills bcmills commented Jun 25, 2020

The same reasoning applies to both. If we are going to break the behavior of an existing program, we should surface that break as an error at compile time, not as unexpected behavior at run time.

@networkimprov
Copy link

@networkimprov networkimprov commented Jun 25, 2020

But there are a lot more public symbols in the stdlib than keywords & operators in the language spec. And most programs invoke a small fraction of the former. Reserving all of them for all time might be detrimental.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 25, 2020

Backward compatibility for a package means that we are in effect reserving any given name for as long as that version of the package exists. This doesn't seem so terrible to me; there are an infinite number of names available. If this becomes burdensome, it's likely time to start thinking about a v2 of the package.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Nov 10, 2020

New syntax suggestion mostly from @bradfitz. Use a //go:hide comment that names the function or type or variable or constant or method being hidden.

For example:

//go:hide Function 1.15
//go:hide Type 1.15
//go:hide Constant 1.15
//go:hide Var 1.15
//go:hide Struct.Field 1.15
//go:hide Type.Method 1.15
//go:hide InterfaceType.Method 1.15

These comments do not remove the symbol. In particular, they do not remove methods, and types continue to work as before. What they remove is specific to the export data: if a package being compiled at language version 1.15 or earlier imports a package p with the above comments, it can refer to p.Function. If a package being compiled at language version 1.16 or later imports p, it cannot refer to p.Function. Any reference to p.Function will be reported as a compiler error saying something like "p.Function has been removed as of language version 1.15".

These comments do not change any aspect of compiling package p. It doesn't affect build caching or type checking of the package p itself, and these comments are not affected by the language version used to compile p itself. In particular, there is no support for p.Function means one thing in language version 1.15 and a different thing in language version 1.16 (although this could be done using build tags). (It might be appropriate to initially forbid the appearance of a go:hide comment in a file that uses a Go release build tag such as //go:build 1.15, as the meaning of that is somewhat obscure.)

This rule will not apply to test code within p, so a test in package p_test will be able to refer to hidden symbols in p, regardless of the language version being used.

We could also add an optional additional argument that is a string that is included in the compiler error message.

//go:hide Function 1.15 "Use NewFunction"
//go:hide Type 1.15 "Use package p2 instead"
//go:hide InsecureFunction 1.15 "InsecureFunction is a security hole"
@networkimprov
Copy link

@networkimprov networkimprov commented Nov 11, 2020

Nice, but maybe leading with the version is more readable over time...

//go:hide 1.15 FunctionA "use FunctionB"
//go:hide 1.15 TypeA     "use TypeB"
//go:hide 1.16 FunctionB "use FunctionC"
@mvdan
Copy link
Member

@mvdan mvdan commented Nov 11, 2020

I guess that depends on whether you'd like to group the "hide" directives by Go version, or by exported name. I think I'd do the latter, since they should be right next to the declaration itself.

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

Successfully merging a pull request may close this issue.

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