Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
benadamstyles opened this issue Aug 12, 2024 · 6 comments
Open
Assignees

Comments

@benadamstyles
Copy link
Contributor

benadamstyles commented Aug 12, 2024

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
Copy link
Member

cometkim commented Aug 15, 2024

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
Copy link
Member

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
Copy link
Contributor Author

benadamstyles commented Aug 20, 2024

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
Copy link
Member

cometkim commented Aug 20, 2024

@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:

  • This can be easily ignored by skipLibCheck which disables all library type checking. Using skipLibCheck is quite common for TypeScript projects because often TypeScript libraries provide incomplete or incompatible definitions.
  • In some cases, the ReScript type and the TypeScript type do not actually structurally match. (e.g. opaque type)

@benadamstyles
Copy link
Contributor Author

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

@cometkim
Copy link
Member

cometkim commented Aug 20, 2024

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.

@cometkim cometkim self-assigned this Aug 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants