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

go/parser: Accepts method definition without receiver #11271

Closed
dominikh opened this issue Jun 18, 2015 · 6 comments
Closed

go/parser: Accepts method definition without receiver #11271

dominikh opened this issue Jun 18, 2015 · 6 comments
Assignees
Milestone

Comments

@dominikh
Copy link
Member

@dominikh dominikh commented Jun 18, 2015

go/parser accepts the following program:

package main
func () Foo() {}

without reporting any error. It should complain about the missing method receiver.

Code to reproduce:

package main

import (
    "fmt"
    "go/ast"
    "go/parser"
    "go/token"
)

func main() {
    var s = `package main
func () Foo() {}`

    fset := token.NewFileSet()
    f, err := parser.ParseFile(fset, "foo.go", s, parser.AllErrors)
    fmt.Println(err)
    fmt.Println(len(f.Decls[0].(*ast.FuncDecl).Recv.List))
}
@ianlancetaylor ianlancetaylor added this to the Go1.6 milestone Jun 18, 2015
@griesemer
Copy link
Contributor

@griesemer griesemer commented Jun 18, 2015

This is working as intended. The go/parser accepts a wider syntax than is permitted by the language spec, for simplicity, and robustness. Also, some (like this one) checks are easier done after parsing - putting them into the parser makes it harder for other tools (e.g. go/types: which error should it check for, and which should it not check for? This has implications for testing as some errors may be reported twice by different libs, etc.).

See a3c0ca5
(and also the bug associated with it).

@griesemer griesemer closed this Jun 18, 2015
@cespare
Copy link
Contributor

@cespare cespare commented Jun 18, 2015

It's quite unfortunate that downstream tools have to handle invalid syntax in the output of go/parser. This mismatch of expectations caused a bug in lint: golang/lint#134. In that issue, @dsymonds mentions that he too would expect go/parser to handle this case. Gopherjs is another tool that now crashes on this kind of input. I'll check for others, but it's likely that many tools which are based on go/parser assume a receiver exists and will need to be changed to avoid crashes.

@cespare
Copy link
Contributor

@cespare cespare commented Jun 18, 2015

/cc @rsc who originally asked for this change in #8493.

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 18, 2015

CL https://golang.org/cl/11262 mentions this issue.

@cespare
Copy link
Contributor

@cespare cespare commented Jun 18, 2015

(Also, just to check -- doesn't this violate the compatibility promise?)

@griesemer
Copy link
Contributor

@griesemer griesemer commented Jun 18, 2015

@cespare There is no easy or even obvious solution to this problem:

  1. The spec's syntactic declarations are not 100% exact - some of the restrictions are in the prose rather than the syntax. Also, some of the productions are in forms suitable for didactic reasons and not necessarily for a parser. We don't want to be bound by an implementation for the spec.

  2. The AST does not reflect the language 100% in its types - it's either impossible or needlessly complicated (e.g. how to represent the syntactic restrictions of 3-value slices?).

  3. In the presence of 1) and 2) the question arises what should the parser do? Originally, the parser was more stringent and tried to be as close to the spec as possible, in cases also enforcing restrictions not in the syntax but in the prose. This was problematic: for one, it's not always clear how far to go, and secondly, it's even more difficult to specify how far the parser is going since the AST nodes don't necessarily reflect what has been already checked (e.g. ast.SliceExpr simply has 3 Expr fields, but if the 3rd expression is present, all others must be present as well - this is not expressed explicitly).

  4. Tools (such as the type checker) do some of their own checking. In fact, for method receivers, the type-checker has to repeat several of the tests that the parser was doing before. Where to stop? Should it simply assume that the AST's FieldList is restricted to one receiver (in the method decl example)? Should it assume that the receiver type is a named type or a pointer to a named type (all things that the parser could - partially at least - check). It's a bottom-less pit. But worse: what about ASTs that are not generated by the parser? The type-checker thus must assume the AST can be incorrect in various ways, and for its own sake and robustness it must check all properties explicitly. One could argue that it's still ok for the parser to do some of those checks as well (like there must be exactly one receiver) - but that leads yet to other problems: for instance, when testing parser and type-checker, the same error (slightly differently phrased) may appear.

Thus, we decided that it's best to be conservative in the parser and accept a wider language. What wider language one may ask? There's a fairly natural correspondence between accepted language and what the AST can represent: If an AST field is not restricted (e.g. in the case of receivers, the ast.FuncDecl.Recv field is simply an *ast.FieldList), then the parser may accept more than one parameter (as is the case) and a client must do whatever checking is needed if the language imposes restrictions (in the case of the receiver, a client must check that the respective FieldList is correct).

A more formal approach would be welcome, but this is what we have. Note that the situation is the same in an actual compiler, it's just that there the internals are not exposed. Also, note that many tools don't care (e.g. gofmt will just reproduce what it gets).

The rule is: If a program consumes an incoming AST the program must check slice lengths etc. before accessing elements - it cannot simply assume that the AST was built according to the spec. If the program assumes a field of type Expr is of a specific expression, it must use a type assertion (that may fail), etc.

We have been thinking about an paired approach:

    1. a parser that is conservative and accepts a wider language than the spec permits (as is the case now)
    1. a separate light-weight AST checker that checks the AST for syntactic correctness (but no type-checking) so that client programs don't have to. A type-checker wouldn't use this tool.

It's not clear that this avoids many problems. It's very likely that same problem appears between that 2nd light-weight tool and any client.

griesemer added a commit that referenced this issue Jun 18, 2015
See also issue #11271.

Change-Id: I34175f46ce137b14ca483500f673b0f8ee1f2108
Reviewed-on: https://go-review.googlesource.com/11262
Reviewed-by: Alan Donovan <adonovan@google.com>
@mikioh mikioh modified the milestones: Go1.5, Go1.6 Jun 18, 2015
@golang golang locked and limited conversation to collaborators Jun 25, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.