Skip to content

Commit

Permalink
Improve diagnostics for initializers
Browse files Browse the repository at this point in the history 
Summary:
* Improve remaining diagnostic messages mentioning value, type and the name of the innermost named entity as we do in other cases.
* Remove "custom default" from the diagnostic messages since it doesn't make sense for consts.
* Simplify initializer checks for primitive types.
* Fix `double` initializer test which incorrectly used `float` instead of `double`.
* Remove duplicate tests.
* Add a few more tests.

The following example illustrates diagnostic improvement.

Thrift:

```
const i32 big = 2147483648;
```

Before:

```
[ERROR:test.thrift:1] value error: const `big` has an invalid custom default value.
```

After:

```
[ERROR:test.thrift:1] 2147483648 is out of range for `i32` in initialization of `big`
```

Reviewed By: avalonalex

Differential Revision: D61730467

fbshipit-source-id: 4b34e8f73ec28184f10477ef113ea34a4b212269
  • Loading branch information
@vitaut @facebook-github-bot
vitaut authored and facebook-github-bot committed Aug 24, 2024
1 parent 500c52d commit 88ec5c7
Show file tree
Hide file tree
Showing 3 changed files with 106 additions and 227 deletions.
152 changes: 61 additions & 91 deletions thrift/compiler/sema/check_initializer.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,35 +31,6 @@
namespace apache::thrift::compiler {
namespace {

bool is_valid_int_initializer(
const t_primitive_type* type, const t_const_value* value) {
int64_t min = 0, max = 0;
if (type->is_byte()) {
min = std::numeric_limits<int8_t>::min();
max = std::numeric_limits<int8_t>::max();
} else if (type->is_i16()) {
min = std::numeric_limits<int16_t>::min();
max = std::numeric_limits<int16_t>::max();
} else if (type->is_i32()) {
min = std::numeric_limits<int32_t>::min();
max = std::numeric_limits<int32_t>::max();
} else {
assert(false); // Should be unreachable.
}
return min <= value->get_integer() && value->get_integer() <= max;
}

bool is_valid_float_initializer(const t_const_value* value) {
return std::numeric_limits<float>::lowest() <= value->get_double() &&
value->get_double() <= std::numeric_limits<float>::max();
}

template <typename T>
bool is_valid_float_initializer_with_integer_value(const t_const_value* value) {
return value->get_integer() ==
static_cast<int64_t>(static_cast<T>(value->get_integer()));
}

// Returns the category of an initializer. It is not a real type because the
// types of initializers are inferred later.
const char* get_category(const t_const_value* val) {
Expand Down Expand Up @@ -92,7 +63,6 @@ class checker {

if (auto primitive_type = dynamic_cast<const t_primitive_type*>(type)) {
check_primitive_type(primitive_type, value);
check_base_value(primitive_type, value);
} else if (auto enum_type = dynamic_cast<const t_enum*>(type)) {
check_enum(enum_type, value);
} else if (auto structured_type = dynamic_cast<const t_structured*>(type)) {
Expand Down Expand Up @@ -123,18 +93,6 @@ class checker {
diags_.warning(node_, msg, std::forward<T>(args)...);
}

void report_value_precision() {
error(
"value error: const `{}` cannot be represented precisely as `float` "
"or `double`.",
name_);
}

void report_value_mistmatch() {
error(
"value error: const `{}` has an invalid custom default value.", name_);
}

// Report an error when the initializer is incompatible with the type.
void report_incompatible(
const t_const_value* initializer, const t_type* type) {
Expand All @@ -152,78 +110,90 @@ class checker {
name_);
}

// For CV_INTEGER, an overflow of int64_t is checked in the parser;
// therefore, we don't need to check an overflow of i64 or a floating point
// stored in integer value. Similarly, for CV_DOUBLE, we do not need
// to check double. However, we need to check a floating point stored
// with CV_INTEGER that might lead to a precision loss when converting int64_t
// to a floating point.
void check_base_value(
const t_primitive_type* type, const t_const_value* value) {
switch (type->primitive_type()) {
case t_primitive_type::type::t_void:
case t_primitive_type::type::t_string:
case t_primitive_type::type::t_binary:
case t_primitive_type::type::t_bool:
case t_primitive_type::type::t_i64:
break;
case t_primitive_type::type::t_byte:
case t_primitive_type::type::t_i16:
case t_primitive_type::type::t_i32:
if (value->kind() == t_const_value::CV_INTEGER &&
!is_valid_int_initializer(type, value)) {
report_value_mistmatch();
}
break;
case t_primitive_type::type::t_float:
if (value->kind() == t_const_value::CV_DOUBLE &&
!is_valid_float_initializer(value)) {
report_value_mistmatch();
}
if (value->kind() == t_const_value::CV_INTEGER &&
!is_valid_float_initializer_with_integer_value<float>(value)) {
report_value_precision();
}
break;
case t_primitive_type::type::t_double:
if (value->kind() == t_const_value::CV_INTEGER &&
!is_valid_float_initializer_with_integer_value<double>(value)) {
report_value_precision();
}
break;
template <typename T>
void check_int(const t_const_value* value, const t_type* type) {
if (value->kind() != t_const_value::CV_INTEGER) {
report_incompatible(value, type);
return;
}
// Range check is not needed for int64_t but it makes the code simpler and
// will be optimized away.
int64_t int_value = value->get_integer();
if (int_value < std::numeric_limits<T>::min() ||
int_value > std::numeric_limits<T>::max()) {
error(
"{} is out of range for `{}` in initialization of `{}`",
int_value,
type->name(),
name_);
}
}

template <typename T>
void check_float(const t_const_value* value, const t_type* type) {
if (value->kind() == t_const_value::CV_DOUBLE) {
// Range check is not needed for double but it makes the code simpler and
// will be optimized away.
double double_value = value->get_double();
if (double_value < std::numeric_limits<T>::lowest() ||
double_value > std::numeric_limits<T>::max()) {
error(
"{} is out of range for `{}` in initialization of `{}`",
double_value,
type->name(),
name_);
}
} else if (value->kind() == t_const_value::CV_INTEGER) {
int64_t int_value = value->get_integer();
if (int_value != static_cast<int64_t>(static_cast<T>(int_value))) {
error(
"cannot convert {} to `{}` in initialization of `{}`",
int_value,
type->name(),
name_);
}
} else {
report_incompatible(value, type);
}
}

void check_primitive_type(
const t_primitive_type* type, const t_const_value* value) {
bool compatible = false;
const auto kind = value->kind();
switch (type->primitive_type()) {
case t_primitive_type::type::t_void:
return;
case t_primitive_type::type::t_string:
case t_primitive_type::type::t_binary:
compatible = kind == t_const_value::CV_STRING;
if (kind != t_const_value::CV_STRING) {
report_incompatible(value, type);
}
break;
case t_primitive_type::type::t_bool:
compatible =
kind == t_const_value::CV_BOOL || kind == t_const_value::CV_INTEGER;
if (kind != t_const_value::CV_BOOL &&
kind != t_const_value::CV_INTEGER) {
report_incompatible(value, type);
}
break;
case t_primitive_type::type::t_byte:
check_int<int8_t>(value, type);
break;
case t_primitive_type::type::t_i16:
check_int<int16_t>(value, type);
break;
case t_primitive_type::type::t_i32:
check_int<int32_t>(value, type);
break;
case t_primitive_type::type::t_i64:
compatible = kind == t_const_value::CV_INTEGER;
check_int<int64_t>(value, type);
break;
case t_primitive_type::type::t_double:
check_float<double>(value, type);
break;
case t_primitive_type::type::t_float:
compatible = kind == t_const_value::CV_INTEGER ||
kind == t_const_value::CV_DOUBLE;
check_float<float>(value, type);
break;
}
if (!compatible) {
report_incompatible(value, type);
}
}

void check_enum(const t_enum* type, const t_const_value* value) {
Expand Down
86 changes: 44 additions & 42 deletions thrift/compiler/test/compiler_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -344,30 +344,30 @@ TEST(CompilerTest, integer_overflow_underflow) {
check_compile(R"(
# Unsigned Ints
const i64 overflowUint = 18446744073709551615; # max uint64
# expected-error@-1: integer constant 18446744073709551615 is too large
# expected-error@-1: integer constant 18446744073709551615 is too large
)");
check_compile(R"(
const i64 overflowUint2 = 18446744073709551616; # max uint64 + 1
# expected-error@-1: integer constant 18446744073709551616 is too large
# expected-error@-1: integer constant 18446744073709551616 is too large
)");
}

TEST(CompilerTest, double_overflow_underflow) {
check_compile(R"(
const double overflowConst = 1.7976931348623159e+308;
# expected-error@-1: floating-point constant 1.7976931348623159e+308 is out of range
# expected-error@-1: floating-point constant 1.7976931348623159e+308 is out of range
)");
check_compile(R"(
const double overflowConst = 1.7976931348623159e+309;
# expected-error@-1: floating-point constant 1.7976931348623159e+309 is out of range
# expected-error@-1: floating-point constant 1.7976931348623159e+309 is out of range
)");
check_compile(R"(
const double overflowConst = 4.9406564584124654e-325;
# expected-error@-1: magnitude of floating-point constant 4.9406564584124654e-325 is too small
# expected-error@-1: magnitude of floating-point constant 4.9406564584124654e-325 is too small
)");
check_compile(R"(
const double overflowConst = 1e-324;
# expected-error@-1: magnitude of floating-point constant 1e-324 is too small
# expected-error@-1: magnitude of floating-point constant 1e-324 is too small
)");
}

Expand All @@ -380,86 +380,84 @@ TEST(CompilerTest, void_data) {
)");
}

TEST(CompilerTest, const_wrong_type) {
check_compile(R"(
const i32 wrongInt = "stringVal";
# expected-error@-1: cannot convert string to `i32` in initialization of `wrongInt`
struct A {}
struct B {}
const A wrongStruct = B{}; # expected-error: type mismatch: expected test.A, got test.B
)");
}

TEST(CompilerTest, const_byte_value) {
TEST(CompilerTest, byte_initializer) {
check_compile(R"(
const byte c1 = 127;
const byte c2 = 128;
# expected-error@-1: value error: const `c2` has an invalid custom default value.
# expected-error@-1: 128 is out of range for `byte` in initialization of `c2`
const byte c3 = -128;
const byte c4 = -129;
# expected-error@-1: value error: const `c4` has an invalid custom default value.
# expected-error@-1: -129 is out of range for `byte` in initialization of `c4`
)");
}

TEST(CompilerTest, const_i16_value) {
TEST(CompilerTest, i16_initializer) {
check_compile(R"(
const i16 c1 = 32767;
const i16 c2 = 32768;
# expected-error@-1: value error: const `c2` has an invalid custom default value.
# expected-error@-1: 32768 is out of range for `i16` in initialization of `c2`
const i16 c3 = -32768;
const i16 c4 = -32769;
# expected-error@-1: value error: const `c4` has an invalid custom default value.
# expected-error@-1: -32769 is out of range for `i16` in initialization of `c4`
)");
}

TEST(CompilerTest, const_i32_value) {
TEST(CompilerTest, i32_initializer) {
check_compile(R"(
const i32 c1 = 2147483647;
const i32 c2 = 2147483648;
# expected-error@-1: value error: const `c2` has an invalid custom default value.
const i32 c0 = 42;
const i32 c1 = 4.2;
# expected-error@-1: cannot convert floating-point number to `i32` in initialization of `c1`
const i32 c2 = "string typing";
# expected-error@-1: cannot convert string to `i32` in initialization of `c2`
const i32 c3 = -2147483648;
const i32 c4 = -2147483649;
# expected-error@-1: value error: const `c4` has an invalid custom default value.
const i32 c3 = 2147483647;
const i32 c4 = 2147483648;
# expected-error@-1: 2147483648 is out of range for `i32` in initialization of `c4`
const i32 c5 = -2147483648;
const i32 c6 = -2147483649;
# expected-error@-1: -2147483649 is out of range for `i32` in initialization of `c6`
)");
}

TEST(CompilerTest, const_float_value) {
TEST(CompilerTest, float_initializer) {
check_compile(R"(
const float c0 = 1e8;
const float c1 = 3.4028234663852886e+38;
const float c2 = 3.402823466385289e+38; // max float + 1 double ulp
# expected-error@-1: value error: const `c2` has an invalid custom default value.
# expected-error@-1: 3.402823466385289e+38 is out of range for `float` in initialization of `c2`
const float c3 = -3.4028234663852886e+38;
const float c4 = -3.402823466385289e+38; // min float - 1 double ulp
# expected-error@-1: value error: const `c4` has an invalid custom default value.
# expected-error@-1: -3.402823466385289e+38 is out of range for `float` in initialization of `c4`
const float c5 = 100000001;
# expected-error@-1: value error: const `c5` cannot be represented precisely as `float` or `double`.
# expected-error@-1: cannot convert 100000001 to `float` in initialization of `c5`
const float c6 = -100000001;
# expected-error@-1: value error: const `c6` cannot be represented precisely as `float` or `double`.
# expected-error@-1: cannot convert -100000001 to `float` in initialization of `c6`
)");
}

TEST(CompilerTest, const_double_value) {
TEST(CompilerTest, double_initializer) {
check_compile(R"(
const double c0 = 1e8;
const double c1 = 1.7976931348623157e+308;
const double c2 = -1.7976931348623157e+308;
const float c3 = 10000000000000001;
# expected-error@-1: value error: const `c3` cannot be represented precisely as `float` or `double`.
const double c3 = 10000000000000001;
# expected-error@-1: cannot convert 10000000000000001 to `double` in initialization of `c3`
const float c4 = -10000000000000001;
# expected-error@-1: value error: const `c4` cannot be represented precisely as `float` or `double`.
const double c4 = -10000000000000001;
# expected-error@-1: cannot convert -10000000000000001 to `double` in initialization of `c4`
)");
}

TEST(CompilerTest, const_binary) {
TEST(CompilerTest, binary_initializer) {
check_compile(R"(
const binary b0 = "foo";
Expand Down Expand Up @@ -487,6 +485,11 @@ TEST(CompilerTest, struct_initializer) {
const S s6 = [];
# expected-error@-1: cannot convert list to `S` in initialization of `s6`
struct A {}
struct B {}
const A a = B{};
# expected-error@-1: type mismatch: expected test.A, got test.B
)");
}

Expand Down Expand Up @@ -851,8 +854,7 @@ TEST(CompilerTest, mixins_and_refs) {
1: optional D a (cpp.mixin); # expected-warning: The annotation cpp.mixin is deprecated. Please use @thrift.Mixin instead.
# expected-error@-1: Mixin field `a` cannot be optional.
}
)");
)");
}

TEST(CompilerTest, bitpack_with_tablebased_seriliazation) {
Expand Down
Loading

0 comments on commit 88ec5c7

Please sign in to comment.