wails icon indicating copy to clipboard operation
wails copied to clipboard

[v3] Binding generator using `go/types`

Open abichinger opened this issue 1 year ago • 12 comments

Description

This PR continues the work done by @fbbdev in https://github.com/wailsapp/wails/pull/3347. I used this commit 241c578c7abe6e8cd916e561f626bb53ce275340 as a starting point and modified the binding and model generator code to work with the go/types package. Using go/types instead of go/ast fixes the problems with object resolution. https://github.com/wailsapp/wails/pull/3347#issuecomment-2026294489 https://github.com/golang/go/issues/48141#issuecomment-912126040

Breaking changes

  • The identifier of a bound method is now packagePath.structName.methodName instead of packageName.structName.methodName to avoid collisions (a70289787eeb7dc7b146e885b19340ebf01a88f2)
  • GenerateBindingsOptions.UseIDs got replaced by GenerateBindingsOptions.UseNames making CallByID the new default (aa098b73a69bbcc01d7f341b20fc5d6fcc764466)
  • I changed the interface of the parser package

New features

  • It is possible to bind structs from external packages. An example can be found at wails/v3/internal/parser/testdata/multiple_packages. You can run this example with go run .
  • go types that are not structs or "enums" generate a Typescript alias type: https://github.com/abichinger/wails/blob/1660a12ba2d0f20e2ab5698b4156432c999b043f/v3/internal/parser/testdata/complex_json/frontend/bindings/main/models.ts#L5
  • Support types implementing the encoding.TextMarshaler interface. The resulting Typescript type is string. https://github.com/abichinger/wails/blob/1660a12ba2d0f20e2ab5698b4156432c999b043f/v3/internal/parser/testdata/multiple_packages/frontend/bindings/github.com-google-uuid/models.ts#L9

Fixes

  • Use the first constant value of an Enum as default
  • The JSDoc enum definition was missing the <type> parameter
  • The ID of bound methods from a nested package was previously incorrect

Limitations

  • iota is not supported to define enums
  • It is not possible to bind methods, which accept an interface as parameter
  • Interface fields inside structs are dropped
  • Generic types are ignored
  • Can not bind packages
  • Unable to predict fields of types, which implement json.Marshaler

