Fit type check - #11
Conversation
Summary of ChangesHello @abco20, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly advances the BT-DSL compiler's type checking capabilities by introducing a sophisticated type inference system. It enables variables to have their types inferred from usage, supports wildcard types in declarations, and enforces strict integer literal bounds. The changes aim to reduce boilerplate in type declarations while maintaining strong type safety, ensuring that the compiler can intelligently deduce types and report conflicts early in the development cycle. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a significant enhancement to the type checker by implementing a full type inference system for local variables, using a union-find algorithm. It also adds checks to ensure integer literals fit within their expected types. The changes are extensive and well-structured, and the addition of a comprehensive test suite for the new inference logic is excellent. I've found one critical issue in the type unification logic that could lead to incorrect type inference or erroneous errors. My detailed feedback is in the review comments.
| const bool a_concrete_int = a->is_integer(); | ||
| const bool b_concrete_int = b->is_integer(); | ||
| const bool a_concrete_flt = a->is_float(); | ||
| const bool b_concrete_flt = b->is_float(); | ||
|
|
||
| if ((a_concrete_int && b_concrete_flt) || (a_concrete_flt && b_concrete_int)) { | ||
| return types.error_type(); | ||
| } | ||
| // Literal with float: choose float. | ||
| if (a_flt && b_int) return a->is_float() ? a : types.float64_type(); | ||
| if (b_flt && a_int) return b->is_float() ? b : types.float64_type(); |
There was a problem hiding this comment.
This block for unifying mixed integer and float types has a couple of issues:
-
The
_concrete_boolean variables are not correctly identifying concrete (non-literal) types. For example,a_concrete_intis assigneda->is_integer(), which is true for both concrete integer types andIntegerLiteral. This causes the check on line 115 to incorrectly reject valid unifications, such as between afloat32and anIntegerLiteral. -
The early returns on lines 119-120 are incorrect. For example, when unifying
{float}andint32, the code returns{float}. The result should be a concrete float type (e.g.,float64), whichcommon_numeric_typewould correctly determine. These early returns prevent the correct unification logic from being reached.
Here is a suggested fix that corrects the concrete type checks and removes the buggy early returns, allowing common_numeric_type to handle all valid mixed-family unifications.
const bool a_concrete_int = a->is_integer() && a->kind != TypeKind::IntegerLiteral;
const bool b_concrete_int = b->is_integer() && b->kind != TypeKind::IntegerLiteral;
const bool a_concrete_flt = a->is_float() && a->kind != TypeKind::FloatLiteral;
const bool b_concrete_flt = b->is_float() && b->kind != TypeKind::FloatLiteral;
if ((a_concrete_int && b_concrete_flt) || (a_concrete_flt && b_concrete_int)) {
return types.error_type();
}
// Fall through to common_numeric_type for mixed-family unification involving literals.
No description provided.