Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 24 additions & 21 deletions slicec/src/validators/comments.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,29 +8,42 @@ pub fn validate_common_doc_comments(commentable: &dyn Commentable, diagnostics:
// Only run this validation if a doc comment is present.
let Some(comment) = commentable.comment() else { return };

only_operations_have_parameters(comment, commentable, diagnostics);
only_operations_can_return(comment, commentable, diagnostics);
validate_param_tag_placement(comment, commentable, diagnostics);
validate_returns_tag_placement(comment, commentable, diagnostics);
}

fn only_operations_have_parameters(comment: &DocComment, entity: &dyn Commentable, diagnostics: &mut Diagnostics) {
fn validate_param_tag_placement(comment: &DocComment, entity: &dyn Commentable, diagnostics: &mut Diagnostics) {
let concrete_entity = entity.concrete_entity();
if !matches!(concrete_entity, Entities::Operation(_) | Entities::Enumerator(_)) {
for param_tag in &comment.params {
report_only_operation_error(param_tag, param_tag.message.span(), entity, diagnostics);
report_only_operation_error(
"comment has a 'param' tag, but only operations and enumerators have parameters".to_owned(),
param_tag,
param_tag.message.span(),
entity,
diagnostics,
);
}
}
}

fn only_operations_can_return(comment: &DocComment, entity: &dyn Commentable, diagnostics: &mut Diagnostics) {
fn validate_returns_tag_placement(comment: &DocComment, entity: &dyn Commentable, diagnostics: &mut Diagnostics) {
if !matches!(entity.concrete_entity(), Entities::Operation(_)) {
for returns_tag in &comment.returns {
report_only_operation_error(returns_tag, returns_tag.message.span(), entity, diagnostics);
report_only_operation_error(
"comment has a 'returns' tag, but only operations have return types".to_owned(),
returns_tag,
returns_tag.message.span(),
entity,
diagnostics,
);
}
}
}

/// Helper function that reports an error if an operation-only comment-tag was used on something other than a comment.
fn report_only_operation_error(
message: String,
tag: &impl Symbol,
message_span: &Span,
entity: &dyn Commentable,
Expand All @@ -43,19 +56,9 @@ fn report_only_operation_error(
a = crate::utils::string_util::indefinite_article(entity_kind),
);

// All tag kinds are of the form "<kind> tag", so it's safe to unwrap. We only want the first word for the message.
let tag_kind = tag.kind().split_once(' ').unwrap().0;
let action_phrase = match tag_kind {
"param" => "have parameters",
"returns" => "return",
_ => unreachable!("'report_only_operation_error' was called with unsupported tag '{tag_kind}'"),
};

Diagnostic::from_lint(Lint::IncorrectDocComment {
message: format!("comment has a '{tag_kind}' tag, but only operations can {action_phrase}"),
})
.set_span(&(tag.span() + message_span))
.set_scope(entity.parser_scoped_identifier())
.add_note(note, Some(entity.span()))
.push_into(diagnostics);
Diagnostic::from_lint(Lint::IncorrectDocComment { message })
.set_span(&(tag.span() + message_span))
.set_scope(entity.parser_scoped_identifier())
.add_note(note, Some(entity.span()))
.push_into(diagnostics);
}
27 changes: 26 additions & 1 deletion slicec/src/validators/enums.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// Copyright (c) ZeroC, Inc.

use crate::diagnostics::{Diagnostic, Diagnostics, Error};
use crate::diagnostics::{Diagnostic, Diagnostics, Error, Lint};
use crate::grammar::*;

use std::collections::HashMap;
Expand All @@ -17,6 +17,10 @@ pub fn validate_enum(enum_def: &Enum, diagnostics: &mut Diagnostics) {
if enum_def.underlying.is_some() {
cannot_contain_fields(enum_def, diagnostics);
}

for enumerator in enum_def.enumerators() {
validate_param_tags(enumerator, diagnostics);
}
}

/// Validate that the enumerators are within the bounds of the specified underlying type.
Expand Down Expand Up @@ -182,3 +186,24 @@ fn compact_enums_cannot_contain_tags(enum_def: &Enum, diagnostics: &mut Diagnost
}
}
}

