[naga] Comments parsing (naga + wgsl) #6364

Vrixyz · 2024-10-03T15:56:18Z

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.

Testing

wgmath documentation uses Support comments when generating documentation jannik4/shader_docs#1 to add documentation strings to it, it's based on an older version of this PR, based on wgpu 0.22 branch.
A cargo xtask test run doesn't yield more errors than trunk.

Status

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.
Add change to CHANGELOG.md. See simple instructions inside file.

jimblandy · 2024-10-04T03:28:25Z

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 · 2024-10-04T08:02:42Z

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.

jimblandy

Some more comments here.

You'll also need to change compact::compact to adjust the type handles, even though only named types can have comments and compaction never removes named types, because compaction may remove anonymous types, and that renumbers the handles.

jimblandy · 2025-01-07T19:33:28Z

naga/src/lib.rs

@@ -2263,4 +2263,27 @@ pub struct Module {
    pub functions: Arena<Function>,
    /// Entry points.
    pub entry_points: Vec<EntryPoint>,
+    /// Comments, usually serving as documentation
+    pub comments: Comments,


I think this should be an Option<Box<Comments>>, so that users who aren't interested in collecting these comments don't need to spend memory on them.

I did the change ; we could also add a feature for that ?

Maybe I didn't really say what I meant:

I think users should be able to choose to collect or ignore comments at run time. That is, naga::front::wgsl::FrontEnd::new should take an Options type, or FrontEnd should have a new_with_comments constructor - or whatever, something boring but tasteful - and then parsing should return a Module whose comments field is either populated or not.

Thanks ! I addressed this in 57865be ; I'm open to suggestions about the implementation.

I updated a few tests to test both options.

naga/src/lib.rs

naga/src/front/wgsl/tests.rs

naga/src/lib.rs

naga/src/valid/handles.rs

naga/src/front/wgsl/parse/lexer.rs

naga/src/front/wgsl/parse/mod.rs

jimblandy · 2025-01-08T00:07:43Z

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

It should be fine to work on trunk. This PR should be rebased on that.

jimblandy · 2025-01-08T03:33:33Z

@Vrixyz Since this isn't passing CI, to make it easier to track stuff I need to review, I'm going to convert this to a draft PR until those issues are sorted out. Feel free to flip it back into non-draft when you're ready.

Vrixyz · 2025-01-09T10:36:11Z

I rebased on trunk (github comment reviews are degraded, but I adressed those 🤞), tests are encouraging, there's a major bug I'm investigating still (validation_error_messages): I think spans are incorrect, which I hope to fix by working on Jim's feedback.

❓ I'm curious about "pushing rules": I imagine parsing should contain a rule for parsing comments ? so we push that rule when checking for comments.

naga/src/front/wgsl/parse/mod.rs

jimblandy · 2025-01-10T21:58:33Z

❓ I'm curious about "pushing rules": I imagine parsing should contain a rule for parsing comments ? so we push that rule when checking for comments.

You mean Parser::rules? You shouldn't need to worry about that at all. You should just call start_byte_offset and span_from directly.

The rule stack is currently being used as a rough approximation to WGSL's template list discovery, which you definitely do not want to get tangled up with, and which shouldn't affect your work at all. And it's used for some sanity checks.

comments support

…est in naga/tests/in

…th comments parsing enabled.

Vrixyz · 2025-02-25T09:51:58Z

Let's go with the Rust syntax. Can we take care of it in this PR?

I did that in 9cb6d53 👍

teoxoy · 2025-04-11T10:54:23Z

naga/src/front/wgsl/parse/lexer.rs

+            } else if let Token::Trivia = token {
+                self.input = rest;
+            } else if let Token::CommentDocModule(_) = token {
+                self.input = rest;


Should it be valid to have these items in-between the doc comment and the item itself? It seems odd to me.

you're probably right, I thought it wouldn't hurt to not fail, but rust does indeed raise an error if a module comment is at the wrong place. I can do that.

What about Token::Trivia (comments) does rust allow those in-between doc comments?

naga/src/front/wgsl/parse/mod.rs

teoxoy · 2025-04-11T10:59:23Z

naga/src/front/wgsl/parse/lexer.rs

+                    let mut constructing_token = if !save_comments {
+                        Token::Trivia
+                    } else {
+                        let mut peeker = char_indices.clone().peekable();


We can make char_indices peekable, avoiding the clone.

char_indices is actually an iterator (from rustlib), so the clone is not expensive. We could make the api more ergonomic but ultimately I think that would lead to the same generated code.

That's fair, I should have mentioned my main concern was about readability as it took me a while to figure out what the code was doing, though I realize making char_indices peekable doesn't improve the flow much.

Actually, we are already cloning the iterator, we shouldn't need to make it peekable at all, we can just call next on it.

teoxoy · 2025-04-11T11:02:37Z

naga/src/front/wgsl/parse/mod.rs

@@ -1182,6 +1200,9 @@ impl Parser {
                    ExpectedToken::Token(Token::Separator(',')),
                ));
            }
