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.

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

Jump to bottom

error comparing declarations: missing ',' in parameter list #39

Open
dmitshur opened this issue Nov 7, 2017 · 9 comments
Open

error comparing declarations: missing ',' in parameter list #39

dmitshur opened this issue Nov 7, 2017 · 9 comments

Comments

@dmitshur
Copy link

dmitshur commented Nov 7, 2017

This is not a critical issue for me; reporting it for your benefit only. You should prioritize accordingly.

I noticed the following unexpected error in GopherCI (https://gci.gopherci.io/analysis/1198) from apicompat:

$ apicompat -before 5a67cc840e31516e2b2e1ab0b839ecb628ec56b9~3 ./...
error comparing declarations: 2:77: missing ',' in parameter list parsing: package expr
type ExternalService interface{MarkRead(ctx context.Context, repo github.com/shurcooL/notifications.RepoSpec, threadType string, threadID uint64) error; Notify(ctx context.Context, repo github.com/shurcooL/notifications.RepoSpec, threadType string, threadID uint64, nr github.com/shurcooL/notifications.NotificationRequest) error; Subscribe(ctx context.Context, repo github.com/shurcooL/notifications.RepoSpec, threadType string, threadID uint64, subscribers []github.com/shurcooL/users.UserSpec) error}
     0  *ast.GenDecl {
     1  .  TokPos: -
     2  .  Tok: type
     3  .  Lparen: -
     4  .  Specs: []ast.Spec (len = 1) {
     5  .  .  0: *ast.TypeSpec {
     6  .  .  .  Name: *ast.Ident {
     7  .  .  .  .  NamePos: 5a67cc840e31516e2b2e1ab0b839ecb628ec56b9~3:notifications.go:13:6
     8  .  .  .  .  Name: "Service"
     9  .  .  .  .  Obj: *ast.Object {
    10  .  .  .  .  .  Kind: type
    11  .  .  .  .  .  Name: "Service"
    12  .  .  .  .  .  Decl: *(obj @ 5)
    13  .  .  .  .  }
    14  .  .  .  }
    15  .  .  .  Assign: -
    16  .  .  .  Type: *ast.InterfaceType {
    17  .  .  .  .  Interface: 5a67cc840e31516e2b2e1ab0b839ecb628ec56b9~3:notifications.go:13:14
    18  .  .  .  .  Methods: *ast.FieldList {
    19  .  .  .  .  .  Opening: 5a67cc840e31516e2b2e1ab0b839ecb628ec56b9~3:notifications.go:13:24
    20  .  .  .  .  .  List: []*ast.Field (len = 4) {
    21  .  .  .  .  .  .  0: *ast.Field {
    22  .  .  .  .  .  .  .  Names: []*ast.Ident (len = 1) {
    23  .  .  .  .  .  .  .  .  0: *ast.Ident {
    24  .  .  .  .  .  .  .  .  .  NamePos: 5a67cc840e31516e2b2e1ab0b839ecb628ec56b9~3:notifications.go:16:2
    25  .  .  .  .  .  .  .  .  .  Name: "List"
    26  .  .  .  .  .  .  .  .  .  Obj: *ast.Object {
    27  .  .  .  .  .  .  .  .  .  .  Kind: func
    28  .  .  .  .  .  .  .  .  .  .  Name: "List"
    29  .  .  .  .  .  .  .  .  .  .  Decl: *(obj @ 21)
    30  .  .  .  .  .  .  .  .  .  }
    31  .  .  .  .  .  .  .  .  }
    32  .  .  .  .  .  .  .  }
    33  .  .  .  .  .  .  .  Type: *ast.FuncType {
    34  .  .  .  .  .  .  .  .  Func: -
    35  .  .  .  .  .  .  .  .  Params: *ast.FieldList {
    36  .  .  .  .  .  .  .  .  .  Opening: 5a67cc840e31516e2b2e1ab0b839ecb628ec56b9~3:notifications.go:16:6
    37  .  .  .  .  .  .  .  .  .  List: []*ast.Field (len = 2) {
    38  .  .  .  .  .  .  .  .  .  .  0: *ast.Field {
    39  .  .  .  .  .  .  .  .  .  .  .  Names: []*ast.Ident (len = 1) {
    40  .  .  .  .  .  .  .  .  .  .  .  .  0: *ast.Ident {
    41  .  .  .  .  .  .  .  .  .  .  .  .  .  NamePos: 5a67cc840e31516e2b2e1ab0b839ecb628ec56b9~3:notifications.go:16:7
    42  .  .  .  .  .  .  .  .  .  .  .  .  .  Name: "ctx"
    43  .  .  .  .  .  .  .  .  .  .  .  .  .  Obj: *ast.Object {
    44  .  .  .  .  .  .  .  .  .  .  .  .  .  .  Kind: var
    45  .  .  .  .  .  .  .  .  .  .  .  .  .  .  Name: "ctx"
    46  .  .  .  .  .  .  .  .  .  .  .  .  .  .  Decl: *ast.Field {
    47  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  Names: []*ast.Ident (len = 1) {
    48  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  0: *(obj @ 40)
    49  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  }
    50  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  Type: *ast.SelectorExpr {
    51  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  X: *ast.Ident {
    52  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  NamePos: 5a67cc840e31516e2b2e1ab0b839ecb628ec56b9~3:notifications.go:16:11
    53  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  Name: "context"
    54  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  }
    55  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  Sel: *ast.Ident {
    56  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  NamePos: 5a67cc840e31516e2b2e1ab0b839ecb628ec56b9~3:notifications.go:16:19
    57  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  Name: "Context"
    58  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  }
    59  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  }
    60  .  .  .  .  .  .  .  .  .  .  .  .  .  .  }
    61  .  .  .  .  .  .  .  .  .  .  .  .  .  }
    62  .  .  .  .  .  .  .  .  .  .  .  .  }
    63  .  .  .  .  .  .  .  .  .  .  .  }
    64  .  .  .  .  .  .  .  .  .  .  .  Type: *(obj @ 50)
    65  .  .  .  .  .  .  .  .  .  .  }
    66  .  .  .  .  .  .  .  .  .  .  1: *ast.Field {
    67  .  .  .  .  .  .  .  .  .  .  .  Names: []*ast.Ident (len = 1) {
    68  .  .  .  .  .  .  .  .  .  .  .  .  0: *ast.Ident {
    69  .  .  .  .  .  .  .  .  .  .  .  .  .  NamePos: 5a67cc840e31516e2b2e1ab0b839ecb628ec56b9~3:notifications.go:16:28
    70  .  .  .  .  .  .  .  .  .  .  .  .  .  Name: "opt"
    71  .  .  .  .  .  .  .  .  .  .  .  .  .  Obj: *ast.Object {
    72  .  .  .  .  .  .  .  .  .  .  .  .  .  .  Kind: var
    73  .  .  .  .  .  .  .  .  .  .  .  .  .  .  Name: "opt"
    74  .  .  .  .  .  .  .  .  .  .  .  .  .  .  Decl: *ast.Field {
    75  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  Names: []*ast.Ident (len = 1) {
    76  .  .  .  .  .  .  .  .  .  .  .  .  .  .  . ...237791 bytes suppressed....  .  .  .  .  .  .  .  .  .  .  .  .  .  NamePos: HEAD:notifications.go:87:2
  1340  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  Name: "HTMLURL"
  1341  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  Obj: *ast.Object {
  1342  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  Kind: var
  1343  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  Name: "HTMLURL"
  1344  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  Decl: *ast.Field {
  1345  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  Names: []*ast.Ident (len = 1) {
  1346  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  0: *(obj @ 1338)
  1347  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  }
  1348  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  Type: *ast.Ident {
  1349  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  NamePos: HEAD:notifications.go:87:12
  1350  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  Name: "string"
  1351  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  }
  1352  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  }
  1353  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  }
  1354  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  }
  1355  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  }
  1356  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  Type: *(obj @ 1348)
  1357  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  }
  1358  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  }
  1359  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  Closing: HEAD:notifications.go:88:1
  1360  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  }
  1361  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  Incomplete: false
  1362  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  }
  1363  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  }
  1364  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  }
  1365  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  }
  1366  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  }
  1367  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  }
  1368  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  }
  1369  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  }
  1370  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  Type: *(obj @ 1194)
  1371  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  }
  1372  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  }
  1373  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  Closing: HEAD:notifications.go:44:103
  1374  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  }
  1375  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  Results: *ast.FieldList {
  1376  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  Opening: -
  1377  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  List: []*ast.Field (len = 1) {
  1378  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  0: *ast.Field {
  1379  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  Type: *ast.Ident {
  1380  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  NamePos: HEAD:notifications.go:44:105
  1381  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  Name: "error"
  1382  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  }
  1383  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  }
  1384  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  }
  1385  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  Closing: -
  1386  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  }
  1387  .  .  .  .  .  .  .  .  .  .  .  .  .  .  }
  1388  .  .  .  .  .  .  .  .  .  .  .  .  .  }
  1389  .  .  .  .  .  .  .  .  .  .  .  .  }
  1390  .  .  .  .  .  .  .  .  .  .  .  .  Closing: HEAD:notifications.go:45:1
  1391  .  .  .  .  .  .  .  .  .  .  .  }
  1392  .  .  .  .  .  .  .  .  .  .  .  Incomplete: false
  1393  .  .  .  .  .  .  .  .  .  .  }
  1394  .  .  .  .  .  .  .  .  .  }
  1395  .  .  .  .  .  .  .  .  }
  1396  .  .  .  .  .  .  .  }
  1397  .  .  .  .  .  .  }
  1398  .  .  .  .  .  }
  1399  .  .  .  .  .  Closing: HEAD:notifications.go:26:1
  1400  .  .  .  .  }
  1401  .  .  .  .  Incomplete: false
  1402  .  .  .  }
  1403  .  .  }
  1404  .  }
  1405  .  Rparen: -
  1406  }

