go/parser: isolate object resolution from parsing #45104
Comments
|
I'd love this. I assume the vast majority of the Parse API users could skip object resolution. It would certainly help with syntax-only tools like gofmt and gofumpt. I assume we can't change the default behavior because of the backwards compatibility guarantee. Have you thought about go/packages? Since it supports go/types, I imagine practically noone should be relying on go/parser's object resolution. |
Not really. There's a pretty big gap between practically noone and noone. Given that parsing is generally a fraction of type checking (5-15% perhaps), I'm not sure it's worth skipping object resolution if it adds complexity to the API. However, for tools such as gofumpt (which needs only syntax) or maybe gopls, which manages parsing independently of go/packages, there is an opportunity for performance gains. We certainly can't change the default behavior for either the parser or go/packages. Were the only benefit performance I'm not sure this would be worth doing. Also, in my prototype there appears to be a small but statistically significant cost of resolving in a post processing pass, simply due to the extra traversal -- maybe 2-5%. I'm sure I can make that cost up elsewhere if necessary :) |
That's true, but it's also true that a good portion of go/packages users load syntax without loading type information. Whether or not it's worth adding API complexity is a fair point.
I'd even go as far as saying that a <10% slow-down is okay. Anywhere where the added cost will really matter can likely just enable the flag to skip object resolution and get a speed-up instead. |
cherrymui
added
the
NeedsFix
|
Ok, now I've got everything matching the current behavior identically (after normalizing for bugs in the current implementation).
CLs incoming, once I can reassemble these changes into a sensible chain. |
|
Happy to help review these, though I assume you want gri's approval on them anyway. |
|
Change https://golang.org/cl/304455 mentions this issue: |
|
Change https://golang.org/cl/304453 mentions this issue: |
|
Change https://golang.org/cl/304450 mentions this issue: |
Feel free to review if you want, but yes I will require gri's approval :) |
Add new tests for object resolution driven by source files with declarations and uses marked via special comments. This made it easier to add test coverage while refactoring object resolution for #45104. Tests are added to a new resolver_test.go file. In a subsequent CL the resolver.go file will be added, making this choice of file name more sensible. For #45104 For #45136 For #45160 Change-Id: I240fccc0de95aa8f2d03e39c77146d4c61f1ef9e Reviewed-on: https://go-review.googlesource.com/c/go/+/304450 Trust: Robert Findley <[email protected]> Trust: Robert Griesemer <[email protected]> Run-TryBot: Robert Findley <[email protected]> Reviewed-by: Robert Griesemer <[email protected]> TryBot-Result: Go Bot <[email protected]>
For #45104 For #45221 Change-Id: I8966555f4e8844d5b6766d00d48f7a81868ccf40 Reviewed-on: https://go-review.googlesource.com/c/go/+/304453 Trust: Robert Findley <[email protected]> Trust: Robert Griesemer <[email protected]> Run-TryBot: Robert Findley <[email protected]> TryBot-Result: Go Bot <[email protected]> Reviewed-by: Robert Griesemer <[email protected]>
Coupling object resolution to parsing complicates the parsing code, and is a barrier to improvement. It requires passing around context such as 'lhs' or 'keyOk', and even then sometimes requires guess-work, such as whether to resolve the key in a composite literal. In this CL we delay object resolution to a separate pass after the file parse completes. This makes it easier to see logic of scoping, and removes state from the parsing code. This can enable subsequent improvements such as optionally skipping object resolution, aligning the parser with cmd/compile/internal/syntax, and allowing alternative parsers to reuse object resolution. The additional AST traversal appears to slow down parsing by around 4%. That seems small enough not to worry about, especially since performance sensitive users may eventually be able to disable object resolution entirely, saving around 18% off the previous baseline. I'll also mail a speculative CL showing how we can significantly mitigate the cost of object resolution by transposing scopes. For #45104 Change-Id: I98d9143fd77ae29e84ec7c3ae2fdb1139510da37 Reviewed-on: https://go-review.googlesource.com/c/go/+/304455 Trust: Robert Findley <[email protected]> Trust: Robert Griesemer <[email protected]> Run-TryBot: Robert Findley <[email protected]> TryBot-Result: Go Bot <[email protected]> Reviewed-by: Robert Griesemer <[email protected]>
|
Change https://golang.org/cl/306149 mentions this issue: |
|
We had a bit of discussion on #46298, and the addition of I also think it could be beneficial to more formally consider the change. As I wrote in that proposal, I think it's a small benefit for smaller cost, but I'd like to hear from others. If the discussion is one-sided in favor of the new mode, we can perhaps still add it for 1.17, but I don't want to rush it. |
This issue relates to an implementation detail of go/parser, but one that is fundamental enough to track independently.
Currently go/parser does a bit more than just parse and produce an AST: while parsing it creates *ast.Scopes and uses them to map identifiers to declarations (object resolution), storing the result of this resolution in ast.Ident.Obj, and reporting some declaration errors if, for example, there's no new identifier on the left side of ':='. There's a fair bit of overlap with go/types, which has its own more robust concept of scopes.
Furthermore:
40% of parsing timeI'm working on improving go/parser error recovery (and to a lesser extent, performance), and it's proving to be easier if identifier resolution is moved out of the parsing pass into a separate post-processing pass (thanks @griesemer for the suggestion).
At this point I have a working version of go/parser that achieves this, albeit hacked-up so that I can run in both paradigms and compare the result. Here's the plan forward:
SkipResolution).func Resolve(*ast.Node, *ast.Scope), so that alternate parsers can use the same logic to enrich the AST with objects.It's worth noting that gopls can't quite disable object resolution, as there are a few analyzers that use it.
The text was updated successfully, but these errors were encountered: