Style Guide for DSLX

[vincent-mirian-google]

Create a DSLX style guide: given that DSLX is Rust-based, the style guide should resemble Rust. There is an example of a C++ style guide here.

[mikex-oss]

I was reminded today of this because I ran into the Standard style is SCREAMING_SNAKE_CASE for constant identifiers error. It seemed surprising that DSLX enforces some style as errors when we don't have an explicit DSLX style guide. (Tangentially, I'd also argue style should be enforced by a linter and not a DSLX type system error.)

I understand this can be turned off with a waiver#![allow(nonstandard_constant_naming)]but it seems like this would be better codified in a style guide rather than as note describing a waiver. (https://google.github.io/xls/dslx_reference/#identifiers also mentions this but uses the phrasing suggest).

Another candidate for the style guide could be if/when #352 is deployed. It'd be good to call out if we follow Rust conventions on import style and if this is enforced by the front-end. In Rust, it seems to be idiomatic to import modules when using functions but to import symbols for other stuff like enums/structs/etc.: doc.rust-lang.org/stable/book/ch07-04-bringing-paths-into-scope-with-the-use-keyword.html#creating-idiomatic-use-paths. Just for contrast, with Python, Google Pystyle says to only import modules, not individual symbols so you always know where stuff comes from: https://google.github.io/styleguide/pyguide.html#22-imports.

[cdleary]

@mikex-oss A nit, maybe not a deep distinction but seemed worth mentioning -- it's a warning (collected by the warning collector) and we have warnings-as-errors on by default, which is why it's reported as an error.

Another small distinction, in the frontend design I'd claim we "happen to" fuse the analysis for warnings into typechecking since it's our one semantic analysis traversal for the frontend, so it's a convenient place to slot in. Could make it a separate traversal, but not sure it'd win very much, since the layers don't really affect each other with the WarningCollector observer-style design.

We could make a separate binary for linting, but seemed a bit overkill given the common use case is to not want style divergences unless there's a good reason (in which case we use the allow attribute at module scope) and the fact warnings-as-errors is generally on for production oriented languages.

Hope those are useful and not just pedantic to point out -- style guide seems like a good idea in the places we differ from Rust or have constructs that are different. Note that I believe rustc also checks these by default: https://users.rust-lang.org/t/this-warning-seems-a-bit-over-the-top-why-the-snake-lint/55986

   Compiling playground v0.0.1 (/playground)
warning: variable `nAmEdWeIrDlY` should have a snake case name
 --> src/main.rs:2:9
  |
2 |     let nAmEdWeIrDlY = 42;
  |         ^^^^^^^^^^^^ help: convert the identifier to snake case: `n_am_ed_we_ir_dl_y`
  |
  = note: `#[warn(non_snake_case)]` on by default

warning: `playground` (bin "playground") generated 1 warning
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 3.90s
     Running `target/debug/playground`
42

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=24894a87b8a8f4b1c0f279532d6b9ee8

[mikex-oss]

@cdleary you 100% know more about Rust than me, so thanks for pointing out that Rust also has some built-in warnings that enforce standard code style. But if I'm not mistaken, both Rust (--deny warnings) and C++ (-Werror in clang/gcc) are not enabled by default.

Another point worth mentioning is that unlike those languages, where the user generally does not care what the compiler output looks like (from a readability perspective), XLS users (or at least their partners) will likely care about the emitted Verilog.

@hongted has been working on a DSLX-to-Verilog types code generator where type naming actually came up. If the DSLX type is MyType, should the Verilog type also be MyType or my_type or my_type_t (common in SV to indicate the name refers to a typedef)? @meheff's preference is to not have special magic here, but then that means you have to sprinkle those attribute waivers all over if your Verilog style guide wants snake case. This wouldn't be a problem if the compiler wasn't opinionated about style and left it to a configurable linter.

Also, both C++ (clang-tidy) and Rust (rust-clippy I think?) have standalone linters, so even if we leave some style warnings in the compiler, could be worth a separate, more powerful linter for DSLX as well.

[cdleary]

@mikex-oss I was trying to avoid naming it directly, but I do believe warnings-as-errors is the default policy in g3, which is where this "why on by default" stemmed from :-) It's a nice property of code bases to not be throwing standard warnings so better to consider whether the errors should be default-on (vs opt in) than to be disabling warnings-as-errors by default IMO.

I think if g3 wanted to disable this warning by default we could make a DSLX config file that the interpreter read from a canonical location similar to how there's rust_toolchain.toml (and for other examples crosstool and clang-tidy configures the g3 set of default issues and their severity internally).

I think it's useful/important to guide new/early users on what the normal style one might expect is and then let people opt into their exceptional cases as they presumably have more familiarity with the trade-offs they're making at that point. (Although this is just a point towards "default on is good", and then there's the subsequent consideration of "code bases giving warnings that are just ignored is not great", as you pointed out those can be peeled apart.)