Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Support for Compact Enums #686

Merged
Merged
Show file tree
Hide file tree
Changes from 2 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
28 changes: 23 additions & 5 deletions src/diagnostics/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,14 @@ pub enum Error {
enum_identifier: String,
},

/// A type was marked 'compact' when it was invalid to do so.
CannotBeCompact {
/// The kind of type that was marked compact.
kind: &'static str,
/// The identifier of the type.
identifier: String,
},

/// An enumerator was found that was out of bounds of the underlying type of the parent enum.
EnumeratorValueOutOfBounds {
/// The identifier of the enumerator.
Expand Down Expand Up @@ -137,9 +145,6 @@ pub enum Error {
/// Compact structs cannot be empty.
CompactStructCannotBeEmpty,

/// Compact structs cannot contain tagged fields.
CompactStructCannotContainTaggedFields,

// ---------------- Tag Errors ---------------- //
/// A duplicate tag value was found.
CannotHaveDuplicateTag {
Expand Down Expand Up @@ -168,6 +173,12 @@ pub enum Error {
identifier: String,
},

/// Compact types cannot contain tagged fields.
CompactTypeCannotContainTaggedFields {
/// The kind of type that contains the fields.
kind: &'static str,
},

// ---------------- General Errors ---------------- //
/// A compact ID was not in the expected range, 0 .. i32::MAX.
CompactIdOutOfBounds,
Expand Down Expand Up @@ -365,8 +376,9 @@ implement_diagnostic_functions!(
),
(
"E018",
CompactStructCannotContainTaggedFields,
"tagged fields are not supported in compact structs\nconsider removing the tag, or making the struct non-compact"
CompactTypeCannotContainTaggedFields,
format!("tagged fields are not supported in compact {kind}s\nconsider removing the tag, or making the {kind} non-compact"),
kind
Copy link
Member

Choose a reason for hiding this comment

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

I agree with @externl on the new lines in the error descriptions. Do we do this anywhere else?

Copy link
Member Author

Choose a reason for hiding this comment

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

There's a few scattered around notes and error messages.
Just whenever something seemed more readable on two lines.

),
(
"E019",
Expand Down Expand Up @@ -552,6 +564,12 @@ implement_diagnostic_functions!(
EnumeratorCannotContainFields,
format!("invalid enumerator '{enumerator_identifier}': fields cannot be declared within enums that specify an underlying type"),
enumerator_identifier
),
(
"E055",
CannotBeCompact,
format!("'{kind}' '{identifier}' cannot be marked compact"),
kind, identifier
)
);

Expand Down
1 change: 1 addition & 0 deletions src/grammar/elements/enum.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ pub struct Enum {
pub identifier: Identifier,
pub enumerators: Vec<WeakPtr<Enumerator>>,
pub underlying: Option<TypeRef<Primitive>>,
pub is_compact: bool,
pub is_unchecked: bool,
pub scope: Scope,
pub attributes: Vec<WeakPtr<Attribute>>,
Expand Down
6 changes: 3 additions & 3 deletions src/parsers/slice/grammar.lalrpop
Original file line number Diff line number Diff line change
Expand Up @@ -194,9 +194,9 @@ ExceptionSpecification: Vec<TypeRef> = {
}

Enum: OwnedPtr<Enum> = {
<p: Prelude> <l1: @L> <uk: unchecked_keyword?> <l2: @L> enum_keyword <i: ContainerIdentifier> <r: @R> <tr: (":" <TypeRef>)?> "{" <es: UndelimitedList<Enumerator>> "}" ContainerEnd => {
let l = if uk.is_some() { l1 } else { l2 };
construct_enum(parser, p, uk.is_some(), i, tr, es, Span::new(l, r, parser.file_name))
<p: Prelude> <l1: @L> <ck: compact_keyword?> <uk: unchecked_keyword?> <l2: @L> enum_keyword <i: ContainerIdentifier> <r: @R> <tr: (":" <TypeRef>)?> "{" <es: UndelimitedList<Enumerator>> "}" ContainerEnd => {
let l = if ck.is_some() || uk.is_some() { l1 } else { l2 };
construct_enum(parser, p, ck.is_some(), uk.is_some(), i, tr, es, Span::new(l, r, parser.file_name))
},
}

Expand Down
3 changes: 3 additions & 0 deletions src/parsers/slice/grammar.rs
Original file line number Diff line number Diff line change
Expand Up @@ -359,9 +359,11 @@ fn check_return_tuple(parser: &mut Parser, return_tuple: &Vec<OwnedPtr<Parameter
}
}

#[allow(clippy::too_many_arguments)]
fn construct_enum(
parser: &mut Parser,
(raw_comment, attributes): (RawDocComment, Vec<WeakPtr<Attribute>>),
is_compact: bool,
is_unchecked: bool,
identifier: Identifier,
underlying_type: Option<TypeRef>,
Expand All @@ -375,6 +377,7 @@ fn construct_enum(
identifier,
enumerators: Vec::new(),
underlying,
is_compact,
is_unchecked,
scope: parser.current_scope.clone(),
attributes,
Expand Down
8 changes: 8 additions & 0 deletions src/patchers/encoding_patcher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -415,6 +415,14 @@ impl ComputeSupportedEncodings for Enum {
}
}

if self.is_compact {
// Compact enums are not allowed in Slice1 mode.
supported_encodings.disable(Encoding::Slice1);
if compilation_mode == CompilationMode::Slice1 {
return Some("enums defined in Slice1 mode cannot be 'compact'");
}
}

for enumerator in self.enumerators() {
// Enums with fields are not allowed in Slice1 mode.
if enumerator.fields.is_some() {
Expand Down
52 changes: 52 additions & 0 deletions src/validators/enums.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ pub fn validate_enum(enum_def: &Enum, diagnostics: &mut Diagnostics) {
enumerator_values_are_unique(enum_def, diagnostics);
underlying_type_cannot_be_optional(enum_def, diagnostics);
nonempty_if_checked(enum_def, diagnostics);
check_compact_modifier(enum_def, diagnostics);
compact_enums_cannot_contain_tags(enum_def, diagnostics);

// If the enum wasn't defined in a Slice1 file, validate whether fields or explicit values are allowed,
// based on whether it has an underlying type. Fields in Slice1 files are already rejected by `encoding_patcher`.
Expand Down Expand Up @@ -173,3 +175,53 @@ fn cannot_contain_explicit_values(enum_def: &Enum, diagnostics: &mut Diagnostics
}
}
}

/// Validate that compact enums do not have an underlying type and are not marked as 'unchecked'.
fn check_compact_modifier(enum_def: &Enum, diagnostics: &mut Diagnostics) {
if enum_def.is_compact {
if let Some(underlying) = &enum_def.underlying {
Diagnostic::new(Error::CannotBeCompact {
kind: enum_def.kind(),
identifier: enum_def.identifier().to_owned(),
})
.set_span(enum_def.span())
.add_note(
"compact enums cannot also have underlying types\ntry removing either the 'compact' modifier, or the underlying type",
Copy link
Member

Choose a reason for hiding this comment

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

I think we should try to avoid using newlines in these notes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, I'll change them all to other punctuation.

What's the problem with newlines though?
The console output will already have newlines from snippets, and the json output is all string-escaped.

Some(underlying.span()),
)
.push_into(diagnostics);
}

if enum_def.is_unchecked {
Diagnostic::new(Error::CannotBeCompact {
kind: enum_def.kind(),
identifier: enum_def.identifier().to_owned(),
})
.set_span(enum_def.span())
.add_note(
"An enum cannot be both unchecked and compact - try removing the 'compact' modifier",
None,
)
.push_into(diagnostics);
}
}
}

/// Validate that tags cannot be used in compact enums.
fn compact_enums_cannot_contain_tags(enum_def: &Enum, diagnostics: &mut Diagnostics) {
if enum_def.is_compact {
for enumerator in enum_def.enumerators() {
for field in enumerator.fields() {
if field.is_tagged() {
Diagnostic::new(Error::CompactTypeCannotContainTaggedFields { kind: enum_def.kind() })
.set_span(field.span())
.add_note(
format!("enum '{}' is declared compact here", enum_def.identifier()),
Some(enum_def.span()),
)
.push_into(diagnostics);
}
}
}
}
}
16 changes: 9 additions & 7 deletions src/validators/structs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,15 @@ fn compact_structs_cannot_contain_tags(struct_def: &Struct, diagnostics: &mut Di
if struct_def.is_compact {
for field in struct_def.fields() {
if field.is_tagged() {
Diagnostic::new(Error::CompactStructCannotContainTaggedFields)
.set_span(field.span())
.add_note(
format!("struct '{}' is declared compact here", struct_def.identifier()),
Some(struct_def.span()),
)
.push_into(diagnostics);
Diagnostic::new(Error::CompactTypeCannotContainTaggedFields {
kind: struct_def.kind(),
})
.set_span(field.span())
.add_note(
format!("struct '{}' is declared compact here", struct_def.identifier()),
Some(struct_def.span()),
)
.push_into(diagnostics);
}
}
}
Expand Down
1 change: 1 addition & 0 deletions tests/dictionaries/key_type.rs
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ fn allowed_constructed_types(key_type: &str, key_type_def: &str) {
}

#[test_case("MyEnum", "enum MyEnum { A }", "enum", "Slice2" ; "enums")]
#[test_case("MyEnum", "compact enum MyEnum { A }", "enum", "Slice2" ; "compact enums")]
#[test_case("MyEnum", "unchecked enum MyEnum {}", "enum", "Slice2" ; "unchecked enums")]
#[test_case("MyClass", "class MyClass {}", "class", "Slice1"; "classes")]
fn disallowed_constructed_types(key_type: &str, key_type_def: &str, key_kind: &str, mode: &str) {
Expand Down
38 changes: 38 additions & 0 deletions tests/enums/container.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,44 @@ mod associated_fields {
use super::*;
use test_case::test_case;

#[test]
fn enumerator_fields_can_be_tagged() {
// Arrange
let slice = "
module Test
enum E {
A(tag(1) b: bool?),
}
";

// Act
let ast = parse_for_ast(slice);

// Assert
let field = ast.find_element::<Field>("Test::E::A::b").unwrap();
assert!(field.is_tagged());
}

#[test]
fn tags_are_disallowed_in_compact_enums() {
// Arrange
let slice = "
module Test
compact enum E {
A(tag(1) b: bool?),
}
";

// Act
let diagnostics = parse_for_diagnostics(slice);

// Assert
let expected = Diagnostic::new(Error::CompactTypeCannotContainTaggedFields { kind: "enum" })
.add_note("enum 'E' is declared compact here", None);

check_diagnostics(diagnostics, [expected]);
}

#[test]
fn explicit_values_are_not_allowed() {
// Arrange
Expand Down
77 changes: 77 additions & 0 deletions tests/enums/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ mod mode_compatibility;

use crate::test_helpers::*;
use slicec::diagnostics::{Diagnostic, Error};
use slicec::slice_file::Span;
use test_case::test_case;

#[test_case("int8"; "int8")]
Expand Down Expand Up @@ -78,3 +79,79 @@ fn optional_underlying_types_fail() {
});
check_diagnostics(diagnostics, [expected]);
}

#[test]
fn enums_can_be_unchecked() {
// Arrange
let slice = "
module Test

unchecked enum E { A }
";

// Act/Assert
assert_parses(slice);
}

#[test]
fn enums_can_be_compact() {
// Arrange
let slice = "
module Test

compact enum E { A }
";

// Act/Assert
assert_parses(slice);
}

#[test]
fn compact_enums_cannot_have_underlying_types() {
// Arrange
let slice = "
module Test

compact enum E: uint8 { A }
";

// Act
let diagnostics = parse_for_diagnostics(slice);

// Assert
let expected = Diagnostic::new(Error::CannotBeCompact {
kind: "enum",
identifier: "E".to_owned(),
})
.set_span(&Span::new((4, 9).into(), (4, 23).into(), "string-0"))
.add_note(
"compact enums cannot also have underlying types\ntry removing either the 'compact' modifier, or the underlying type",
Some(&Span::new((4, 25).into(), (4, 30).into(), "string-0")),
);
check_diagnostics(diagnostics, [expected]);
}

#[test]
fn compact_enums_cannot_be_unchecked() {
// Arrange
let slice = "
module Test

compact unchecked enum E { A }
";

// Act
let diagnostics = parse_for_diagnostics(slice);

// Assert
let expected = Diagnostic::new(Error::CannotBeCompact {
kind: "enum",
identifier: "E".to_owned(),
})
.set_span(&Span::new((4, 9).into(), (4, 33).into(), "string-0"))
.add_note(
"An enum cannot be both unchecked and compact - try removing the 'compact' modifier",
None,
);
check_diagnostics(diagnostics, [expected]);
}
29 changes: 29 additions & 0 deletions tests/enums/mode_compatibility.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,4 +111,33 @@ mod slice1 {
);
check_diagnostics(diagnostics, [expected]);
}

/// Verifies that compact enums are disallowed in Slice1 mode.
#[test]
fn compact_enums_fail() {
// Arrange
let slice = "
mode = Slice1
module Test

compact enum E {
A
}
";

// Act
let diagnostics = parse_for_diagnostics(slice);

// Assert
let expected = Diagnostic::new(Error::NotSupportedInCompilationMode {
kind: "enum".to_owned(),
identifier: "E".to_owned(),
mode: CompilationMode::Slice1,
})
.add_note(
"enums defined in Slice1 mode cannot be 'compact'",
None,
);
check_diagnostics(diagnostics, [expected]);
}
}
2 changes: 1 addition & 1 deletion tests/structs/tags.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ mod compact_structs {
let diagnostics = parse_for_diagnostics(slice);

// Assert
let expected = Diagnostic::new(Error::CompactStructCannotContainTaggedFields)
let expected = Diagnostic::new(Error::CompactTypeCannotContainTaggedFields { kind: "struct" })
.add_note("struct 'S' is declared compact here", None);

check_diagnostics(diagnostics, [expected]);
Expand Down
Loading