Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

Disallow import internal root package #1681

Merged
merged 3 commits into from
Jun 4, 2018
Merged

Disallow import internal root package #1681

merged 3 commits into from
Jun 4, 2018

Conversation

uudashr
Copy link
Contributor

@uudashr uudashr commented May 17, 2018

This should ignore any import of internal packages on the root such as:

internal/syscall/windows/registry
internal/nettrace
internal/singleflight
internal/cpu
internal/testenv
internal/trace
internal/testlog
internal/syscall/windows/sysdll
internal/syscall/windows
internal/syscall/unix
internal/poll
internal/race

@uudashr uudashr changed the title Disallow internal package on root Disallow import internal root package May 17, 2018
@@ -278,9 +278,13 @@ export function getNonVendorPackages(folderPath: string): Promise<string[]> {
// see: https://golang.org/s/go14internal
function isAllowToImportPackage(toDirPath: string, currentWorkspace: string, pkgPath: string) {
let internalPkgFound = pkgPath.match(/\/internal\/|\/internal$/);
if (internalPkgFound) {
Copy link
Contributor

@ramya-rao-a ramya-rao-a Jun 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldnt this be if(!internalPkgFound)?

Your case is that pkgPath has a value like internal/nettrace for which internalPkgFound would be null at this point.

Why not exit early with

if (pkgPath.startsWith('internal/') {
    return false;
}

?

Copy link
Contributor Author

@uudashr uudashr Jun 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank for the correction.. will fix soon

Copy link
Contributor

@ramya-rao-a ramya-rao-a left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@uudashr I've left a comment on why this change wouldn't work as expected. Please take a look.

@ramya-rao-a
Copy link
Contributor

Also, am curious. If no one can import such packages, how do they get used?

@uudashr
Copy link
Contributor Author

uudashr commented Jun 4, 2018

based on https://golang.org/s/go14internal

An import of a path containing the element “internal” is disallowed if the importing code is outside the tree rooted at the parent of the “internal” directory.

$ grep -r 'internal/nettrace' "$(go env GOROOT)/src"
/usr/local/Cellar/go/1.10.2/libexec/src/go/build/deps_test.go:		"internal/nettrace", "internal/poll",
/usr/local/Cellar/go/1.10.2/libexec/src/go/build/deps_test.go:		"internal/nettrace",
/usr/local/Cellar/go/1.10.2/libexec/src/go/build/deps_test.go:	"net/http/httptrace": {"context", "crypto/tls", "internal/nettrace", "net", "reflect", "time"},
/usr/local/Cellar/go/1.10.2/libexec/src/net/dial.go:	"internal/nettrace"
/usr/local/Cellar/go/1.10.2/libexec/src/net/http/httptrace/trace.go:	"internal/nettrace"
/usr/local/Cellar/go/1.10.2/libexec/src/net/http/transport_test.go:	"internal/nettrace"
/usr/local/Cellar/go/1.10.2/libexec/src/net/lookup.go:	"internal/nettrace"

It can be used by all the source reside on GOROOT

@uudashr
Copy link
Contributor Author

uudashr commented Jun 4, 2018

@ramya-rao-a please check

if (internalPkgFound) {
let rootProjectForInternalPkg = path.join(currentWorkspace, pkgPath.substr(0, internalPkgFound.index));
return toDirPath.startsWith(rootProjectForInternalPkg + path.sep) || toDirPath === rootProjectForInternalPkg;
return (rootProjectForInternalPkg !== currentWorkspace && toDirPath.startsWith(rootProjectForInternalPkg + path.sep)) || toDirPath === rootProjectForInternalPkg;
Copy link
Contributor

@ramya-rao-a ramya-rao-a Jun 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this return true for pkgPath which is say internal/something and user is working on a file directly under GOPATH/src?

I would still say, exiting early with if (pkgPath.startsWith('internal/') return false; is cleaner

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Impossible to work under GOPATH/src

Ahh.. yes.. correct.

@uudashr
Copy link
Contributor Author

uudashr commented Jun 4, 2018

More simple with your way

@ramya-rao-a ramya-rao-a merged commit 1d48333 into microsoft:master Jun 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants