convert to valid identifier before checking for collision

[primoly]

Because the current to_*_ident functions check for collisions with keywords before converting to snake/camel case some edge cases are missed. For example with the following type in Wit:

record weird {
    CONST: u32,
    BREAK: u32,
}

wit-bindgen currently generates fields, attributes and parameters named const and break.

This commit also resolves the //TODO: Repace with actual keywords in the C# bindings generator.

[jsturtevant]

could you add a test case in https://github.com/bytecodealliance/wit-bindgen/tree/main/tests/codegen?

[jsturtevant]

@primoly looks like a few tests are failing with this change

[Mossaka]

It seems like the Go bindgen was broken for this change

[primoly]

The name generation for Go is quite a bit more involved due to the conversions and collision checks being done in different functions and files and handled differently on a case-by-case basis (keyword escapes/name mangling, snake/camel, Go/C). Because https://github.com/bytecodealliance/wit-bindgen/pull/784#issuecomment-2086354488 might supersede the bindgen here (?) I’m unsure if fixing this now would be worth the effort.

Also, the current camel case conversion, for example, turns serve-HTTP into ServeHttp (Rust convention) instead of ServeHTTP (Go convention).

Maybe it makes sense to just drop that test since it is very unlikely that all caps keywords will appear in Wit names since they are usually not acronyms. On the other hand I believe automated binding generators should generally be able to handle edge cases since these kinds of issues, while rare, are really difficult to debug when they do occur.

[jsturtevant]

On the other hand I believe automated binding generators should generally be able to handle edge cases since these kinds of issues, while rare, are really difficult to debug when they do occur.

I generally agree but lets get some input from @alexcrichton

[alexcrichton]

I definitely think this is a good test to add, thanks! Feel free to add ignores for other generators and maintainers can get around to supporting them after this PR lands.

[Mossaka]

Yeah feel free to ignore the tests for Go at the moment. I will take a step to fix them as soon as I got some time. (might be a good idea to create an issue to track since I sometimes forgot things)

The name generation for Go is quite a bit more involved due to the conversions and collision checks being done in different functions and files and handled differently on a case-by-case basis (keyword escapes/name mangling, snake/camel, Go/C).

I am hoping to do some refactoring work to make this centralized and easier to change.

Because https://github.com/bytecodealliance/wit-bindgen/pull/784#issuecomment-2086354488 might supersede the bindgen here (?) I’m unsure if fixing this now would be worth the effort.

I intend to still maintain this project to pass all the test cases since it's been used in several other projects, while waiting for the new wit-bindgen-go to land.