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.

Sign up for GitHub

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

Jump to bottom

[naga] Comments parsing #6364

Draft
wants to merge 8 commits into
base: v22
Choose a base branch
from
Draft

[naga] Comments parsing #6364

wants to merge 8 commits into from

Conversation

Vrixyz
Copy link

@Vrixyz Vrixyz commented Oct 3, 2024 •
edited
Loading

Connections
None as far as I know.

Description
Generating automated documentation à la https://docs.rs is very useful, but currently difficult.

naga lacks information about comments to be too helpful, so this pull requests adds support to parsing those.

Note: I branched off 0.22 because I'm not confident with jumping on the maybe unstable trunk.

Testing
Not much for now, I'm compiling a currently private project based on https://github.com/jannik4/shader_docs to attempt to add documentation strings to it.

Status

  • Add a token Comment
  • Parse // comments
  • Parse /* */ comments
  • Add comments to ast::Struct
  • Support comments for multiple items
    • struct
      • struct members
    • var (global variables)
    • consts
    • functions
    • module level comment
    • alias ; 🚫 I consider this out of scope for this PR.
  • Add comments list in naga::Module

Checklist

  • Run cargo fmt.
  • Run cargo clippy. If applicable, add:
    • --target wasm32-unknown-unknown
    • --target wasm32-unknown-emscripten
  • Run cargo xtask test to run tests.
    • ⚠️ I'm seeing a lot of failures, but I'm not sure how to debug exactly 🤔 ; ideally we could isolate the problematic shader(s) and make a smaller test.
Test failures

Reminder: I'm branched off tag wgpu-v22.1.0, which had mopre flaky tests.

New failing tests 

