bswap breaking change in 0.20

[fetsorn]

Bug description

I noticed that bswap requires a type argument for i16 integers since 0.20.00.
Without the type argument it overflows.
Versions before 0.20.00 did not require a type argument and did not overflow.

Is this expected behaviour? If this is a regression, which changes from a7c87e6 might have caused it?

Steps to reproduce

Reproduction at https://github.com/fetsorn/as-bswap-repro

  const value: i16 = -32768;

  // [ 128 0 ]
  assert(value == <i16>0x8000);

  // [ 0 128 ], asserts in both <=0.19 and >=0.20
  assert(bswap<i16>(value) == <i16>0x0080);

  // [ 0 128 ], asserts in <=0.19, fails in >=0.20
  assert(bswap(value) == <i16>0x0080);

  // [ 255 128 ], asserts in >=0.20, fails in <=0.19
  assert(bswap(value) == <i16>0xff80);

AssemblyScript version

0.20.00

[dcodeIO]

Here is bswap in 0.19.0, already requiring a type argument. Hence not sure if the observation regarding a new type argument requirement is correct: https://github.com/AssemblyScript/assemblyscript/blob/5427eb9cf7cc165e87c50b3b656b321e1ac25b6e/std/assembly/polyfills.ts#L1C1-L26

On a glimpse, there is one PR part of 0.19.17 that touched bswap: #2077 Does this one perhaps explain the change in behavior?

[fetsorn]

Thank you for a swift reply! Behaviour is the same in all versions before 0.20, including 0.19.17 and 0.19.23. The signature of bswap hasn't changed, I meant to say that type inference that used to work before 0.20 now requires an explicit type and overflows otherwise.

[dcodeIO]

Appears that the PR causing this is #2417, which is part of 0.20.19, whereas 0.20.18 does not show the difference in behavior. In the PR, bswap was moved from stdlib to a builtin, no longer honoring the argument's type if the type argument is absent. In 0.20.18, if one did bswap(someI16), what was compiled was bswap<i16>(someI16), whereas in 0.20.19, u32 is assumed if no type argument is given. So, yeah, this is indeed a bug.

[dcodeIO]

cc @MaxGraey: What would you prefer here? Move bswap back to stdlib, or try to type-infer in the builtin instead of assuming u32?

assemblyscript/src/builtins.ts

Lines 1196 to 1216 in a0c27fa

	// bswap<T?>(value: T) -> T
	function builtin_bswap(ctx: BuiltinFunctionContext): ExpressionRef {
	let compiler = ctx.compiler;
	let module = compiler.module;
	if (
	checkTypeOptional(ctx, true) |
	checkArgsRequired(ctx, 1)
	) return module.unreachable();
	let typeArguments = ctx.typeArguments;
	let arg0 = typeArguments
	? compiler.compileExpression(
	ctx.operands[0],
	typeArguments[0].toUnsigned(),
	Constraints.ConvImplicit | Constraints.MustWrap
	)
	: compiler.compileExpression(
	ctx.operands[0],
	Type.u32,
	Constraints.MustWrap
	);

[MaxGraey]

Up to you. But I guess put bswap back to std is better. Its quite a complex function and when it is in std it is easier to read and edit