Fix Forwarding of Doc-Comments on Return-Parameters and Variant-Fields

[InsertCreativityHere]

This PR fixes #809, by ensuring that we detect and forward @returns for return parameters, and @param for fields of enumerators. Previously, these were not detected at all, and were accidentally being thrown away.

The fix for parameters was to add a is_return: bool that we check to know whether we're searching for @param or @returns.
We could split this into 2 functions instead of using a trailing bool; the function is small, and shares some code. But if anyone prefers 2 functions instead of my little bool, feel free to say so.

The fix for enumerator fields, is we re-use the logic for creating a struct field, but then 'patch' it's doc-comment with any @param that were present on the enumerator.

What's Changed

slicec:

• The fields of a Variant now correctly have their doc-comments populated by @param tags on their enclosing variant.
• The returnType fields of an Operation now correctly have their doc-comments populated by @returns tags on their enclosing operation

[Copilot]

🟡 Changes recommended

Variant conversion can erase an existing field comment when no matching @param exists.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Fixes forwarding of operation return and variant-field documentation to code generators.

Changes:

• Maps @returns tags to return fields.
• Maps enumerator @param tags to variant fields.

File summaries

File	Description
slicec/src/slice_file_converter.rs	Adds return and variant-field comment conversion.

Review details

• Files reviewed: 1/1 changed files
• Comments generated: 1
• Review effort level: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[pepone]

LGTM.

Suggestion (non-blocking)

• A regression test here would be the first for slice_file_converter.rs, but needs no new infrastructure: a #[cfg(test)] mod in the binary crate that compiles a snippet with compile_from_strings, converts it with SliceFile::from, and asserts on Field.entity_info.comment. The module below fails 3/4 on main and passes on this branch.

