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: No way to produce correct offsets when doing partial parsing #2706

Closed
nsf opened this issue Jan 16, 2012 · 4 comments
Closed

go/parser: No way to produce correct offsets when doing partial parsing #2706

nsf opened this issue Jan 16, 2012 · 4 comments
Assignees
Milestone

Comments

@nsf
Copy link

@nsf nsf commented Jan 16, 2012

Now it is suggested that everyone who uses ParseDeclList and such can and should produce
a simple wrapper like ParseExpr does. The problem is that the ParseExpr example is
broken. Let me remind you how it's implemented:

file, err := ParseFile(token.NewFileSet(), "", "package p;func
_(){_=\n//line :1\n"+x+";}", 0)

1. "//line :1" is nice, but what for? There is no access to the corresponding
token.FileSet, and therefore there is no access to line information.

2. In that example the bytes offset of the expression is broken, it will say that
expression starts with file offset of 33 bytes (minimum). But by definition the real
minimum is 0.

And unfortunately, with current interface, there is no way to produce correct offset for
partial parsing. I'm sorry, but my program was relying on correct offsets. And now I
have to do patches like this, where I fix every offset usage taking another offset
(introduced by a wrapper) into account:

nsf/gocode@805066f#diff-1


I hate clang for this, it doesn't provide a way to do partial parsing and now I have to
hate your parser too? :(
@rsc
Copy link
Contributor

@rsc rsc commented Jan 16, 2012

Comment 1:

Labels changed: added priority-go1, removed priority-triage.

Owner changed to @griesemer.

Status changed to Accepted.

@griesemer
Copy link
Contributor

@griesemer griesemer commented Jan 18, 2012

Comment 2:

1) The "//line :1" comment makes sure error messages returned by the parser have correct
line:col: information.
2) You are correct that the offset is broken, for ParseExpr(...) it's also not defined.
It's trivial to use that example and provide a custom implementation that returns the
file set and makes the offset available (even though it may need to be corrected).
For what it's worth, your code would benefit from a helper function:
func (f *AutoCompleteFile) offset(pos token.Position) int {
   f.fset.Position(pos).Offset - f.correction  // f.correction == fixlen in your code
}
and then you can just write:
f.offset(f.TokPos)
instead of the lengthy expression you have now. And then it doesn't matter if you have
to correct the offset or not as it is only needed once. That is, having such a helper
function would also be beneficial with the old parser API.
There are several reasons for eliminating the special partial parser entry points:
1) As they were implemented, they didn't take any precautions with setting up the proper
scopes. It could be done, but then the question arises if the scopes should be exposed
or not, etc.
2) The question arises, why only have those special partial parsers? Why not have one
for signatures, types, etc. It's a slippery slope that ends only when there is an entry
point for each production. We don't want to go there.
3) Creating a custom version of ParseExpr is trivial. By using exactly one parser entry
point (ParseFile), we can make sure that everything is always correctly set up. It's
easy to correct position information if necessary.
If you can provide me with a use case that cannot be handled this way, please let me
know. Otherwise I will consider this as closed once we freeze the API for Go 1.

Status changed to WaitingForReply.

@nsf
Copy link
Author

@nsf nsf commented Jan 18, 2012

Comment 3:

Ok, I can live with that.
@griesemer
Copy link
Contributor

@griesemer griesemer commented Jan 18, 2012

Comment 4:

Status changed to WorkingAsIntended.

@rsc rsc added this to the Go1 milestone Apr 10, 2015
@rsc rsc removed the priority-go1 label Apr 10, 2015
@golang golang locked and limited conversation to collaborators Jun 24, 2016
This issue was closed.
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
4 participants
You can’t perform that action at this time.