Enable type conversions by AprupKale · Pull Request #63 · source-academy/java-slang

AprupKale · 2025-01-21T06:02:55Z

Fixes #56

github-actions · 2025-01-21T06:04:51Z

Coverage report

St.^❔	Category	Percentage	Covered / Total
🟡	Statements	74% (+0.05% 🔼)	7259/9809
🟡	Branches	60.43% (+0.04% 🔼)	2410/3988
🟡	Functions	69.55% (+0.1% 🔼)	1293/1859
🟡	Lines	74.97% (+0.04% 🔼)	6844/9129

Show new covered files 🐣

St.^❔	File	Statements	Branches	Functions	Lines
🟢	`...` / assignmentExpression.test.ts	100%	100%	100%	100%

Show files with reduced coverage 🔻

St.^❔	File	Statements	Branches	Functions	Lines
🟢	`...` / code-generator.ts	91.51% (-2.9% 🔻)	77.45% (-7.2% 🔻)	93.62% (+0.44% 🔼)	91.75% (-2.99% 🔻)

Test suite run success

1136 tests passing in 63 suites.

Report generated by 🧪jest coverage report action from 2a2cc0b

1001mei · 2025-02-08T02:51:15Z

+    if (areClassTypesCompatible(fromType, toType) || fromType === '') {
+      return 0;
+    }


When will fromType be empty?

1001mei · 2025-02-08T03:05:38Z

+    const methodInfo = symbolInfos[symbolInfos.length - 1] as MethodInfos
+    if (!methodInfo || methodInfo.length === 0) {
+      throw new Error(`No method information found for ${n.identifier}`)
+    }
+


perhaps can rename methodInfo to methodInfos, then can remove the methodInfos in line 796 to remove duplication

1001mei · 2025-02-08T03:43:33Z

