rescript-lang / rescript-compiler

The compiler for ReScript.
https://rescript-lang.org
Other
6.74k stars 448 forks source link

genType doesn't check typecheck imports from typescript into rescript #6947

Open benadamstyles opened 2 months ago

benadamstyles commented 2 months ago

Rescript v11.1.3

Writing the following code:

@genType.import(("@supabase/supabase-js", "SupabaseClient"))
type client<'database>

@genType.import("@supabase/supabase-js")
external createClient: (int, string) => client<'database> = "createClient"

generates the following typescript:

import {createClient as createClientNotChecked} from '@supabase/supabase-js';

// In case of type error, check the type of 'createClient' in 'Supabase.res' and '@supabase/supabase-js'.
export const createClientTypeChecked: <database>(_1:number, _2:string) => client<database> = createClientNotChecked as any;

// Export 'createClient' early to allow circular import from the '.bs.js' file.
export const createClient: unknown = createClientTypeChecked as <database>(_1:number, _2:string) => client<database> as any;

import type {SupabaseClient as $$client} from '@supabase/supabase-js';

export type client<database> = $$client<database>;

This should fail tsc typechecking, because the API of createClient is (string, string), not (int, string). It doesn't however, because of createClientNotChecked as any. Simply removing that as any makes tsc complain and therefore fixes this issue.

At first I thought this was a deliberate change from previous versions of genType, and asked about that on the forum, but now I think that it's a bug due to the wording of the comment in the generated typescript:

// In case of type error, check the type of 'createClient' in 'Supabase.res' and '@supabase/supabase-js'.

I would argue that the generated code is not doing what that comment says it is doing.

Maybe I have made a mistake somewhere 😅 thanks for your help!

cometkim commented 2 months ago

Good catch. This is unintended. I think it is more like a design-space issue, not an implementation bug

The addition of as any assumes that it is already fully verified by the ReScript compiler, which is not the right approach for external types.

cometkim commented 2 months ago

Runtime binding will be completely removed from #6196

The problem is that it is no longer checked even from tsc. I could make it generate an additional hidden type to force tsc to check it, but that would be easily opt-out by skipLibCheck option.

benadamstyles commented 2 months ago

Hey, thanks for the reply, sorry I missed it as I was AFK.

The problem is that it is no longer checked even from tsc. I could make it generate an additional hidden type to force tsc to check it, but that would be easily opt-out by skipLibCheck option.

Could you elaborate or reword this? I'm struggling to fully understand! What is no longer checked from tsc? And what kind of hidden type are you thinking of?

cometkim commented 2 months ago

@benadamstyles Of course.

The next version of genType output will be "library types". It will only have .d.ts files. So, library projects will not need any additional TypeScript configuration at all.

The ReScript compiler fully verifies the output, but you can still force unsafe definitions using @genType.import.

E.g. input:

@genType.import(("external-module", "Type"))
type t = string

let print = (t: t) => Console.log(t)

output:

export type t = import("external-module").Type;

export const print: (t: t) => void;

Then, if the actual external type is number, this is unsafe. To make this report an error, we need to adjust the output as follows

type $GenTypeImport<Expected, T extends Expected> = T;

export type t = $GenTypeImport<string, import("external-module").Type>;
//                                                               ^ Type 'number' does not satisfy the constraint 'string'.

export const print: (t: t) => void;

This may solve the issue, but still is not a complete solution.

A few minor problems:

benadamstyles commented 2 months ago

@cometkim Ok thanks, I get it. And what about other kinds of @gentype.import, such as functions? Will that still be supported?

cometkim commented 2 months ago

Ok thanks, I get it. And what about other kinds of @gentype.import, such as functions? Will that still be supported?

There is no particular restriction on specifying types on the ReScript side. It is a kind of assertion for FFI, and additional validation should be done by tsc.