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

feat: Codegen plugins, powered by WASM #1470

Closed
wants to merge 6 commits into from
Closed

Conversation

kyleconroy
Copy link
Collaborator

Replaces #1414

@kyleconroy kyleconroy marked this pull request as draft March 3, 2022 16:43
}

func init() { file_plugin_codegen_proto_init() }
func file_plugin_codegen_proto_init() {
return //XXX
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tracking here: tinygo-org/tinygo#2667

@@ -2606,6 +2606,7 @@ var file_python_ast_proto_depIdxs = []int32{

func init() { file_python_ast_proto_init() }
func file_python_ast_proto_init() {
return
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tracking here: tinygo-org/tinygo#2667

Comment on lines +103 to +108
message WASM
{
string path = 1;
string out = 2;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a temporary addition to make it easy to test out plugin support. I haven't figured out what the plugin config should look like yet.

@skabbes
Copy link
Contributor

skabbes commented Apr 2, 2022

I know you're probably not wanting / expecting feedback on this. But in the spirit of the internet, here's unsolicited feedback haha...

I think wasm is really nice to support out of the box, but producing json output over stdout is probably a bit more approachable for most code-gen plugin authors (and it opens it up to languages not supported by wasm easily). Though supporting wasm plugins would also be trivial to support with a generic wasm runner.

# illustrative
# sqlc generate --protobuf > sqlcoutput.pb 
sqlc generate --file sqlc.json --json > sqlcoutput.json 
cat sqlcoutput.json | gen-go-code
cat sqlcoutput.json | gen-ts-code
cat sqlcoutput.json | wasm-runner ./python-gen.wasm

That said if your goal is here to to scratch an engineering itch you have - don't let me stop you!

@skabbes
Copy link
Contributor

skabbes commented Apr 23, 2022

@kyleconroy can you clarify (just for my curiosity) what TinyGo has to do with wasm plugins? Is it because you are trying to write the Go and Python code generators in wasm (in Go, compiled to wasm)? And only TinyGo supports wasm??

Or are you envisioning a world where all of sqlc can run in wasm (maybe for security or something like that?).

@skabbes
Copy link
Contributor

skabbes commented Apr 24, 2022

FYI I did a little digging in the tinygo repo, it appears like you can fix your defer atomic.StoreUint32 by specifically building a little shim for the symbol. I did a little proof of concept, and it seems like you can do this completely outside of tinygo (until they can fix the bug the right way), by adding this little bit of code anywhere in your project. Although that said, I think the fix is going to be relatively easy for someone familiar with the tinygo codebase - there are some other examples of doing similar things - and maybe they just need a little pushing.

//go:build tinygo

package main

import (
	"sync/atomic"
	_ "unsafe" // needed for go:linkname
)

//go:linkname bugfixSyncAtomicStoreUint32 sync/atomic.StoreUint32
func bugfixSyncAtomicStoreUint32(addr *uint32, val uint32) {
	// this will get rewritten by tinygo into llvm atomic store
	atomic.StoreUint32(addr, val)
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants