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

more type safe dual signature, closes #2967 #3068

Open
wants to merge 1 commit into
base: next-minor
Choose a base branch
from

Conversation

kristiannotari
Copy link

@kristiannotari kristiannotari commented Jun 24, 2024

Type

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

Description

Ad discussed in #2967, the dual type signature could be improved to avoid inferring an (...args: any[]) => any function type, which is assignable to every function signature you define for your dual api, and instead be precise over the implementation signature used. Of course the rest of the type signature would still be unsafe, but that's unavoidable.

This should help prevent errors when the type signature of the implementation used for the dual api is different to the one used as the final signature of such dual api.

NOTICE:

There are a couple of tests failing, regarding the schema package, which should not be related to this change. In fact, if you run the tests on the main this pr is based on, those tests keep failing.

This PR should be type level only. I added dtslint tests for the dual signature type checks I did in the first place that brought this change to exist. This should help to assert those cases are preserved in the future.

Related

Copy link

changeset-bot bot commented Jun 24, 2024

🦋 Changeset detected

Latest commit: fb2b42f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 30 packages
Name Type
@effect/typeclass Major
@effect/platform Major
effect Minor
@effect/schema Major
@effect/printer-ansi Major
@effect/printer Major
@effect/cli Major
@effect/cluster-browser Major
@effect/cluster-node Major
@effect/cluster Major
@effect/experimental Major
@effect/platform-browser Major
@effect/platform-bun Major
@effect/platform-node-shared Major
@effect/platform-node Major
@effect/rpc-http Major
@effect/rpc Major
@effect/sql-d1 Major
@effect/sql-mssql Major
@effect/sql-mysql2 Major
@effect/sql-pg Major
@effect/sql-sqlite-bun Major
@effect/sql-sqlite-node Major
@effect/sql Major
@effect/cluster-workflow Major
@effect/opentelemetry Major
@effect/sql-drizzle Major
@effect/sql-sqlite-react-native Major
@effect/sql-sqlite-wasm Major
@effect/vitest Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

// Note: `Schema.All extends S ? "you can't...` is used to prevent the case where `optional` is implicitly applied.
// For example: `S.String.pipe(S.optional)` would result in `S.String` being inferred as `Schema.All`,
// which is not the intended behavior. This is mostly an aesthetic consideration, so if it causes issues, we can remove it.
return new PropertySignatureWithFromImpl(optionalPropertySignatureAST(from, options), from)
})
})) as unknown as typeof optional
Copy link
Author

@kristiannotari kristiannotari Jun 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why this is needed, but I had to put it here to make it work. Remember the dual application before was inferred as (...args: any[]) => any so this stuff is expected on complex cases, still, should be doable without casting. Probably the user facing error case is giving some problems in using such types within the implementation code. Not sure how you all want to deal with this, so I left it here.

@mikearnaldi
Copy link
Member

Imho seems to be over complexifying usage, also it would be a breaking change that potentially breaks user code, not sure about it

@kristiannotari
Copy link
Author

kristiannotari commented Jun 25, 2024

Imho seems to be over complexifying usage, also it would be a breaking change that potentially breaks user code, not sure about it

Usage should be the same in the "common case" scenario. There are a couple of instances where it can't infer automatically the whole type you'd like your dual signature to be, and require you to type the Other and Impl parts of it as generic types. This, btw, remove the need to type the implementation internally though, so it's more like moving and typing more directly than just inferring.

It's true some user code could be breaking due to this change, at least at the type level, since this PR is all about types, even if it's just about reordering types so to put generics in place. But it should help spot places where inference would put you in a possibly bad situation runtime wise, due to (...args: any[]) => any being inferred, which if you want to, could be considered a "fixed bug", in the sense the original user code could be potentially broken already, and this pr help spotting and let user fix those cases.

@github-actions github-actions bot force-pushed the pr-dual-signature branch 2 times, most recently from a974f1a to 36f92ae Compare July 4, 2024 17:04
@tim-smart
Copy link
Member

This would be a major change, the changeset will need adjusting.

dual was also made quite "relaxed" on purpose, so I'm on fence about this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Discussion Ongoing
Development

Successfully merging this pull request may close these issues.

More precise dual signature (prevent type errors/issues)
3 participants