Reject Streamed Parameters that are Also Tagged - #824
InsertCreativityHere merged 2 commits into
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The validation, diagnostic, and regression coverage are complete with no unresolved blocking issues.
Pull request overview
This PR updates slicec to reject streamed parameters that also specify tags.
Changes:
- Add validation for tagged streamed parameters.
- Add the
E037diagnostic. - Add regression test coverage.
File summaries
| File | Description |
|---|---|
slicec/tests/interfaces/operations.rs |
Tests rejection of tagged streamed parameters. |
slicec/src/validators/parameters.rs |
Validates streamed parameters are untagged. |
slicec/src/diagnostics/errors.rs |
Defines the new diagnostic. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| module Test | ||
|
|
||
| interface I { | ||
| op(tag(79) s: stream bool?) |
There was a problem hiding this comment.
Maybe add a second test case for stream return?
There was a problem hiding this comment.
Added a 2nd test case. We have one operation which tests an input-parameter, and a second operation which tests a return-type.
| parameter_identifier: String, | ||
| }, | ||
|
|
||
| /// A streamed parameter was also tagged. |
There was a problem hiding this comment.
| /// A streamed parameter was also tagged. | |
| /// A streamed parameter must not be tagged. |
There was a problem hiding this comment.
I think the current wording is consistent with the rest of the file. We structure our errors like:
/// What actually happened
TheRuleThatWasBroken,
Otherwise, it'd just be like restating the same thing twice.
Here, the variant is named StreamedParamsCannotBeTagged (the rule),
and the comment says"A streamed parameter was also tagged" (what concretely happens to break the rule).
(cherry picked from commit b57663f)
This PR fixes #823, which I realized while reviewing the recent docs changes to streamed parameters.
Apparently,
slicechas never rejected these:tag(5) myParam: stream int32?- which it should.Tags allow you to add/remove parameters and fields while maintaining on-the-wire compatibility, which is already possible for
streamed parameters, even withouttags. So this was never a meaningful thing to write.Additionally,
streams use their own encoding and semantics, both different fromtags.I'm targeting this for 0.4.1, even though it's technically a source-incompatible change. I don't think anyone is doing this, it's never been an intentionally supported syntax, and it's a trivial change (just remove the
tag(6)) for users.For the record, the code generators just completely ignore the tag in this case, since it splits parameters into streamed and non-streamed before checking
tags.What's Changed:
slicec
slicecnow emits an error for streamed parameters that also have tags. There is no point in applying a tag to a streamed parameter, and any such tags should be removed.