The code compiles without errors. I'm not sure what this is. It might be related to alias support (#27), but it doesn't look that way. notifications.ExternalService is a pretty simple interface.

I can provide additional information if needed, just ask.
@dmitshur
Copy link
Author

dmitshur commented Nov 7, 2017

The commit that triggered CI was shurcooL/notifications@5a67cc8. It includes the following code:

 type Router interface {
 	// IssueURL returns the HTML URL of the specified GitHub issue.
-	IssueURL(owner, repo string, issueID uint64, commentID uint64) string
+	IssueURL(owner, repo string, issueID, commentID uint64) string
+
+	// PullRequestURL returns the HTML URL of the specified GitHub pull request.
+	PullRequestURL(owner, repo string, prID, commentID uint64) string
 }

Just a hunch from the error text, could it be that owner, repo string, issueID, commentID uint64 syntax isn't supported?

@bradleyfalzon
Copy link
Owner

Thanks, I'll get a look at this next, gimmie a little bit as things are pretty intense at the moment.

Just a hunch from the error text, could it be that owner, repo string, issueID, commentID uint64 syntax isn't supported?

I'd make the same hunch, but this should be supported, it was one of the main reasons I started using go/types instead of just go/ast. It should be supported by the test:

apicompat/testdata/after.go

Line 78 in 5f916b1

var VarChangeTypeFuncInferred func(arg1, arg2 int) (ret1, ret2 bool)

But perhaps there's an edge case here.

@bradleyfalzon
Copy link
Owner

bradleyfalzon commented Nov 17, 2017 •
edited
Loading

Seems it's broken in go1.9.0 (working in go1.8.5), didn't notice as I was running an old build.

@bradleyfalzon
Copy link
Owner

apicompat tests appear to fail due to golang/go@ee272bb but this issue may be unrelated.

@bradleyfalzon
Copy link
Owner

OK, tests failing were unrelated, I'll create a separate issue for that.

I need to be brief (out of time) but the issue is in exprInterfaceType (changes an ast.Expr into an ast.InterfaceType).

It does this by changing expr into a string and then uses go/ast to parse it (this is a terrible hack, I know, but it's there because I couldn't do it any other way).

The issue is the string in this case is:

package expr
type ExternalService interface{
        MarkRead(ctx context.Context, appID string, repo github.com/shurcooL/notifications.RepoSpec, threadID uint64) error;
        Notify(ctx context.Context, appID string, repo github.com/shurcooL/notifications.RepoSpec, threadID uint64, nr github.com/shurcooL/notifications.NotificationRequest) error;
        Subscribe(ctx context.Context, appID string, repo github.com/shurcooL/notifications.RepoSpec, threadID uint64, subscribers []github.com/shurcooL/users.UserSpec) error
}

Which importantly, contains import path with the type identifiers github.com/shurcooL/notifications.RepoSpec.

Adding the following fixes the issue.

src = strings.Replace(src, "github.com/shurcooL/", "", -1)

The semicolon may also be a problem, but it's been working all this time, I'd be surprised if it's an issue but should be fixed anyway.

Thanks for the issue, I'll work on a fix for this.

@dmitshur
Copy link
Author

Thanks for the issue, I'll work on a fix for this.

No problem, I'm happy if it's helpful. :)

Thanks for describing what the issue appears to be, it's interesting for me to learn about.

Just for extra visibility, I'll quote this from my original issue report:

This is not a critical issue for me; reporting it for your benefit only. You should prioritize accordingly.

@dmitshur
Copy link
Author

The issue is the string in this case is:

package expr
type ExternalService interface{
        MarkRead(ctx context.Context, appID string, repo github.com/shurcooL/notifications.RepoSpec, threadID uint64) error;
        Notify(ctx context.Context, appID string, repo github.com/shurcooL/notifications.RepoSpec, threadID uint64, nr github.com/shurcooL/notifications.NotificationRequest) error;
        Subscribe(ctx context.Context, appID string, repo github.com/shurcooL/notifications.RepoSpec, threadID uint64, subscribers []github.com/shurcooL/users.UserSpec) error
}

Something jumps to me here. Where are you getting that from? That looks like a much older version of that interface. The current version of github.com/shurcooL/notifications.ExternalService interface has a different order and names of parameters.

@bradleyfalzon
Copy link
Owner

bradleyfalzon commented Nov 18, 2017 •
edited
Loading

I had been manually bisecting older notifications commits, so I might have just noted it from an older, but still broken version. So ignore the age issue.

The following definition from master is right? If so, then it must just be me noting an older interface.

package expr
type ExternalService interface{
    MarkRead(ctx context.Context, repo github.com/shurcooL/notifications.RepoSpec, threadType string, threadID uint64) error
    Notify(ctx context.Context, repo github.com/shurcooL/notifications.RepoSpec, threadType string, threadID uint64, nr github.com/shurcooL/notifications.NotificationRequest) error
    Subscribe(ctx context.Context, repo github.com/shurcooL/notifications.RepoSpec, threadType string, threadID uint64, subscribers []github.com/shurcooL/users.UserSpec) error
}

@dmitshur
Copy link
Author

dmitshur commented Nov 19, 2017 •
edited
Loading

Oh, ok, gotcha. Yeah, that’s the latest. I just wanted to make sure the original issue wasn’t caused due to version skew.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@bradleyfalzon @dmitshur