error comparing declarations: missing ',' in parameter list

[dmitshur]

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]

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]

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]

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

[bradleyfalzon]

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

[bradleyfalzon]

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]

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]

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]

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]

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