/// Validates that any `@param` tags on an enumerator are valid (i.e. they refer to actual fields of the enumerator).
fn validate_param_tags(enumerator: &Enumerator, diagnostics: &mut Diagnostics) {
let Some(comment) = enumerator.comment() else { return };

let fields: Vec<_> = enumerator.fields().iter().map(|f| f.identifier()).collect();
for param_tag in &comment.params {
let tag_identifier = param_tag.identifier.value.as_str();
if !fields.contains(&tag_identifier) {
Diagnostic::from_lint(Lint::IncorrectDocComment {
message: format!(
"comment has a 'param' tag for '{tag_identifier}', but enumerator '{}' has no field with that name",
enumerator.identifier(),
),
})
.set_span(param_tag.span())
.set_scope(enumerator.parser_scoped_identifier())
.push_into(diagnostics);
}
}
}
2 changes: 1 addition & 1 deletion slicec/tests/attribute_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ mod attributes {
message: "no element named 'fake' exists in scope".to_owned(),
}),
Diagnostic::from_lint(Lint::IncorrectDocComment {
message: "comment has a 'returns' tag, but only operations can return".to_owned(),
message: "comment has a 'returns' tag, but only operations have return types".to_owned(),
}),
];
for (index, lint) in updated_diagnostics.iter().enumerate() {
Expand Down
69 changes: 65 additions & 4 deletions slicec/tests/comment_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,23 @@ mod comments {
check_diagnostics(diagnostics, expected);
}

#[test]
fn enumerator_with_correct_doc_comments() {
// Arrange
let slice = "
module tests

enum E {
/// This enumerator has 2 fields.
/// @param testParam1: A string param
A(testParam1: string, testParam2: bool)
}
";

// Act/Assert
assert_parses(slice);
}

#[test]
fn operation_with_correct_doc_comments() {
// Arrange
Expand Down Expand Up @@ -395,6 +412,50 @@ mod comments {
check_diagnostics(diagnostics, [expected]);
}

#[test]
fn param_tag_is_rejected_for_enumerators_with_no_fields() {
// Arrange
let slice = "
module tests

enum E {
/// @param foo: this parameter doesn't exist.
A
}
";

// Act
let diagnostics = parse_for_diagnostics(slice);

// Assert
let expected = Diagnostic::from_lint(Lint::IncorrectDocComment {
message: "comment has a 'param' tag for 'foo', but enumerator 'A' has no field with that name".to_owned(),
});
check_diagnostics(diagnostics, [expected]);
}

#[test]
fn param_tag_is_rejected_if_its_identifier_does_not_match_a_field() {
// Arrange
let slice = "
module tests

enum E {
/// @param foo: this parameter doesn't exist.
A(bar: bool)
}
";

// Act
let diagnostics = parse_for_diagnostics(slice);

// Assert
let expected = Diagnostic::from_lint(Lint::IncorrectDocComment {
message: "comment has a 'param' tag for 'foo', but enumerator 'A' has no field with that name".to_owned(),
});
check_diagnostics(diagnostics, [expected]);
}

#[test]
fn param_tag_is_rejected_for_operations_with_no_parameters() {
// Arrange
Expand Down Expand Up @@ -513,7 +574,7 @@ mod comments {
}

#[test]
fn param_tags_can_only_be_used_with_operations() {
fn param_tags_are_rejected_on_incorrect_elements() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we update this expected message and the corresponding diagnostic in validators/comments.rs? Enumerators are now valid owners of @PARAM tags, so 'only operations can have parameters' is misleading. For example: 'comment has a param tag, but only operations and enumerators can use param tags'.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the message, and the names of the validation functions around it!

// Arrange
let slice = "
module tests
Expand All @@ -527,13 +588,13 @@ mod comments {

// Assert
let expected = Diagnostic::from_lint(Lint::IncorrectDocComment {
message: "comment has a 'param' tag, but only operations can have parameters".to_owned(),
message: "comment has a 'param' tag, but only operations and enumerators have parameters".to_owned(),
});
check_diagnostics(diagnostics, [expected]);
}

#[test]
fn returns_tags_can_only_be_used_with_operations() {
fn returns_tags_are_rejected_on_incorrect_elements() {
// Arrange
let slice = "
module tests
Expand All @@ -547,7 +608,7 @@ mod comments {

// Assert
let expected = Diagnostic::from_lint(Lint::IncorrectDocComment {
message: "comment has a 'returns' tag, but only operations can return".to_owned(),
message: "comment has a 'returns' tag, but only operations have return types".to_owned(),
});
check_diagnostics(diagnostics, [expected]);
}
Expand Down