Skip to content

WIP: Ability to call overridden functions from the std library #798

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

WIP: Ability to call overridden functions from the std library #798

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b17b713
build: Allow overriden functions calls
r-l-x Dec 21, 2016
1baef4b
regexp test: added an example use for gopherjs:keep_overridden
r-l-x Apr 20, 2018
4bcab8f
compiler/natives: regenerate
r-l-x Apr 20, 2018
21522a8
fixed typo
r-l-x Apr 21, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 41 additions & 11 deletions build/build.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,23 @@ func ImportDir(dir string, mode build.ImportMode, installSuffix string, buildTag
return &PackageData{Package: pkg, JSFiles: jsFiles}, nil
}

// Test if we find the '//gopherjs:keep_overridden' comment
func findKeepOverriddenComment(doc *ast.CommentGroup) bool {
if doc == nil {
return false
}
for _, comment := range doc.List {
text := comment.Text
Copy link
Member

Choose a reason for hiding this comment

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

Might I suggest the following is slightly clearer in its intention?

text := strings.TrimRightFunc(comment.Text, unicode.IsSpace)
if text == "//gopherjs:keep_overridden" {
  return true
}

i.e. you're looking for a line that contains only the special comment, save any trailing space.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually took this part from the go pragma directive parser:
https://github.com/golang/go/blob/master/src/cmd/compile/internal/gc/noder.go#L1435
It seems like you can add text after a go pragma directive, and it will be ignored, and I think it's a good idea to do the same here.

Copy link
Member

Choose a reason for hiding this comment

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

Got it. I think you'll find that is effectively the parse phase of the compiler; so it has to deal with all types of pragma. Many of which take arguments.

In this case we've already parsed into the AST and we know we have a comment group.

And specifically, I'm guessing you want //gopherjs:keep_overridden to be used without arguments, because there are none. Hence it doesn't make sense for anything to follow that special comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Considering the pragma function from the standard library, my understanding is that pragmas expecing arguments (go:linkname, go:cgo_*) are treated in the non-default cases of the switch. The default case, from which this code is taken, only treats pragmas with no arguments (go:nointerface, go:noescape, etc., see the implementation of pragmaValues)
So it looks like the standard library allows putting any text (so basically a comment) after a pragma without argument. I think it's appropriate to have the same behaviour here (even if I agree that it's not the typical use case).

Copy link
Member

Choose a reason for hiding this comment

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

So it looks like the standard library allows putting any text (so basically a comment) after a pragma without argument. I think it's appropriate to have the same behaviour here (even if I agree that it's not the typical use case).

You're absolutely right, thanks for correcting me.

I'm easy either way; in this PR I think you can be very specific and prescriptive because you're defining the pragma so it's in your gift to do what you want 😄 - to my mind doing so keeps the documentation (on how to use your pragma) simple as well as the implementation.

But, as I said earlier, it's a minor point in any case.

if i := strings.Index(text, " "); i >= 0 {
text = text[:i]
}
if text == "//gopherjs:keep_overridden" {
return true
}
}
return false
}

// parseAndAugment parses and returns all .go files of given pkg.
// Standard Go library packages are augmented with files in compiler/natives folder.
// If isTest is true and pkg.ImportPath has no _test suffix, package is built for running internal tests.
Expand All @@ -250,12 +267,19 @@ func ImportDir(dir string, mode build.ImportMode, installSuffix string, buildTag
// The native packages are augmented by the contents of natives.FS in the following way.
// The file names do not matter except the usual `_test` suffix. The files for
// native overrides get added to the package (even if they have the same name
// as an existing file from the standard library). For all identifiers that exist
// in the original AND the overrides, the original identifier in the AST gets
// replaced by `_`. New identifiers that don't exist in original package get added.
// as an existing file from the standard library). For function identifiers that exist
// in the original AND the overrides AND that include the following directive in their comment:
// //gopherjs:keep_overridden, the original identifier in the AST gets prefixed by
// `_gopherjs_overridden_`. For other identifiers that exist in the original AND the overrides,
// the original identifier gets replaced by `_`. New identifiers that don't exist in original
// package get added.
func parseAndAugment(bctx *build.Context, pkg *build.Package, isTest bool, fileSet *token.FileSet) ([]*ast.File, error) {
var files []*ast.File
replacedDeclNames := make(map[string]bool)

type overrideInfo struct {
keepOverridden bool
}
replacedDeclNames := make(map[string]overrideInfo)
funcName := func(d *ast.FuncDecl) string {
if d.Recv == nil || len(d.Recv.List) == 0 {
return d.Name.Name
Expand Down Expand Up @@ -334,17 +358,17 @@ func parseAndAugment(bctx *build.Context, pkg *build.Package, isTest bool, fileS
for _, decl := range file.Decls {
switch d := decl.(type) {
case *ast.FuncDecl:
replacedDeclNames[funcName(d)] = true
replacedDeclNames[funcName(d)] = overrideInfo{keepOverridden: findKeepOverriddenComment(d.Doc)}
case *ast.GenDecl:
switch d.Tok {
case token.TYPE:
for _, spec := range d.Specs {
replacedDeclNames[spec.(*ast.TypeSpec).Name.Name] = true
replacedDeclNames[spec.(*ast.TypeSpec).Name.Name] = overrideInfo{}
}
case token.VAR, token.CONST:
for _, spec := range d.Specs {
for _, name := range spec.(*ast.ValueSpec).Names {
replacedDeclNames[name.Name] = true
replacedDeclNames[name.Name] = overrideInfo{}
}
}
}
Expand Down Expand Up @@ -396,23 +420,29 @@ func parseAndAugment(bctx *build.Context, pkg *build.Package, isTest bool, fileS
for _, decl := range file.Decls {
switch d := decl.(type) {
case *ast.FuncDecl:
if replacedDeclNames[funcName(d)] {
d.Name = ast.NewIdent("_")
if info, ok := replacedDeclNames[funcName(d)]; ok {
if info.keepOverridden {
// Allow overridden function calls
// The standard library implementation of foo() becomes _gopherjs_overridden_foo()
d.Name.Name = "_gopherjs_overridden_" + d.Name.Name
} else {
d.Name = ast.NewIdent("_")
}
}
case *ast.GenDecl:
switch d.Tok {
case token.TYPE:
for _, spec := range d.Specs {
s := spec.(*ast.TypeSpec)
if replacedDeclNames[s.Name.Name] {
if _, ok := replacedDeclNames[s.Name.Name]; ok {
s.Name = ast.NewIdent("_")
}
}
case token.VAR, token.CONST:
for _, spec := range d.Specs {
s := spec.(*ast.ValueSpec)
for i, name := range s.Names {
if replacedDeclNames[name.Name] {
if _, ok := replacedDeclNames[name.Name]; ok {
s.Names[i] = ast.NewIdent("_")
}
}
Expand Down
Loading