+    const fullDescriptor = methodInfo[0].typeDescriptor // Full descriptor, e.g., "(Ljava/lang/String;C)V"
+    const paramDescriptor = fullDescriptor.slice(1, fullDescriptor.indexOf(')')) // Extract "Ljava/lang/String;C"
+    const params = paramDescriptor.match(/(\[+[BCDFIJSZ])|(\[+L[^;]+;)|[BCDFIJSZ]|L[^;]+;/g)
+
+    // Parse individual parameter types
+    if (params && params.length !== n.argumentList.length) {
+      throw new Error(
+        `Parameter mismatch: expected ${params?.length || 0}, got ${n.argumentList.length}`
+      )
+    }
+
    n.argumentList.forEach((x, i) => {
      const argCompileResult = compile(x, cg)
-      maxStack = Math.max(maxStack, i + 1 + argCompileResult.stackSize)
+
+      const expectedType = params?.[i] // Expected parameter type
+      const stackSizeChange = handleImplicitTypeConversion(argCompileResult.resultType, expectedType ?? '', cg)
+      maxStack = Math.max(maxStack, i + 1 + argCompileResult.stackSize + stackSizeChange)


i assume your scope are not supporting method overloading? otherwise might have issues of converting type unnecessary because here it's always converted to match the first method in methodInfo array.

1001mei · 2025-02-08T04:10:47Z

+          cg.code.push(typeConversionsImplicit[conversionKeyRight])
+          finalType = 'D';
+        } else if (leftType !== 'F' && rightType === 'F') {
+          // handleImplicitTypeConversion(leftType, 'F', cg);


can remove this commented out line~

martin-henz · 2026-05-28T03:45:35Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces support for implicit type conversions, string concatenation, and unary operations for various types (long, float, double) in the compiler's code generator, along with corresponding test cases. The reviewer identified several critical and high-severity issues in the code generator: (1) incorrect use of the JavaScript in operator on arrays instead of includes(), (2) potential undefined bytecode generation when implicit conversions for smaller types are not found in typeConversionsImplicit, (3) underestimation of peak stack sizes when stackSizeChange is negative, and (4) a bug in the parameter mismatch check where a method with no parameters could bypass the check if arguments are provided.

gemini-code-assist · 2026-05-28T03:47:37Z

+        if (leftType !== 'D' && rightType === 'D') {
+          cg.code.fill(typeConversionsImplicit[conversionKeyLeft],
+            insertConversionIndex, insertConversionIndex + 1)
+          finalType = 'D';
+        } else if (leftType === 'D' && rightType !== 'D') {
+          cg.code.push(typeConversionsImplicit[conversionKeyRight])
+          finalType = 'D';
+        } else if (leftType !== 'F' && rightType === 'F') {
+          // handleImplicitTypeConversion(leftType, 'F', cg);
+          cg.code.fill(typeConversionsImplicit[conversionKeyLeft],
+            insertConversionIndex, insertConversionIndex + 1)
+          finalType = 'F';
+        } else if (leftType === 'F' && rightType !== 'F') {
+          cg.code.push(typeConversionsImplicit[conversionKeyRight])
+          finalType = 'F';
+        }


Similar to the integer promotion block, if a smaller type like byte ('B') is implicitly converted to double ('D') or float ('F'), the conversion key (e.g., 'B->D') will not be found in typeConversionsImplicit, resulting in undefined being pushed or filled into the bytecode. You should check if the conversion opcode is defined before pushing or filling it.

if (leftType !== 'D' && rightType === 'D') { const conversionOp = typeConversionsImplicit[conversionKeyLeft]; if (conversionOp !== undefined) { cg.code.fill(conversionOp, insertConversionIndex, insertConversionIndex + 1); } finalType = 'D'; } else if (leftType === 'D' && rightType !== 'D') { const conversionOp = typeConversionsImplicit[conversionKeyRight]; if (conversionOp !== undefined) { cg.code.push(conversionOp); } finalType = 'D'; } else if (leftType !== 'F' && rightType === 'F') { const conversionOp = typeConversionsImplicit[conversionKeyLeft]; if (conversionOp !== undefined) { cg.code.fill(conversionOp, insertConversionIndex, insertConversionIndex + 1); } finalType = 'F'; } else if (leftType === 'F' && rightType !== 'F') { const conversionOp = typeConversionsImplicit[conversionKeyRight]; if (conversionOp !== undefined) { cg.code.push(conversionOp); } finalType = 'F'; }

gemini-code-assist · 2026-05-28T03:47:37Z

+        if (leftType !== 'I') {
+          cg.code.fill(typeConversionsImplicit[conversionKeyLeft],
+            insertConversionIndex, insertConversionIndex + 1)
+        }
+        if (rightType !== 'I') {
+          cg.code.push(typeConversionsImplicit[conversionKeyRight])
+        }
+        finalType = 'I';


If leftType or rightType is a smaller type like byte ('B'), short ('S'), or char ('C'), they are loaded as int ('I') on the JVM stack. However, their resultType is still returned as 'B', 'S', or 'C'. This causes conversionKeyLeft or conversionKeyRight to be something like 'B->I', which is not defined in typeConversionsImplicit. As a result, typeConversionsImplicit[conversionKeyLeft] returns undefined, which is then filled into cg.code, leading to invalid bytecode generation. You should check if the conversion opcode is defined before pushing or filling it.

if (leftType !== 'I') { const conversionOp = typeConversionsImplicit[conversionKeyLeft]; if (conversionOp !== undefined) { cg.code.fill(conversionOp, insertConversionIndex, insertConversionIndex + 1); } } if (rightType !== 'I') { const conversionOp = typeConversionsImplicit[conversionKeyRight]; if (conversionOp !== undefined) { cg.code.push(conversionOp); } }

gemini-code-assist · 2026-05-28T03:47:37Z

+    if (!(fromType in ['J', 'D']) && toType in ['J', 'D']) {
+      return 1;
+    } else if (!(toType in ['J', 'D']) && fromType in ['J', 'D']) {
+      return -1;


The in operator in JavaScript/TypeScript checks for the presence of property names/indices, not values, when used on an array. Therefore, fromType in ['J', 'D'] will check if 'J' or 'D' is an index of the array (which evaluates to false), rather than checking if the array contains the value. Use Array.prototype.includes() instead.

Suggested change

if (!(fromType in ['J', 'D']) && toType in ['J', 'D']) {

return 1;

} else if (!(toType in ['J', 'D']) && fromType in ['J', 'D']) {

return -1;

if (!['J', 'D'].includes(fromType) && ['J', 'D'].includes(toType)) {

return 1;

} else if (!['J', 'D'].includes(toType) && ['J', 'D'].includes(fromType)) {

return -1;

gemini-code-assist · 2026-05-28T03:47:37Z

-        maxStack = Math.max(maxStack, compile(vi, cg).stackSize)
+        const { stackSize: initializerStackSize, resultType: initializerType } = compile(vi, cg)
+        const stackSizeChange = handleImplicitTypeConversion(initializerType, variableInfo.typeDescriptor, cg)
+        maxStack = Math.max(maxStack, initializerStackSize + stackSizeChange)


When calculating maxStack, if stackSizeChange is negative (for example, during a widening conversion from long to float where stackSizeChange is -1), adding stackSizeChange directly to the expression's stack size will underestimate the peak stack size. The peak stack size is reached before the conversion instruction is executed (when the long value is still on the stack). To prevent stack underflow/overflow errors at runtime, you should only add positive stack size changes to the peak stack calculation.

Suggested change

maxStack = Math.max(maxStack, initializerStackSize + stackSizeChange)

maxStack = Math.max(maxStack, initializerStackSize + Math.max(0, stackSizeChange))

gemini-code-assist · 2026-05-28T03:47:37Z

-      maxStack = 1 + compile(right, cg).stackSize
+      const { stackSize: rhsSize, resultType: rhsType } = compile(right, cg)
+      const stackSizeChange = handleImplicitTypeConversion(rhsType, info.typeDescriptor, cg)
+      maxStack = Math.max(maxStack, 1 + rhsSize + stackSizeChange)


Similar to other stack size calculations, if stackSizeChange is negative (e.g., converting long to float), adding it directly will underestimate the peak stack size. Only positive stack size changes should be added.

Suggested change

maxStack = Math.max(maxStack, 1 + rhsSize + stackSizeChange)

maxStack = Math.max(maxStack, 1 + rhsSize + Math.max(0, stackSizeChange))

gemini-code-assist · 2026-05-28T03:47:37Z

+      const { stackSize: rhsSize, resultType: rhsType } = compile(right, cg)
+      const stackSizeChange = handleImplicitTypeConversion(rhsType, fieldInfo.typeDescriptor, cg)
+
+      maxStack = Math.max(maxStack, maxStack + rhsSize + stackSizeChange)


Only positive stack size changes should be added to the peak stack size calculation to avoid underestimating the stack size when stackSizeChange is negative.

Suggested change

maxStack = Math.max(maxStack, maxStack + rhsSize + stackSizeChange)

maxStack = Math.max(maxStack, maxStack + rhsSize + Math.max(0, stackSizeChange))

gemini-code-assist · 2026-05-28T03:47:37Z

+    if (params && params.length !== n.argumentList.length) {
+      throw new Error(
+        `Parameter mismatch: expected ${params?.length || 0}, got ${n.argumentList.length}`
+      )
+    }


If a method has no parameters, paramDescriptor is empty, and params (the result of the regex match) will be null. If the method is called with arguments (i.e., n.argumentList.length > 0), the condition params && params.length !== n.argumentList.length will evaluate to null (falsy), and the compiler will not throw a parameter mismatch error. You should calculate the expected parameter count safely and check against it.

// Parse individual parameter types const expectedParamCount = params ? params.length : 0; if (expectedParamCount !== n.argumentList.length) { throw new Error( 'Parameter mismatch: expected ' + expectedParamCount + ', got ' + n.argumentList.length ) }

AprupKale added 8 commits January 14, 2025 14:53

Add opcodes for type conversions to code-generator.ts

70eedf6

Add implicit type conversions in MethodInvocation to code-generator.ts

64e245d

Add implicit type conversions in MethodInvocation to code-generator.ts

a33c9af

Handle stack size during primitive type conversions in code-generator.ts

1848b4d

Handle type conversions in binary expressions in code-generator.ts

f88069e

Fix bugs with float conversions

db87720

Allow primitive types to be safely converted to String

6e0a2d7

Allow primitive types other than boolean in ternary conditional

7e58159

AprupKale added the enhancement New feature or request label Jan 21, 2025

AprupKale self-assigned this Jan 21, 2025

AprupKale added 3 commits January 21, 2025 14:35

Fix bugs in type checker

6bbafe7

Enable unary expressions for non-integer types

57eeb8b

Fix bug in type conversion for binary expressions

2a2cc0b

AprupKale requested a review from 1001mei January 28, 2025 07:51

1001mei reviewed Feb 8, 2025

View reviewed changes

gemini-code-assist Bot reviewed May 28, 2026

View reviewed changes

	maxStack = Math.max(maxStack, initializerStackSize + stackSizeChange)
	maxStack = Math.max(maxStack, initializerStackSize + Math.max(0, stackSizeChange))

	maxStack = Math.max(maxStack, 1 + rhsSize + stackSizeChange)
	maxStack = Math.max(maxStack, 1 + rhsSize + Math.max(0, stackSizeChange))

	maxStack = Math.max(maxStack, maxStack + rhsSize + stackSizeChange)
	maxStack = Math.max(maxStack, maxStack + rhsSize + Math.max(0, stackSizeChange))

Conversation

AprupKale commented Jan 21, 2025

Uh oh!

github-actions Bot commented Jan 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage report

Test suite run success

Uh oh!

1001mei Feb 8, 2025

Choose a reason for hiding this comment

Uh oh!

1001mei Feb 8, 2025

Choose a reason for hiding this comment

Uh oh!

1001mei Feb 8, 2025

Choose a reason for hiding this comment

Uh oh!

1001mei Feb 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martin-henz commented May 28, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot May 28, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 28, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 28, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 28, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 28, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 28, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 28, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions Bot commented Jan 21, 2025 •

edited

Loading

1001mei Feb 8, 2025 •

edited

Loading