Refactor Attribute Diagnostics#766
Conversation
| debug_assert_eq!(directive, Self::directive()); | ||
|
|
||
| check_that_at_most_one_argument_was_provided(args, Self::directive(), span, diagnostics); | ||
| check_argument_count_is_within(0..2, args, Self::directive(), span, diagnostics); |
There was a problem hiding this comment.
Note that ranges in rust are "half-open" ranges.
In mathematical notation: 0..2 = [0,2), so this range only includes 0, and 1.
In English: Rust ranges include the starting value, but exclude the ending value.
You don't need to read this:
But Austin, why not use the alternate syntax of 0..=1 instead of 0..2?
Which makes it easier to see that this accepts 0 or 1 arguments!
These correspond to different constructs in Rust,
x..y is a shorthand for Range, and x..=y is a shorthand for RangeInclusive.
TL;DR, RangeInclusive is actually a pain to use, and has way less functionality than Range, all because it's possible to type 0..=u8::MAX. The length of this range is 256, which does not fit in a u8!
Unlike 0..u8::Max which only has a length of 255 (since the end value isn't included).
The fact that RangeInclusive can create ranges larger than their backing type means it can't safely implement alot of the same functions as Range. Which in turn makes it less useful.
| &self.directive | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Helper functions.
These are spiritual successors to the ones deleted from attribute_parsing_util, and should be reviewed.
|
|
||
| /// Reports an error if an incorrect number of attributes was provided to the specified attribute. | ||
| /// | ||
| /// # Arguments |
There was a problem hiding this comment.
This is the Rusty syntax for documenting function arguments. But in Rust, you should only document arguments if there's alot or if it's not clear what they do just from the summary and it's name.
So, it's actually rare that you need to document your arguments like this.
There was a problem hiding this comment.
Pull request overview
This PR refactors how slicec reports and validates attribute-related diagnostics by consolidating attribute error variants, moving attribute parsing helpers into slicec::grammar::attributes, and updating tests/emitters accordingly (Issue #764).
Changes:
- Replaced
utils::attribute_parsing_utilwith attribute-local helpers (report_invalid_attribute,check_argument_count_is_within) underslicec::grammar::attributes. - Consolidated/renamed attribute error variants and updated all call sites/tests to the new diagnostics.
- Updated diagnostic output expectations to match new
E###codes and attribute error messages.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| slicec/tests/diagnostic_output_tests.rs | Updates expected E### diagnostic codes in emitted JSON/text output. |
| slicec/tests/attribute_tests.rs | Updates tests to assert the new attribute error variants/fields. |
| slicec/src/validators/attribute.rs | Uses renamed field (directive) for non-repeatable attribute diagnostics. |
| slicec/src/utils/mod.rs | Removes attribute_parsing_util module export. |
| slicec/src/utils/attribute_parsing_util.rs | Deletes the old attribute parsing utility module (moved/replaced). |
| slicec/src/patchers/mod.rs | Emits UnknownAttribute for known-prefix-but-unknown directives. |
| slicec/src/grammar/attributes/mod.rs | Adds the new shared attribute helper functions and imports. |
| slicec/src/grammar/attributes/allow.rs | Switches to new arg-count validation and new error variants. |
| slicec/src/grammar/attributes/compress.rs | Switches to new arg-count validation and new error variants. |
| slicec/src/grammar/attributes/deprecated.rs | Switches to new arg-count validation and new error variants. |
| slicec/src/grammar/attributes/oneway.rs | Switches to new arg-count validation and new error variants. |
| slicec/src/grammar/attributes/sliced_format.rs | Switches to new arg-count validation and new error variants. |
| slicec/src/diagnostics/errors.rs | Introduces consolidated attribute errors + formatting and updates diagnostic code mapping. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| InvalidAttributeArgument { | ||
| /// The directive of the attribute. | ||
| directive: String, | ||
| /// the invalid argument that was provided. | ||
| argument: String, | ||
| }, | ||
|
|
||
| AttributeIsNotRepeatable { | ||
| attribute: String, | ||
| /// Too few or too many arguments were supplied to an otherwise valid attribute. | ||
| /// For example: `[oneway(FooBar)]` ('oneway' takes no arguments) or `[compress]` ('compress' requires arguments). | ||
| IncorrectAttributeArgumentCount { | ||
| /// The directive of the attribute. | ||
| directive: String, | ||
| /// A range representing the number of arguments this attribute can be given. | ||
| expected_count: Range<usize>, | ||
| /// The number of arguments this attribute was actually given. | ||
| actual_count: usize, |
There was a problem hiding this comment.
InvalidAttributeArgument, IncorrectAttributeArgumentCount
What is the point of distinguishing between the two?
There was a problem hiding this comment.
Because they just feel like different errors to me.
[compress(FooBar)] the error is that FooBar isn't a valid argument. It has to be either Args or Returns
[oneway(FooBar)] the error is that oneway shouldn't take any arguments at all.
IMO it's nice to have separate, more clearer error messages for the user instead of something more generic like:
"invalid attribute arguments supplied"
slicec has always tried to give very rich and useful errors, like rust does, and unlike our old compiler does.
There was a problem hiding this comment.
For what it's worth, with this scheme, the C# generator will never throw InvalidArgumentException, only IncorrectAttributeArgumentCount, since it never validates the inner argument.
| ), | ||
| ( | ||
| "E028", | ||
| IncorrectAttributeArgumentCount, |
There was a problem hiding this comment.
Seems quite complicated, IMO the invalid attribute argument is enough here.
There was a problem hiding this comment.
The complexity is just from me trying to emit the most accurate and grammatically correct message.
If you really want, I can prune the grammar corrections, and not bother handling singular vs plural.
Personally, I don't think is so bad tho.
This PR refactors how
slicecreports attribute errors, ahead of me adding a better API for generators to report attribute errors.Replaced it's 4 functions, with only 2 (now in the
slicec::grammar::attributesmodule)The errors I've defined in
errors.rsare identical to the ones thatslicecwill accept over the wire from Code-Generators (in my next PR), so review them with this context in mind.