Fix incorrect type inference for InclusiveRange

[darkdrag00nv2]

Issue to be solved

https://github.com/onflow/cadence/pull/2523/files#r1361281514

While testing, I also ran into the following cases that currently report errors, but shouldn't:

The inclusive range constructor always takes Int as the type parameter's type.

let r = InclusiveRange<Int>(0, 10)    // Works

let r = InclusiveRange<Int8>(0, 10)    // Checker error (should also work)

Also probably related to above, Type inferring also doesn't infer the type parameter from LHS.

let r: InclusiveRange<Int8> = InclusiveRange(0, 10)   // Type-mismatch error

Suggested Solution

No response

[darkdrag00nv2]

@SupunS

Did a bit of investigation on this. I can see that we don't support type inference of function arguments as suggested by this comment:

cadence/runtime/sema/check_invocation_expression.go

Line 577 in 323b887

// TODO: remove this once type inferring support for parameters is added

So seems like both the cases mentioned are not bugs but more of an existing limitation.

I am happy to take a shot at adding this support but would it be possible to untie this with the InclusiveRange work? I would like to wrap up the grant work sooner if possible and then take this up unless we consider this to be a blocker for the InclusiveRange feature. Thoughts?

cc: @turbolent

[turbolent]

I didn't realize we still had some gaps there. Aren't we already using this elsewhere, e.g. for the storage save function? Has this maybe improved on the feature/stable-cadence branch?

The work on the standard library identified this gap in the checker, but the work / grant for the standard library is not dependent on resolving these gaps, no worries.

We might still need to address these gaps before shipping the feature, to avoid UX issues like shown in the description, but that is separate from your grant.

I wouldn't consider this a bug, but rather just missing functionality in the type checker, as you shown 👍

[SupunS]

Makes sense. If this is already an existing limitation, and fixing it requires changing the way type-parameters are type-checked in general, then yeah, we can/should do it separately 👍

Thanks for looking into this!

[darkdrag00nv2]

I didn't realize we still had some gaps there. Aren't we already using this elsewhere, e.g. for the storage save function? Has this maybe improved on the feature/stable-cadence branch?

@turbolent No, this hasn't improved. Please check this test:

cadence/runtime/tests/checker/type_inference_test.go

Lines 338 to 409 in 7e955a3

	t.Run("with generics", func(t *testing.T) {
	t.Parallel()
	typeParameter := &sema.TypeParameter{
	Name: "T",
	TypeBound: nil,
	}
	_, err := parseAndCheckWithTestValue(t,
	`
	let res = test<[Int8]>([1, 2, 3])
	`,
	&sema.FunctionType{
	TypeParameters: []*sema.TypeParameter{
	typeParameter,
	},
	Parameters: []sema.Parameter{
	{
	Label: sema.ArgumentLabelNotRequired,
	Identifier: "value",
	TypeAnnotation: sema.NewTypeAnnotation(
	&sema.GenericType{
	TypeParameter: typeParameter,
	},
	),
	},
	},
	ReturnTypeAnnotation: sema.VoidTypeAnnotation,
	},
	)
	errs := RequireCheckerErrors(t, err, 2)
	require.IsType(t, &sema.TypeParameterTypeMismatchError{}, errs[0])
	typeParamMismatchErr := errs[0].(*sema.TypeParameterTypeMismatchError)
	assert.Equal(
	t,
	&sema.VariableSizedType{
	Type: sema.Int8Type,
	},
	typeParamMismatchErr.ExpectedType,
	)
	assert.Equal(
	t,
	&sema.VariableSizedType{
	Type: sema.IntType,
	},
	typeParamMismatchErr.ActualType,
	)
	require.IsType(t, &sema.TypeMismatchError{}, errs[1])
	typeMismatchErr := errs[1].(*sema.TypeMismatchError)
	assert.Equal(
	t,
	&sema.VariableSizedType{
	Type: sema.Int8Type,
	},
	typeMismatchErr.ExpectedType,
	)
	assert.Equal(
	t,
	&sema.VariableSizedType{
	Type: sema.IntType,
	},
	typeMismatchErr.ActualType,
	)
	})

[turbolent]

Looking into this