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

x/tools/go/analysis/cmd/vet: add -cgoinclude check for #include of code in subdirectories #26506

Open
rsc opened this issue Jul 20, 2018 · 8 comments
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@rsc
Copy link
Contributor

rsc commented Jul 20, 2018

The go build model is that all code for a package is in one directory.
The cache relies on this.
Vendoring tools rely on this.
Probably many other things rely on this.

One of the few ways a package can violate this rule is by using
#include in cgo source code to access code in nearby directories.
This appears to work, in that the package builds,
but it doesn't cache properly, doesn't vendor properly, and so on.

It seems reasonable for cmd/vet to notice such #includes and
complain about them, and we could enable that check as one
of the automatic ones run during go test.

Suggested in #26366.

@rsc rsc added this to the Go1.12 milestone Jul 20, 2018
@rsc rsc added NeedsFix The path to resolution is known, but the work has not been done. help-wanted labels Jul 20, 2018
@iamoryanmoshe
Copy link
Contributor

What's the bullet point for this issue?
Not to allow #include in code if it contains a relative path?

Would love to work on it if @shivakumargn isn't

@ianlancetaylor
Copy link
Contributor

Yes, look at the cgo preamble comment and the C/C++ files (including header files) in the directory, and warn about uses of #include "" (not #include <>, <> is always OK) with a relative path that contains at least one path separator.

@iamoryanmoshe
Copy link
Contributor

iamoryanmoshe commented Jul 24, 2018

I'm on it!
@ianlancetaylor BTW, why check the files in the directory? I can just throw every include that contains the / char.
Or am I missing something?

@bcmills
Copy link
Contributor

bcmills commented Jul 24, 2018

I can just throw every include that contains the / char.

Includes that fall outside the Go source tree aren't our responsibility to warn about: #include "/usr/include/blah.h" is not usually a good idea, but not for the same reason as #include "subdir/blah.h".

@iamoryanmoshe
Copy link
Contributor

OK, so I throw an error only when the include is in a subdir of the project, but not when it is directly in the same dir as the go file, and not when it's outside the src tree.

Got you right?

Should I throw a warning for the third case or just leave it be?

@bcmills
Copy link
Contributor

bcmills commented Jul 26, 2018

If it's outside the source tree, just leave it be. (It's not portable, but that is likely intentional: for example, the Go program may be a command-line tool for managing some aspect of a particular OS or distribution.)

@gopherbot
Copy link

Change https://golang.org/cl/126375 mentions this issue: vet: check for invalid Cincludes in cgo code

@odeke-em
Copy link
Member

odeke-em commented Jun 6, 2019

Hello @iamoryanmoshe, thank you for sending the CL and for taking this on!

As @mvdan and @thepudds had mentioned in the CL, these checks have moved from the standard library to https://godoc.org/golang.org/x/tools/go/analysis/cmd/vet aka https://github.com/golang/tools/tree/master/go/analysis/cmd/vet perhaps we can close the original CL and resurrect it by sending a CL to x/tools but also then I'll retitle this issue and we can even no longer tie it down to a specific release.

@odeke-em odeke-em changed the title cmd/vet: add -cgoinclude check for #include of code in subdirectories x/tools/go/analysis/cmd/vet: add -cgoinclude check for #include of code in subdirectories Jun 6, 2019
@odeke-em odeke-em modified the milestones: Go1.13, Unplanned Jun 6, 2019
@adonovan adonovan added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label Apr 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

9 participants