Downsides

  • The new binding generator is significantly slower. In my test project, generation takes 243ms (18 times slower than https://github.com/wailsapp/wails/pull/3347)

Type of change

  • [x] Bug fix (non-breaking change which fixes an issue)
  • [x] New feature (non-breaking change which adds functionality)
  • [x] Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • [ ] This change requires a documentation update

How Has This Been Tested?

All tests have passed. Some expected results of tests inside v3/internal/parser/testdata have been corrected. The binding example has been tested and updated. The bindings of a test project have been generated.

  • [ ] Windows
  • [ ] macOS
  • [x] Linux

Test Configuration

# System
┌───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐
| Name         | Manjaro Linux                                                                                                  |
| Version      | Unknown                                                                                                        |
| ID           | manjaro                                                                                                        |
| Branding     |                                                                                                                |
| Platform     | linux                                                                                                          |
| Architecture | amd64                                                                                                          |
| CPU          | AMD Ryzen 5 3600 6-Core Processor                                                                              |
| GPU 1        | Navi 10 [Radeon RX 5600 OEM/5600 XT / 5700/5700 XT] (Advanced Micro Devices, Inc. [AMD/ATI]) - Driver: amdgpu  |
| Memory       | 32GB                                                                                                           |
└───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘

# Build Environment
┌─────────────────────────────────────────────────────────────────────────────────────────────────┐
| Wails CLI      | v3.0.0-alpha.4                                                                 |
| Go Version     | go1.22.0                                                                       |
| Revision       | 43418e96f36455467657822f8487d8a567dfda17                                       |
| Modified       | true                                                                           |
| -buildmode     | exe                                                                            |
| -compiler      | gc                                                                             |
| CGO_CFLAGS     |                                                                                |
| CGO_CPPFLAGS   |                                                                                |
| CGO_CXXFLAGS   |                                                                                |
| CGO_ENABLED    | 1                                                                              |
| CGO_LDFLAGS    |                                                                                |
| DefaultGODEBUG | httplaxcontentlength=1,httpmuxgo121=1,tls10server=1,tlsrsakex=1,tlsunsafeekm=1 |
| GOAMD64        | v1                                                                             |
| GOARCH         | amd64                                                                          |
| GOOS           | linux                                                                          |
| vcs            | git                                                                            |
| vcs.modified   | true                                                                           |
| vcs.revision   | 43418e96f36455467657822f8487d8a567dfda17                                       |
| vcs.time       | 2024-04-15T14:26:43Z                                                           |
└─────────────────────────────────────────────────────────────────────────────────────────────────┘

# Dependencies
┌───────────────────────────┐
| pkg-config | 2.1.0-2      |
| gcc        | 13.2.1-5     |
| libgtk-3   | 1:3.24.41-1  |
| libwebkit  | 2.42.5-2     |
| npm        | 10.5.0       |
└─ * - Optional Dependency ─┘

# Diagnosis
 SUCCESS  Your system is ready for Wails development!

Checklist:

  • [ ] I have updated website/src/pages/changelog.mdx with details of this PR
  • [x] My code follows the general coding style of this project
  • [x] I have performed a self-review of my own code
  • [ ] I have commented my code, particularly in hard-to-understand areas
  • [ ] I have made corresponding changes to the documentation
  • [x] My changes generate no new warnings
  • [x] I have added tests that prove my fix is effective or that my feature works
  • [x] New and existing unit tests pass locally with my changes

abichinger avatar Apr 15 '24 18:04 abichinger

[!IMPORTANT]

Auto Review Skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

coderabbitai[bot] avatar Apr 15 '24 18:04 coderabbitai[bot]

Hello!

Thank you for this and sorry to everyone for disappearing. As I told @leaanthony, I was swamped in other work for some time.

@leaanthony suggested that we coordinate to compare and integrate our work.

The situation might not be that easy on my part, as I have quite a lot of unpublished work that diverged from what you have now. I essentially rewrote my own static analyser from scratch, altering its API significantly and fixing some mistakes in the process. I also had in mind a slightly different (and parallelised) architecture for the generator, and I already laid the groundwork for the part that collects information and renders types. I hope parallelisation could take some time off the generation step on multi-core machines.

I had a cursory look at your implementation. I like how you dealt with documentation, it didn't occur to me to use the doc package. I would definitely integrate that. I didn't have the time to look at the parser and tests yet.

If you and @leaanthony both agree, I suggest that we proceed as follows. I would like to still bring my current work to completion and publish that to my fork. Then we might get in contact and compare our code w.r.t. functionality and architecture, and proceed to integrate everything with @leaanthony 's feedback.

What do you think?

fbbdev avatar Apr 15 '24 20:04 fbbdev

Hi!

Sounds good to me! I'm excited to see how much you can improve the performance by using parallelisation.

I like how you dealt with documentation, it didn't occur to me to use the doc package.

Thanks! Unfortunately, some comments still have to be read from the syntax tree.

abichinger avatar Apr 16 '24 11:04 abichinger

I managed to improve the performance a bit. The previously mentioned test project now takes about 115ms (before: 243ms) to process. The main performance gain comes from calling LoadPackages() only once instead of multiple times.

During benchmarking, I noticed that ParseProject(=LoadPackages) is by far the most time consuming step. I glanced over the source code of packages [packages.go;l=849] and I think it already runs concurrently. Unfortunately, improving the packages.Load function is definitely beyond my skill set. So I guess I can not improve this PR much further.

Running tool: /usr/bin/go test -benchmem -run=^$ -bench ^BenchmarkParser$ github.com/wailsapp/wails/v3/internal/parser

goos: linux
goarch: amd64
pkg: github.com/wailsapp/wails/v3/internal/parser
cpu: AMD Ryzen 5 3600 6-Core Processor              
BenchmarkParser/struct_literal_single/ParseProject-12         	       9	 122664394 ns/op	 6680450 B/op	   59841 allocs/op
BenchmarkParser/struct_literal_single/ParsePackages-12        	    7963	    244462 ns/op	   79549 B/op	    1532 allocs/op
BenchmarkParser/struct_literal_single/GenerateBindings-12     	     751	   1475458 ns/op	  191068 B/op	    5965 allocs/op
BenchmarkParser/struct_literal_single/GenerateModels-12       	    5888	    220428 ns/op	   32871 B/op	     792 allocs/op
BenchmarkParser/complex_json/ParseProject-12                  	       9	 121939557 ns/op	 6273351 B/op	   56070 allocs/op
BenchmarkParser/complex_json/ParsePackages-12                 	   10000	    122095 ns/op	   34350 B/op	     723 allocs/op
BenchmarkParser/complex_json/GenerateBindings-12              	   18594	     65682 ns/op	    8942 B/op	     241 allocs/op
BenchmarkParser/complex_json/GenerateModels-12                	    1755	    614594 ns/op	   85877 B/op	    2502 allocs/op
BenchmarkParser/multiple_packages/ParseProject-12             	       9	 117322676 ns/op	 8458869 B/op	   69577 allocs/op
BenchmarkParser/multiple_packages/ParsePackages-12            	      98	  12656978 ns/op	  831900 B/op	   18418 allocs/op
BenchmarkParser/multiple_packages/GenerateBindings-12         	    3052	    516789 ns/op	   86948 B/op	    2411 allocs/op
BenchmarkParser/multiple_packages/GenerateModels-12           	    1986	    523006 ns/op	   80129 B/op	    1954 allocs/op
PASS
ok  	github.com/wailsapp/wails/v3/internal/parser	20.733s

BenchmarkParser/multiple_packages/ParsePackages takes 13ms, because ParsePackages loads the documentation for each struct coming from an imported package.

abichinger avatar Apr 17 '24 19:04 abichinger

Hey, just want to give some feedback, I have quite complicated models and this is first version of parser that managed to parse it and generate TS models correctly. Only error types are still problem, it seems go/types does not handle this.

I have also fixed parse so call to application.New can be anywhere, not only in main. Seems it works :D

overlordtm avatar Apr 19 '24 10:04 overlordtm

@overlordtm - It would be great if you could contribute to this PR 🙏

leaanthony avatar Apr 19 '24 11:04 leaanthony

Thank you for testing!

Only error types are still problem, it seems go/types does not handle this.

@overlordtm could you provide an example?

abichinger avatar Apr 19 '24 14:04 abichinger

@abichinger - do you have a feature list for this enhancement? I'm thinking this will help with the integration of @fbbdev 's work.

leaanthony avatar Apr 20 '24 06:04 leaanthony

Apart from the bugfixes and features I already mentioned above, this PR is functionally identical to https://github.com/wailsapp/wails/pull/3347.

I think it is best to wait for @fbbdev to publish his new architecture and then we can compare.

abichinger avatar Apr 20 '24 11:04 abichinger

I spoke to @fbbdev earlier and he suggested closing the other PR and concentrating efforts on this, so that's what I'll do.

leaanthony avatar Apr 20 '24 11:04 leaanthony

I would appreciate some feedback on the generated package directories.

The current behaviour is as follows.

  1. Services and models inside the project directory will be placed inside main.
  2. Each subpackage in the project directory ends up in the same subfolder as in the project.
  3. External packages get placed inside a folder identical to their package path (/ is replaced with -)

Let's asume our project directory has the package path github.com/alice/foo. Services and models defined inside ...

  • github.com/alice/foo will end up in main
  • github.com/alice/foo/bar will end up in bar
  • github.com/alice/foo/bar/baz will end up in bar/baz
  • log (standard package) will end up in log
  • github.com/google/uuid will end up in github.com-google-uuid

The current solution could still cause collisions. For example, a package log inside the project could collide with the standard log package.

Do you consider the current solution to be okay?

Update 2024-04-21

With 3be360692c6fb7e10cf483c1b99f75ea1f2c48ca the generated package directories are somewhat configurable. The wails3 generate bindings command now has two additional options.

-base string
        The base package path (default ".")
-n    Place the base package bindings inside a directory with the base package name

Here are some examples:

Results for executing wails3 generate bindings -d="." inside github.com/alice/foo:

  • github.com/alice/foo -> .
  • github.com/alice/foo/bar -> bar
  • github.com/alice/foo/bar/baz -> bar/baz
  • log (standard package) -> log
  • github.com/google/uuid -> github.com/google/uuid

Results for executing wails3 generate bindings -d . -n inside github.com/alice/foo with package name main:

  • github.com/alice/foo -> main
  • everything else stays the same

Results for executing wails3 generate bindings -d . -base github.com/alice inside github.com/alice/foo:

  • github.com/alice/foo -> foo
  • github.com/alice/foo/bar -> foo/bar
  • github.com/alice/foo/bar/baz -> foo/bar/baz
  • log (standard package) -> log
  • github.com/google/uuid -> github.com/google/uuid

abichinger avatar Apr 20 '24 14:04 abichinger

Awesome work 😎

I've created a post on discord to discuss the ains/strategy of the parser which I think would be a better forum than GH.

leaanthony avatar Apr 21 '24 20:04 leaanthony

@abichinger - Can we close this in favour of https://github.com/wailsapp/wails/pull/3468 now? @overlordtm - It would be amazing if you could let us know if the next iteration of the parser (linked above ☝️ ) works for your scenarios 🙏

leaanthony avatar May 12 '24 08:05 leaanthony

Closing in favour of #3468

abichinger avatar May 12 '24 10:05 abichinger