>      SIGABRT wgpu-examples [Executed Flaky Failure: ALWAYS] [Vulkan/AMD Radeon Graphics (RADV RAPHAEL_MENDOCINO)/1] wgpu_examples::timestamp_queries::tests::timestamps_encoder
>      SIGSEGV wgpu-test::wgpu-test [Executed] [Gl/AMD Radeon RX 7600 (radeonsi, navi33, LLVM 15.0.7, DRM 3.57, 6.9.3-76060903-generic)/3] wgpu_test::scissor_tests::scissor_test_custom_rect
>         FAIL wgpu-test::wgpu-test [Executed] [Gl/AMD Radeon RX 7600 (radeonsi, navi33, LLVM 15.0.7, DRM 3.57, 6.9.3-76060903-generic)/3] wgpu_test::shader::compilation_messages::shader_compile_error
>      SIGSEGV wgpu-test::wgpu-test [Executed] [Gl/AMD Radeon RX 7600 (radeonsi, navi33, LLVM 15.0.7, DRM 3.57, 6.9.3-76060903-generic)/3] wgpu_test::shader_primitive_index::draw_indexed
>      SIGABRT wgpu-test::wgpu-test [Executed] [Vulkan/AMD Radeon Graphics (RADV RAPHAEL_MENDOCINO)/1] wgpu_test::device::device_lifetime_check
>         FAIL wgpu-test::wgpu-test [Executed] [Vulkan/AMD Radeon Graphics (RADV RAPHAEL_MENDOCINO)/1] wgpu_test::shader::compilation_messages::shader_compile_error
>      SIGABRT wgpu-test::wgpu-test [Executed] [Vulkan/AMD Radeon RX 7600 (RADV NAVI33)/0] wgpu_test::bind_group_layout_dedup::bind_group_layout_deduplication_derived
>      SIGABRT wgpu-test::wgpu-test [Executed] [Vulkan/AMD Radeon RX 7600 (RADV NAVI33)/0] wgpu_test::bind_group_layout_dedup::bind_group_layout_deduplication_with_dropped_user_handle
>      SIGABRT wgpu-test::wgpu-test [Executed] [Vulkan/AMD Radeon RX 7600 (RADV NAVI33)/0] wgpu_test::bind_group_layout_dedup::derived_bgls_incompatible_with_regular_bgls
>      SIGABRT wgpu-test::wgpu-test [Executed] [Vulkan/AMD Radeon RX 7600 (RADV NAVI33)/0] wgpu_test::bind_groups::bind_group_nonfiltering_layout_mipmap_sampler
>      SIGABRT wgpu-test::wgpu-test [Executed] [Vulkan/AMD Radeon RX 7600 (RADV NAVI33)/0] wgpu_test::buffer::clear_offset_outside_resource_bounds
>      SIGABRT wgpu-test::wgpu-test [Executed] [Vulkan/AMD Radeon RX 7600 (RADV NAVI33)/0] wgpu_test::buffer::clear_offset_plus_size_outside_u64_bounds
>      SIGABRT wgpu-test::wgpu-test [Executed] [Vulkan/AMD Radeon RX 7600 (RADV NAVI33)/0] wgpu_test::buffer::minimum_buffer_binding_size_layout
>      SIGABRT wgpu-test::wgpu-test [Executed] [Vulkan/AMD Radeon RX 7600 (RADV NAVI33)/0] wgpu_test::buffer_usages::buffer_usage_mappable_primary_buffers
>      SIGABRT wgpu-test::wgpu-test [Executed] [Vulkan/AMD Radeon RX 7600 (RADV NAVI33)/0] wgpu_test::compute_pass_ownership::compute_pass_keep_encoder_alive
>      SIGABRT wgpu-test::wgpu-test [Executed] [Vulkan/AMD Radeon RX 7600 (RADV NAVI33)/0] wgpu_test::device::device_destroy_then_buffer_cleanup
>      SIGABRT wgpu-test::wgpu-test [Executed] [Vulkan/AMD Radeon RX 7600 (RADV NAVI33)/0] wgpu_test::device::device_destroy_then_more
>      SIGABRT wgpu-test::wgpu-test [Executed] [Vulkan/AMD Radeon RX 7600 (RADV NAVI33)/0] wgpu_test::instance::initialize
>      SIGABRT wgpu-test::wgpu-test [Executed] [Vulkan/AMD Radeon RX 7600 (RADV NAVI33)/0] wgpu_test::poll::wait_after_bad_submission
>      SIGABRT wgpu-test::wgpu-test [Executed] [Vulkan/AMD Radeon RX 7600 (RADV NAVI33)/0] wgpu_test::regression::issue_3457::pass_reset_vertex_buffer
>         FAIL wgpu-test::wgpu-test [Executed] [Vulkan/AMD Radeon RX 7600 (RADV NAVI33)/0] wgpu_test::shader::compilation_messages::shader_compile_error
>         FAIL wgpu-test::wgpu-test [Executed] [Vulkan/llvmpipe (LLVM 15.0.7, 256 bits)/2] wgpu_test::shader::compilation_messages::shader_compile_error

shader_compile_error seems expected as it explicitly handles comments, so we probably want to change the test,
but I'm out of my comfort zone to make more analysis, so I'd appreciate instructions :)

  • Add change to CHANGELOG.md. See simple instructions inside file.

@Vrixyz Vrixyz requested a review from a team as a code owner October 3, 2024 15:56
@Vrixyz Vrixyz marked this pull request as draft October 3, 2024 15:56
@jimblandy
Copy link
Member

This doesn't get the comments out of the front end, though, does it?

It seems to me it could be fine to just add a field like:

    doc_comments: Option<Box<DocComments>>,

to naga::Module, and then have something like

struct DocComments {
    types: FastIndexMap<Handle<Type>, Vec<Span>>,
    global_variables: FastIndexMap<Handle<GlobalVariable>, Vec<Span>>,
    ...
}

I think it would make sense for Naga to adopt Rust's /// syntax as a WGSL extension.

@Vrixyz
Copy link
Author

Vrixyz commented Oct 4, 2024

This doesn't get the comments out of the front end, though, does it?

Correct, I'm looking into propagating them to the module now ; my first intuition was to add comments to naga::TypeInner::Struct and the likes ; but I fear for performance.

Your idea of a "flat" DocComments with an indexmap to Spans is interesting and probably a better idea! Thanks for suggesting. I'll go in that direction.

@Vrixyz Vrixyz mentioned this pull request Oct 4, 2024
Draft
@Vrixyz Vrixyz mentioned this pull request Oct 9, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Vrixyz @jimblandy