+            // Save a lexer to be able to backtrack comments if need be.
+            let mut lexer_comments = lexer.clone();


We can then just call accumulate_doc_comments here instead.

The reason I went this route was because not all items should support comments, so if we try to accumulate on everything, there'd be a Vec instantiated to store the result, then we realize we don't need it, so we drop it. I suspect this would impact performance, but I didn't measure it.

Empty Vecs don't allocate so I don't think it should be a problem.

accumulate_doc_comments leaves the lexer in a "bad" state though, where it consumes 1 item "too many" in order to know it's stopped. I could change this behaviour to peek before consuming, or return the latest consumed item (which is not a comment) for further parsing 🤔

accumulate_doc_item_comments doesn't set self.input = rest when it encounters a token it can't consume and so it won't advance the tokenizer.

teoxoy · 2025-04-11T11:08:43Z

naga/src/front/wgsl/parse/mod.rs

+                token
+            }
+            while let (Token::CommentDocModule(_), span) = peek_any_next(&lexer) {
+                comments.push(lexer.source.index(span));


Doesn't Token::CommentDocModule(_) already contain the &str?

Correct! as I'm peeking I can't use it though because the borrow checker is unhappy, I think I have to make a next_module_comment function for the lexer.

Actually, it's not 100% trivial, changing this would either:

require a peek before, then getting the comment if relevant.

or use an accumulate_comments for module docs, then backtracking to get the last token which resulted in leaving that accumulate_comments.

change accumulate_comment implementation to return the last (token, rest).

A similar method to accumulate_doc_item_comments would work and we wouldn't need to backtrack.

teoxoy · 2025-04-11T11:23:52Z

naga/src/front/wgsl/parse/mod.rs

+            fn peek_any_next<'a>(lexer: &'a Lexer) -> (Token<'a>, Span) {
+                let mut cloned = lexer.clone();
+                let token = cloned.next_until(|_| true, false);
+                token
+            }
+            while let (Token::CommentDocModule(_), span) = peek_any_next(&lexer) {


We could add a next_if method on the lexer for this, making the code easier to look at.

naga/src/front/wgsl/lower/mod.rs

teoxoy · 2025-04-11T11:29:31Z

The overall approach looks good, I mainly left some comments that I think would improve the impl and a one last question about usage.

Vrixyz requested a review from a team October 3, 2024 15:56

Vrixyz marked this pull request as draft October 3, 2024 15:56

Vrixyz force-pushed the token-comment branch from edf20a1 to 999330d Compare October 4, 2024 10:11

Vrixyz mentioned this pull request Oct 4, 2024

Add support for comments bevyengine/naga_oil#108

Draft

Vrixyz force-pushed the token-comment branch from d22bf4b to 19ccb31 Compare October 8, 2024 13:49

Vrixyz mentioned this pull request Oct 9, 2024

Support comments when generating documentation jannik4/shader_docs#1

Open

Vrixyz marked this pull request as ready for review October 10, 2024 13:38

cwfitzgerald assigned jimblandy Dec 11, 2024

jimblandy requested changes Jan 7, 2025

View reviewed changes

naga/src/front/wgsl/parse/lexer.rs Outdated Show resolved Hide resolved

jimblandy requested changes Jan 8, 2025

View reviewed changes

naga/src/front/wgsl/parse/mod.rs Show resolved Hide resolved

jimblandy marked this pull request as draft January 8, 2025 03:34

Vrixyz force-pushed the token-comment branch 3 times, most recently from 0025864 to 243445c Compare January 9, 2025 10:23

Vrixyz changed the base branch from v22 to trunk January 9, 2025 10:24

Vrixyz mentioned this pull request Jan 9, 2025

naga - compile with context #5713

Closed

Vrixyz commented Jan 9, 2025

View reviewed changes

naga/src/front/wgsl/parse/mod.rs Outdated Show resolved Hide resolved

Vrixyz marked this pull request as ready for review January 10, 2025 10:20

Vrixyz requested a review from jimblandy January 10, 2025 10:21

Vrixyz force-pushed the token-comment branch from fa550b5 to cd86330 Compare January 14, 2025 13:20

jimblandy added naga Shader Translator area: naga front-end lang: WGSL WebGPU Shading Language labels Feb 6, 2025

Vrixyz and others added 20 commits February 24, 2025 20:11

few PR feedbacks ; thanks @jimblandy

60eeb63

validate doc comments + split entrypoints and functions + fix emoji

e66a6b0

comments support

do not add doc struct member if the comment is empty

b1cf5a4

module comments now behind an option<box<>> + moved comments parser t…

ad07328

…est in naga/tests/in

update tests snapshots

d3c1fd0

add tests for module docs + fix start span

eaa0901

compact: adjust handles in comments

97cdaf2

remove unused function

46c0861

update test snapshots

45689da

remove hack from error fixed by compact handle adjusting

9436232

fix wrong parameter naming

24d7c5d

adjust types for struct members

7f36ed3

update snapshots

2526753

simplify comments compacting logic (PR feedback)

8324eb6

add an option struct to opt in comments parsing for wgsl

1bacb02

add line in changelog

d1ff1a1

moved Module implementation to front/mod.rs

bc56502

update snapshots ; not sure if we should add 1 or 2 snapshot tests wi…

768d7b6

…th comments parsing enabled.

fix clippy from my changes

5de3752

rename ParserOptions to Options

46f81af

Vrixyz force-pushed the token-comment branch from d0299b6 to 386ed0d Compare February 25, 2025 09:21

use rust-style doc comments ; ignore others

9cb6d53

Vrixyz force-pushed the token-comment branch from 386ed0d to 9cb6d53 Compare February 25, 2025 09:51

Vrixyz changed the title ~~[naga] Comments parsing~~ [naga] Comments parsing (naga + wgsl) Feb 27, 2025

cwfitzgerald assigned teoxoy and unassigned jimblandy Apr 9, 2025

teoxoy requested changes Apr 11, 2025

View reviewed changes

pr feedback: simpler accumulate doc comments

6791409

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[naga] Comments parsing (naga + wgsl) #6364

[naga] Comments parsing (naga + wgsl) #6364

Vrixyz commented Oct 3, 2024 •

edited

Loading

jimblandy commented Oct 4, 2024

Vrixyz commented Oct 4, 2024

jimblandy left a comment

jimblandy Jan 7, 2025

Vrixyz Jan 9, 2025 •

edited

Loading

jimblandy Feb 20, 2025

Vrixyz Feb 21, 2025

jimblandy commented Jan 8, 2025

jimblandy commented Jan 8, 2025

Vrixyz commented Jan 9, 2025 •

edited

Loading

jimblandy commented Jan 10, 2025

Vrixyz commented Feb 25, 2025 •

edited

Loading

teoxoy Apr 11, 2025

Vrixyz Apr 17, 2025

teoxoy Apr 25, 2025

teoxoy Apr 11, 2025

Vrixyz Apr 17, 2025

teoxoy Apr 25, 2025

teoxoy Apr 25, 2025

teoxoy Apr 11, 2025

Vrixyz Apr 17, 2025

teoxoy Apr 17, 2025

Vrixyz Apr 17, 2025 •

edited

Loading

teoxoy Apr 25, 2025

teoxoy Apr 11, 2025

Vrixyz Apr 17, 2025

Vrixyz Apr 17, 2025

teoxoy Apr 25, 2025

teoxoy Apr 11, 2025

teoxoy commented Apr 11, 2025

[naga] Comments parsing (naga + wgsl) #6364

Are you sure you want to change the base?

[naga] Comments parsing (naga + wgsl) #6364

Conversation

Vrixyz commented Oct 3, 2024 • edited Loading

jimblandy commented Oct 4, 2024

Vrixyz commented Oct 4, 2024

jimblandy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vrixyz Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimblandy commented Jan 8, 2025

jimblandy commented Jan 8, 2025

Vrixyz commented Jan 9, 2025 • edited Loading

jimblandy commented Jan 10, 2025

Vrixyz commented Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vrixyz Apr 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teoxoy commented Apr 11, 2025

Vrixyz commented Oct 3, 2024 •

edited

Loading

Vrixyz Jan 9, 2025 •

edited

Loading

Vrixyz commented Jan 9, 2025 •

edited

Loading

Vrixyz commented Feb 25, 2025 •

edited

Loading

Vrixyz Apr 17, 2025 •

edited

Loading