slicec/src/converter_doc_comment_tests.rs (wire in with #[cfg(test)] mod converter_doc_comment_tests; in main.rs)

//! Tests that `@param` / `@returns` doc-comment tags are forwarded onto the mapped `Field`s that code-generator
//! plugins receive (operation parameters, return members, and variant fields).
//
// To wire in: add `#[cfg(test)] mod converter_doc_comment_tests;` next to `mod slice_file_converter;` in main.rs.

use crate::definition_types::*;
use slicec::compile_from_strings;

/// Compiles `slice` (asserting no errors) and returns the converted contents of the file.
fn convert(slice: &str) -> Vec<Symbol> {
    let state = compile_from_strings(&[slice], None);
    assert!(!state.diagnostics.has_errors(), "{:?}", state.diagnostics);
    SliceFile::from(&state.files[0]).contents
}

/// Flattens a mapped doc-comment's overview into a single string (links rendered as `{@link id}`).
fn overview_of(field: &Field) -> Option<String> {
    field.entity_info.comment.as_ref().map(|comment| {
        comment.overview.iter().map(|component| match component {
            MessageComponent::Text(text) => text.clone(),
            MessageComponent::Link(link) => format!("{{@link {link}}}"),
        }).collect()
    })
}

fn operations(symbols: &[Symbol]) -> &[Operation] {
    let Some(Symbol::Interface(interface)) = symbols.iter().find(|s| matches!(s, Symbol::Interface(_))) else {
        panic!("no interface in {symbols:?}");
    };
    &interface.operations
}

fn variants(symbols: &[Symbol]) -> &[Variant] {
    let Some(Symbol::VariantEnum(variant_enum)) = symbols.iter().find(|s| matches!(s, Symbol::VariantEnum(_))) else {
        panic!("no variant enum in {symbols:?}");
    };
    &variant_enum.variants
}

#[test]
fn param_tags_are_forwarded_to_parameters() {
    let symbols = convert("
        module tests
        interface I {
            /// @param p: the param
            op(p: string, undocumented: int32)
        }
    ");
    let op = &operations(&symbols)[0];
    assert_eq!(overview_of(&op.parameters[0]), Some("the param\n".to_owned()));
    assert_eq!(overview_of(&op.parameters[1]), None);
}

#[test]
fn unnamed_returns_tag_is_forwarded_to_single_return_type() {
    let symbols = convert("
        module tests
        interface I {
            /// @returns: the value
            plain() -> int32

            /// @returns: the stream
            streamed() -> stream int32

            /// @returns: the tagged value
            tagged() -> tag(1) int32?
        }
    ");
    for op in operations(&symbols) {
        assert_eq!(op.return_type.len(), 1, "{}", op.entity_info.identifier);
        assert!(overview_of(&op.return_type[0]).is_some(), "{}", op.entity_info.identifier);
    }
}

#[test]
fn named_returns_tags_are_forwarded_to_return_tuple_members() {
    let symbols = convert("
        module tests
        interface I {
            /// @returns b: second
            /// @returns a: first
            op() -> (a: int32, b: string, c: bool)
        }
    ");
    let op = &operations(&symbols)[0];
    assert_eq!(overview_of(&op.return_type[0]), Some("first\n".to_owned()));
    assert_eq!(overview_of(&op.return_type[1]), Some("second\n".to_owned()));
    assert_eq!(overview_of(&op.return_type[2]), None);
}

#[test]
fn param_tags_are_forwarded_to_variant_fields() {
    let symbols = convert("
        module tests
        enum E {
            /// Variant overview.
            /// @param y: the y field
            A(x: int32, y: string)
            B(z: int32)
        }
    ");
    let variants = variants(&symbols);
    assert_eq!(overview_of(&variants[0].fields[0]), None);
    assert_eq!(overview_of(&variants[0].fields[1]), Some("the y field\n".to_owned()));
    assert_eq!(overview_of(&variants[1].fields[0]), None);
}

Author's call

• parameter.parent().return_members().len() collects a Vec just to count it, once per return member; parameter.parent().return_type.len() is the same check without the allocation, and matches how convert_operation already reads return_type directly.
• The is_return bool is fine as-is.

Follow-ups, not for this PR: an unnamed @returns: on a tuple-returning operation passes validation without a lint and, with no single member to attach it to, never reaches a generator either (#819). Same shape for an enumerator @param whose name matches no field, or that sits on a basic enum (#820).

[bernardnormier]

LGTM, with a few small items not already raised by pepone or Copilot.

Nit

• slice_file_converter.rs:274: "you would use" → "you use".

Reuse (author's call)

• The grammar Message → mapped overview conversion is now written three times in this file (get_doc_comment_for_parameter at ~L298, the From<&GrammarDocComment> impl at ~L392, and convert_variant_field at ~L569), and the @param find-and-map chain twice (~L291-300 and ~L566-570). An impl From<&GrammarMessage> for DocComment next to the existing From<&GrammarDocComment> impl, plus a small find_param_message(params, identifier) helper, would collapse all of them, and would let convert_variant_field set the comment when constructing the field instead of building it via convert_struct_field and patching it afterwards.

Follow-up, not for this PR

• No validator rejects duplicate @returns (or @param) tags for the same member, and both lookups use find, so only the first message is forwarded and the rest are silently dropped. Pre-existing for @param, newly applicable to @returns now that they are consumed.

[InsertCreativityHere]

@pepone

A regression test here would be the first for slice_file_converter.rs

I did add a test originally, but it ended up not being very small or very unit-testy, so I removed it.
Right now we don't have great infrastructure for testing the binary (as opposed to the library); we should add this
in the future. Also, much of this hand-written code is going to be replaced by Rust generated code in 0.5.0, which
will hopefully not be too long after releasing 0.4.1. It's for all these reasons I ended up not committing a test.

parameter.parent().return_members().len() collects a Vec just to count it

My Claude also flagged this minor inefficiency, but I think what the PR does is correct.
Right now, slicec has many fields that are public, but probably should not be, and return_type is one of these.
It's more correct to access this information through the return_members() function, instead of directly as a field.
One day, I will go through and make many of these correctly private.

Follow-ups, not for this PR...

The first point about unnnamed @returns is I believe, intentional design, in the case you want to document the whole return type, instead of each individual field. Might very well still be broken though.

The second point is definitely a bug. Will fix separately.

[InsertCreativityHere]

@bernardnormier

I updated the wording to "you use"

I introduced a helper function for the conversion of Message -> overview only DocComment.
But it's a standalone function, not a From. Since From kind of implies this is the de-facto conversion.
And really, converting a single overview to a full DocComment doesn't feel right for this.

And, for your duplication findings, this is a known issue, that I'd like to fix for 0.4.1; see: #625

[Copilot]

🟢 Approval recommended

The implementation matches existing validation semantics; only a non-blocking documentation typo remains.

Review details

• Files reviewed: 1/1 changed files
• Comments generated: 1
• Review effort level: Balanced

[bernardnormier]

Thanks for the fixes. One more thing before approving: several of the comments in this PR narrate the code right below them, which is exactly what we want to avoid.

Please remove

• L566 // Construct the field as normal by re-using the logic of 'convert_struct_field'. — the next line is the call to convert_struct_field.
• L300 // If this isn't a return parameter, we search the operation's doc-comment for a matching '@param' tag. — this is the else branch, spelled out.
• L444 // Iterate through the provided file's contents, and convert each of its top-level definitions. — pre-existing, but since you touched it: it describes a for loop over contents.

Please trim to the "why"

• L289-290: the two sentences mirror the two arms of the match. The only non-obvious fact is the language rule: a single return type is documented by an unnamed @returns, tuple members by name. One sentence stating that rule is enough.
• L569-570: "variant fields get '@PARAM' tags on their parent enumerator" is the why and worth keeping. The rest ("we check for a matching tag and use its message as the field's comment") restates the find/map.

The doc-comments on the new helper and on get_doc_comment_for_parameter are fine: they describe the API and the language rule, not the implementation. And deleting the old "3 steps" comment was the right call, same problem.

[bernardnormier]

One more on the helper, to finish the de-duplication.

[bernardnormier]

This is the same Message -> overview conversion that get_basic_doc_comment_from_message does. The helper can't be used here as-is because it returns a whole DocComment and this impl needs the bare overview plus real see_tags.

Suggest extracting the message conversion itself, e.g. fn convert_message(message: &slicec::grammar::Message) -> Message { message.value.iter().map(Into::into).collect() }, and calling it from both get_basic_doc_comment_from_message and here:

let overview = doc_comment.overview.as_ref().map(convert_message);

[InsertCreativityHere]

I'm not convinced this extra helper earns it's keep. It's a single-line of code, only called from 2 places.
I added it, and it actually increases our net-line-count by 3 lines.

I'd be fine with dropping your other helper (get_basic_doc_comment_from_message) in place of the convert_message suggestion. But if we added both helpers, then we'd have 2 helpers which are collectively only called by 3 places (that's together, not each). Only 1.5 calls per helper function is below the bar of usefulness IMO.
If the implementations were longer, I could see the point, but both helpers are basically one-liners.

[InsertCreativityHere]

Of the 5 doc-comments you flagged:

• I deleted the 2 comments which were obvious narration with no other value.

• I reworked the comment about "variant fields and @param". I agree about the comment including the fact we search for @param on the parent element, but I also think it's useful to state that it isn't just about copying this tag, but that we 'promote' the tag's description to a full-blown doc-comment itself. This is what the comment was trying to express.

• And for the remaining comments in get_doc_comment_for_parameter:

The only non-obvious fact is the language rule: a single return type is documented by an unnamed @returns, tuple members by name

I feel the opposite here. The fact that single return types lack a name is, IMO, an obvious fact of the Slice language.
What I find non-obvious is that when we transmit these parameters over the wire, we synthesize a doc-comment for them
based on the @param and @returns tags on their parent operation. This is non-obvious because that's not how they're
expressed by the Slice language, nor in slicec itself.