Skip to content

Add lint:rust command and resolve various clippy linting warnings - #414

Open
mrxz wants to merge 3 commits into
sparkjsdev:mainfrom
mrxz:rust-linting
Open

Add lint:rust command and resolve various clippy linting warnings#414
mrxz wants to merge 3 commits into
sparkjsdev:mainfrom
mrxz:rust-linting

Conversation

@mrxz

@mrxz mrxz commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator

Currently only the TypeScript code is automatically linted and formatted, whereas the Rust code is left as-is. Ideally we'd apply the same automatic process to both. This helps avoid common mistakes/pitfalls and ensures consistent code formatting. Related #370

This PR introduces a lint:rust script to easily execute clippy. This is not included in the pre-commit hook nor in any of the CI pipelines yet. In order for that to be useful, all pre-existing linting warnings should be resolved (either fixed or explicitly allowed).

Additionally various linter warnings have been resolved. Though the following rules have been skipped as they (subjectively) reduce the readability or are non-trivial:

Rule name Notes
clippy::identity_op Many instances where a consecutive set of elements is computed/referenced, in which case the + 0 is nice for symmetry.
clippy::len_without_is_empty
clippy::new_without_default
clippy::needless_range_loop
clippy::excessive_precision Some of the SH constants trigger this warning, but since these literally match the JS and GLSL code, leaving them unchanged is preferred
clippy::too_many_arguments Part of the exported functions of the WASM blob, so changing this would require further changes
clippy::let_and_return See #370 (comment)
clippy::clone_on_copy
clippy::type_complexity
clippy::wrong_self_convention
clippy::collapsible_if
clippy::needless_return In case of early returns in the same method body, I'd have a slight preference in consistently using return
clippy::some_filter
clippy::large_enum_variant
clippy::only_used_in_recursion
clippy::manual_range_contain Frankly the explicit `version < 1

The needless_range_loop is interesting in the regard that it can help avoid bound checks. Though quite often the index is still needed for offset in the source or destination vectors. The resulting code would not always be as easy to read, especially when the iterated collection is the destination and not the source.

In any case, I do think this is a good start. Follow up steps would be to settle on which linting rules to include and which to exclude as well as integrating it in the CI pipelines + commit hook (preferably only if/when rust code has changed given how slow these Rust tools are).

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.

1 participant