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

Report the use of deprecated stuff #238

Closed
rakyll opened this Issue Sep 7, 2016 · 13 comments

Comments

Projects
None yet
5 participants
@rakyll
Member

rakyll commented Sep 7, 2016

golint should be able to report cases user is depending on deprecated things. For the case below, it should be able to output "Deprecated: Use Client or Transport in package net/http instead".

$ cat main.go
package main

import (
    "net/http/httputil"
)

func main() {
    httputil.NewClientConn(nil, nil)
}

@rakyll rakyll changed the title from Report use of deprecated stuff to Report the use of deprecated stuff Sep 7, 2016

@dsnet

This comment has been minimized.

Show comment
Hide comment
@dsnet

dsnet Sep 7, 2016

Member

For reference: golang/go#10909

Member

dsnet commented Sep 7, 2016

For reference: golang/go#10909

@rakyll

This comment has been minimized.

Show comment
Hide comment
@rakyll

rakyll Sep 7, 2016

Member

This change will also encourage the use of the same deprecation message formatting in the third party packages.

Member

rakyll commented Sep 7, 2016

This change will also encourage the use of the same deprecation message formatting in the third party packages.

@stapelberg

This comment has been minimized.

Show comment
Hide comment
@stapelberg

stapelberg Aug 16, 2017

Currently, golint parses and type-checks the file to lint. For type-checking, the gc compiler’s export data is used.

In order to check whether a certain ast.SelectorExpr refers to something deprecated (and what the deprecation message reads), the export data is not sufficient.

Hence, I see two options:

  1. Change the export data to include the deprecation message, if any. This increases the size of our export data files. Looking at the standard library, the size increase is about 3 KB in total (grep -r -h 'Deprecated: ' | sed 's,.*Deprecated: ,,g' | sed 's,\s*// ,,g' | wc). If we de-duplicated deprecation messages, the size increase is reduced to about 1.6 KB.
  2. Change golint to parse and type-check all packages which are imported directly by the file to lint.

It seems to me that option 1 is overly complicated for what we’re trying to achieve, hence I’d like to implement option 2, even though it makes golint slower (because it needs to do more work). Does that sound reasonable to you?

For option 2, I have a prototype which uses golang.org/x/tools/go/loader and correctly produces a lint warning on your example source code.

I hope to send a pull request within the next few days.

stapelberg commented Aug 16, 2017

Currently, golint parses and type-checks the file to lint. For type-checking, the gc compiler’s export data is used.

In order to check whether a certain ast.SelectorExpr refers to something deprecated (and what the deprecation message reads), the export data is not sufficient.

Hence, I see two options:

  1. Change the export data to include the deprecation message, if any. This increases the size of our export data files. Looking at the standard library, the size increase is about 3 KB in total (grep -r -h 'Deprecated: ' | sed 's,.*Deprecated: ,,g' | sed 's,\s*// ,,g' | wc). If we de-duplicated deprecation messages, the size increase is reduced to about 1.6 KB.
  2. Change golint to parse and type-check all packages which are imported directly by the file to lint.

It seems to me that option 1 is overly complicated for what we’re trying to achieve, hence I’d like to implement option 2, even though it makes golint slower (because it needs to do more work). Does that sound reasonable to you?

For option 2, I have a prototype which uses golang.org/x/tools/go/loader and correctly produces a lint warning on your example source code.

I hope to send a pull request within the next few days.

@rakyll

This comment has been minimized.

Show comment
Hide comment
@rakyll

rakyll Aug 16, 2017

Member

We want golint to be able to report third party library deprecations as well. I don't think (1) is something we should go with. The ecosystem wants to be able to break APIs but our tools don't do a good job helping the authors communicating these breakages.

I tried to prototype (2) a while ago by modifying the loader package and I personally didn't like the solution, because all of a sudden package loading has to deal with loading dependencies and handling them. But to be fair, I cannot think of any perfect solution right now.

Member

rakyll commented Aug 16, 2017

We want golint to be able to report third party library deprecations as well. I don't think (1) is something we should go with. The ecosystem wants to be able to break APIs but our tools don't do a good job helping the authors communicating these breakages.

I tried to prototype (2) a while ago by modifying the loader package and I personally didn't like the solution, because all of a sudden package loading has to deal with loading dependencies and handling them. But to be fair, I cannot think of any perfect solution right now.

@dominikh

This comment has been minimized.

Show comment
Hide comment
@dominikh

dominikh Aug 16, 2017

Member

A couple of thoughts:

  1. Is golint really the right place for this check? As I understand it, golint is primarily about style. Is using deprecated identifiers a style issue?
  2. staticcheck includes a check for use of deprecated identifiers, so feel free to copy from it. Be warned, though, it depends on some parts of my framework that you'd have to replace.
  3. I'd also be concerned with forcing golint to work on entire packages, and type-check all dependencies (which will make it significantly slower) for a single check.
  4. @rakyll, what modifications to the loader package did you feel were necessary? go/loader takes care of dependencies, parsing and type-checking for you. It would be trivial to use it to load a package, and its dependencies, from source. Though of course see point 3.
Member

dominikh commented Aug 16, 2017

A couple of thoughts:

  1. Is golint really the right place for this check? As I understand it, golint is primarily about style. Is using deprecated identifiers a style issue?
  2. staticcheck includes a check for use of deprecated identifiers, so feel free to copy from it. Be warned, though, it depends on some parts of my framework that you'd have to replace.
  3. I'd also be concerned with forcing golint to work on entire packages, and type-check all dependencies (which will make it significantly slower) for a single check.
  4. @rakyll, what modifications to the loader package did you feel were necessary? go/loader takes care of dependencies, parsing and type-checking for you. It would be trivial to use it to load a package, and its dependencies, from source. Though of course see point 3.
@stapelberg

This comment has been minimized.

Show comment
Hide comment
@stapelberg

stapelberg Aug 16, 2017

I'd also be concerned with forcing golint to work on entire packages, and type-check all dependencies (which will make it significantly slower) for a single check.

Note that using the loader doesn’t force us to work on entire packages. Via the CreatePkgs field in loader.Config, we can specify an individual file in what the loader calls an ad-hoc package.

Also note that we only load and type-check direct dependencies, not all dependencies.

stapelberg commented Aug 16, 2017

I'd also be concerned with forcing golint to work on entire packages, and type-check all dependencies (which will make it significantly slower) for a single check.

Note that using the loader doesn’t force us to work on entire packages. Via the CreatePkgs field in loader.Config, we can specify an individual file in what the loader calls an ad-hoc package.

Also note that we only load and type-check direct dependencies, not all dependencies.

@dominikh

This comment has been minimized.

Show comment
Hide comment
@dominikh

dominikh Aug 16, 2017

Member

You can create ad-hoc packages, but if you depend on them to type check without error (which you might not), then the ad-hoc package needs to contain all files of the package.

In order to type-check direct dependencies successfully (which, again, may not be necessary just for the sake of parsing deprecation comments), you need to check dependencies recursively, or build your own version of go/loader that only parses direct dependencies from source, and uses object files for transitive dependencies (which has its own issues.)

Member

dominikh commented Aug 16, 2017

You can create ad-hoc packages, but if you depend on them to type check without error (which you might not), then the ad-hoc package needs to contain all files of the package.

In order to type-check direct dependencies successfully (which, again, may not be necessary just for the sake of parsing deprecation comments), you need to check dependencies recursively, or build your own version of go/loader that only parses direct dependencies from source, and uses object files for transitive dependencies (which has its own issues.)

@dominikh

This comment has been minimized.

Show comment
Hide comment
@dominikh

dominikh Aug 16, 2017

Member

Though I do think you want to load transitive dependencies, to be able to flag deprecated uses in code like this:

import "A"

func fn() {
  A.Fn().Foo()`
}

where A.Fn returns a type from package B (or C or ...) and where Foo is deprecated.

Member

dominikh commented Aug 16, 2017

Though I do think you want to load transitive dependencies, to be able to flag deprecated uses in code like this:

import "A"

func fn() {
  A.Fn().Foo()`
}

where A.Fn returns a type from package B (or C or ...) and where Foo is deprecated.

@stapelberg

This comment has been minimized.

Show comment
Hide comment
@stapelberg

stapelberg Aug 16, 2017

Fair point.

The remaining question is about whether the slow-down is acceptable in golint. How do we make that decision?

stapelberg commented Aug 16, 2017

Fair point.

The remaining question is about whether the slow-down is acceptable in golint. How do we make that decision?

stapelberg added a commit to stapelberg/lint that referenced this issue Aug 17, 2017

@stapelberg

This comment has been minimized.

Show comment
Hide comment
@stapelberg

stapelberg Aug 17, 2017

I sent PR #318 so that we have some actual code to discuss this over (I find that always helps).

Regarding the slow-down, here are numbers on my workstation of running go test github.com/golang/lint:

before:

ok  	github.com/golang/lint	0.130s

after:

ok  	github.com/golang/lint	3.053s

Thoughts?

stapelberg commented Aug 17, 2017

I sent PR #318 so that we have some actual code to discuss this over (I find that always helps).

Regarding the slow-down, here are numbers on my workstation of running go test github.com/golang/lint:

before:

ok  	github.com/golang/lint	0.130s

after:

ok  	github.com/golang/lint	3.053s

Thoughts?

@dominikh

This comment has been minimized.

Show comment
Hide comment
@dominikh

dominikh Aug 17, 2017

Member

My opinion is that this check, in this tool, isn't worth it. It's 30 times slower, which will piss off people who use golint in their editors. It cannot operate on invalid input anymore (a clear change in behaviour). And it isn't a style issue and hence out of scope.

The following advice is clearly biased, but I'd recommend people use my staticcheck if they want to check for deprecated identifiers. There's no need to burden golint with this check.

Member

dominikh commented Aug 17, 2017

My opinion is that this check, in this tool, isn't worth it. It's 30 times slower, which will piss off people who use golint in their editors. It cannot operate on invalid input anymore (a clear change in behaviour). And it isn't a style issue and hence out of scope.

The following advice is clearly biased, but I'd recommend people use my staticcheck if they want to check for deprecated identifiers. There's no need to burden golint with this check.

@andybons

This comment has been minimized.

Show comment
Hide comment
@andybons

andybons Feb 21, 2018

Member

Thanks for the issue.

This appears to be out of scope and so is being closed. If you feel this is in error please feel free to re-open.

Please note this is not the forum to discuss edits to CodeReviewComments. If you wish to propose changes to it please send a mail to golang-nuts.

Member

andybons commented Feb 21, 2018

Thanks for the issue.

This appears to be out of scope and so is being closed. If you feel this is in error please feel free to re-open.

Please note this is not the forum to discuss edits to CodeReviewComments. If you wish to propose changes to it please send a mail to golang-nuts.

@andybons andybons closed this Feb 21, 2018

@dsnet

This comment has been minimized.

Show comment
Hide comment
@dsnet

dsnet Feb 21, 2018

Member

I have no opinion as to whether this is considered "out of scope" of the lint tool, but just wanted to note that if we consider this out-of-scope, then so should #302, which is really just one special-case application of this more general issue.

I feel like use of deprecated is halfway between a style issue and a correctness one. I'll happily use statticcheck. However, the 30x slowdown for type-checking is a serious practical factor to consider.

Member

dsnet commented Feb 21, 2018

I have no opinion as to whether this is considered "out of scope" of the lint tool, but just wanted to note that if we consider this out-of-scope, then so should #302, which is really just one special-case application of this more general issue.

I feel like use of deprecated is halfway between a style issue and a correctness one. I'll happily use statticcheck. However, the 30x slowdown for type-checking is a serious practical factor to consider.

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