Skip to content

feat(allowlist): add Shader Languages support - #975

Open
wakqasahmed wants to merge 3 commits into
alibaba:mainfrom
wakqasahmed:feat/issue-470-shader-languages-support
Open

feat(allowlist): add Shader Languages support#975
wakqasahmed wants to merge 3 commits into
alibaba:mainfrom
wakqasahmed:feat/issue-470-shader-languages-support

Conversation

@wakqasahmed

Copy link
Copy Markdown
Contributor

Summary

  • Add .glsl, .hlsl, .wgsl, .metal to the supported file types allowlist
  • No default exclude pattern added — shader source files have no conventional test-directory layout shared across GLSL/HLSL/WGSL/Metal toolchains, so allowed_ext_test.go documents this explicitly (matching the existing Prisma precedent)
  • Add internal/config/rules/rule_docs/shader.md, a shared shader review rule doc covering precision/NaN propagation, texture and buffer bounds safety (including non-uniform control flow around texture sampling and compute writes), cross-stage binding/layout contracts (std140/std430, register/space, @binding/@group), compute-shader synchronization (barriers, race conditions), GPU performance anti-patterns (dynamic branching, texture-dependent reads), and security/portability concerns (untrusted buffer indices, vendor-specific intrinsics without fallback)
  • Register **/*.{glsl,hlsl,wgsl,metal} -> shader.md in system_rules.json
  • Add test coverage: allowed-extension matching (all four extensions, case-insensitive), non-exclusion of shader files, and rule resolution

Part of the language-allowlist expansion tracked in #470.

Closes #974

Test plan

  • go test ./internal/config/allowlist/... -run 'TestIsAllowedExt|TestIsExcludedPath' -v — PASS
  • go test ./internal/config/rules/... -run TestResolve_DefaultRules -v — PASS
  • gofmt -s -l . — clean
  • go vet ./internal/config/... — clean
  • go run scripts/verify-english-only.go — clean (423 files scanned, no unapproved non-English text)

Add .glsl/.hlsl/.wgsl/.metal to the supported file types allowlist,
a shared shader.md review rule doc covering precision/NaN handling,
texture and buffer bounds safety, cross-stage binding layout
contracts, compute-shader synchronization, and GPU-specific
performance/security pitfalls, and register the glob-to-rule mapping
in system_rules.json. No conventional test-file exclusion pattern
exists for shader source files, matching the tracking issue's guidance.

Part of the language-allowlist expansion tracked in alibaba#470.
@github-actions

Copy link
Copy Markdown
Contributor

OpenCodeReview: Review complete: 0 finding(s) across 2 selected item(s).

@wu21-web wu21-web left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Site documentation updates missing, update at pages/src/content/docs/<locale>/review-rules.md.

@wakqasahmed

Copy link
Copy Markdown
Contributor Author

@wu21-web Thanks for the catch — docs added in the latest commit. Updated pages/src/content/docs/{en,ja,ru,zh}/review-rules.md with a row for the new **/*.{glsl,hlsl,wgsl,metal}shader.md mapping, mirroring the format used for the Elm entry (#978).

Note: the ja/ru/zh translations are my best effort — please have a native speaker double-check the phrasing before merge.

@wu21-web

Copy link
Copy Markdown
Contributor

@wakqasahmed I will review jp and zh for you, but please reslove the merge conflicts first.

…-languages-support

# Conflicts:
#	internal/config/allowlist/allowed_ext_test.go
#	internal/config/allowlist/supported_file_types.json
#	internal/config/rules/system_rules.json
#	internal/config/rules/system_rules_test.go
#	pages/src/content/docs/en/review-rules.md
#	pages/src/content/docs/ja/review-rules.md
#	pages/src/content/docs/ru/review-rules.md
#	pages/src/content/docs/zh/review-rules.md
@wakqasahmed

Copy link
Copy Markdown
Contributor Author

@wu21-web Resolved — rebased onto latest main and merged, fixing the conflicts in supported_file_types.json, system_rules.json, the two allowlist/rules test files, and all four locale review-rules.md docs (they were conflicting with the recently-merged jsonnet/zig/thrift/capnp entries). Ready for the ja/zh review whenever you get a chance.

@wu21-web wu21-web left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approved. Why did you say "the ja/ru/zh translations are my best effort" 🤔 , they didn't need a review really. Good job though.

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.

feat(allowlist): add Shader Languages support

2 participants