Skip to content

Conversation

@sengir
Copy link
Contributor

@sengir sengir commented May 4, 2022

This implements runtime safety checks for @intToEnum. intCast is used to both cast the operand into the enum's backing integer type as well as generate shrinkage safety checks. Then enums without explicit values are range checked while enums with explicit values have each value checked individually. The current implementation is rather simplistic and chains OR's together. In the future, it would be better to instead generate a switch with each value as a case.

I've been preoccupied with non-Zig stuff, so I'm a little out of the loop on how we want to test these. I'd appreciate some pointers. Thanks.

Related to #89.

@sengir sengir force-pushed the stage2-inttoenum-safety branch 2 times, most recently from 491c8d3 to 416b488 Compare May 4, 2022 20:39
@andrewrk andrewrk self-requested a review May 5, 2022 02:26
@sengir sengir force-pushed the stage2-inttoenum-safety branch from 24258d5 to 259376a Compare May 11, 2022 11:03
@sengir sengir force-pushed the stage2-inttoenum-safety branch from 259376a to 9994457 Compare May 23, 2022 16:15
@sengir sengir force-pushed the stage2-inttoenum-safety branch from 9994457 to e0dc608 Compare May 23, 2022 17:31
Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

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

Thanks for your patience on getting a review.

As for testing, the runtime safety tests can be executed like this:

stage1/bin/zig build test-cases -Dtest-filter=safety -Denable-llvm

When you want to mark a test as passing, do this:

--- a/test/cases/safety/integer addition overflow.zig   
+++ b/test/cases/safety/integer addition overflow.zig   
@@ -19,5 +19,5 @@ fn add(a: u16, b: u16) u16 {
 }
 
 // run
-// backend=stage1
+// backend=stage1,llvm
 // target=native

There are a few that can already be marked this way, such as this example.

}
}

pub fn enumBackingInt(ty: Type, ally: Allocator) !Type {
Copy link
Member

Choose a reason for hiding this comment

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

This is already implemented as intTagType.

.enum_nonexhaustive => {},
.enum_full, .enum_numbered, .enum_simple => {
const field_count = dest_ty.enumFieldCount();
if (dest_ty.tag() == .enum_simple or dest_ty.enumValues().count() == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Even though I made up that == 0 rule it took me a moment to remember. Suggestion to make it a bit easier on the reader:

Suggested change
if (dest_ty.tag() == .enum_simple or dest_ty.enumValues().count() == 0) {
if (dest_ty.enumIsAutoNumbered()) {

const is_in_range = try block.addBinOp(.cmp_lt, casted_operand, field_count_inst);
try sema.addSafetyCheck(block, is_in_range, .invalid_enum_value);
} else {
// TODO: Output a switch instead of chained OR's.
Copy link
Member

Choose a reason for hiding this comment

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

Hmmmm. Let's do this the right way from the start, otherwise this will harm compilation performance due to creating AIR bloat.

Suggestion: introduce a new AIR instruction called check_enum_int which returns true if and only if the integer is a valid tag of a given enum type. This way the backends can lower it as a switch directly, instead of having to parse the various AIR tags for a switch emitted from Sema. This would also allow the possibility for a backend to lower it to a function call in the case of enums with many tags.

stage1 already benefits from a similar arrangement because it implements runtime safety in the LLVM IR lowering pass, which is less flexible in some ways (panic function gets outputted even if it is never called) but comes with advantages, as illustrated here.

Alternative idea which you are welcome to explore: create a function in Sema called __zig_intToEnum_E where E is the fully qualified type name of the enum, which does the switch, calls panic if applicable with a custom panic message ("enum '{s}' has no tag with value '{d}'"), exactly matching the corresponding compile error. This alternative would not require introducing a new AIR tag, would not require any changes to the backends, and would pioneer the way forward to more detailed runtime safety messages.

@andrewrk andrewrk added enhancement Solving this issue will likely involve adding new logic or components to the codebase. frontend Tokenization, parsing, AstGen, Sema, and Liveness. labels May 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Solving this issue will likely involve adding new logic or components to the codebase. frontend Tokenization, parsing, AstGen, Sema, and Liveness.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants