Skip to content

Conversation

nikelborm
Copy link
Contributor

@nikelborm nikelborm commented Aug 9, 2025

Type

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

Description

When Schema.brand was extracted into a variable, other schemas, piped through that variable, resulted in Schema.Any type.

Before

const UserIdBrandSchema = Schema.brand("UserId")
const UserIdSchema = pipe(Schema.Number, UserIdBrandSchema)
//    ^? Schema.brand<Schema.Schema.Any, "UserId">

After

const UserIdBrandSchema = Schema.brand("UserId")
const UserIdSchema = pipe(Schema.Number, UserIdBrandSchema)
//    ^? Schema.brand<typeof Schema.Number, "UserId">

Copy link

changeset-bot bot commented Aug 9, 2025

🦋 Changeset detected

Latest commit: 4ab07d6

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

This PR includes changesets to release 35 packages
Name Type
effect Patch
@effect/cli Patch
@effect/cluster Patch
@effect/experimental Patch
@effect/opentelemetry Patch
@effect/platform-browser Patch
@effect/platform-bun Patch
@effect/platform-node-shared Patch
@effect/platform-node Patch
@effect/platform Patch
@effect/printer-ansi Patch
@effect/printer Patch
@effect/rpc Patch
@effect/sql-clickhouse Patch
@effect/sql-d1 Patch
@effect/sql-drizzle Patch
@effect/sql-kysely Patch
@effect/sql-libsql Patch
@effect/sql-mssql Patch
@effect/sql-mysql2 Patch
@effect/sql-pg Patch
@effect/sql-sqlite-bun Patch
@effect/sql-sqlite-do Patch
@effect/sql-sqlite-node Patch
@effect/sql-sqlite-react-native Patch
@effect/sql-sqlite-wasm Patch
@effect/sql Patch
@effect/typeclass Patch
@effect/vitest Patch
@effect/workflow Patch
@effect/ai Patch
@effect/ai-amazon-bedrock Patch
@effect/ai-anthropic Patch
@effect/ai-google Patch
@effect/ai-openai Patch

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

@nikelborm nikelborm force-pushed the fixed_brand_inference branch from 2266589 to 4ab07d6 Compare August 9, 2025 11:25
@nikelborm nikelborm changed the title fix(effect): Made Schema.brand not override piped schemas to any 🥺👉🏻👈🏻 Made Schema.brand not override piped schemas Aug 14, 2025
@nikelborm nikelborm changed the title 🥺👉🏻👈🏻 Made Schema.brand not override piped schemas 🥺👉🏻👈🏻 Made Schema.brand not any-fy piped schemas Aug 14, 2025
@IMax153 IMax153 added the schema label Aug 19, 2025
@nikelborm
Copy link
Contributor Author

nikelborm commented Aug 22, 2025

If anybody else needs a dirty patch (because I didn't change source maps, and so all usages of "go to definition" in Schema file will be a bit shifted) for node_modules folder:

patches/[email protected]

diff --git a/dist/dts/Schema.d.ts b/dist/dts/Schema.d.ts
index 15d098e7573a5f64135fdde3481fbba286514fb2..753d93b41b09ae09c73aa3e79ae2ee4574d1c741 100644
--- a/dist/dts/Schema.d.ts
+++ b/dist/dts/Schema.d.ts
@@ -1591,7 +1591,7 @@ export interface brand<S extends Schema.Any, B extends string | symbol> extends
  * @category branding
  * @since 3.10.0
  */
-export declare const brand: <S extends Schema.Any, B extends string | symbol>(brand: B, annotations?: Annotations.Schema<Schema.Type<S> & Brand<B>>) => (self: S) => brand<S, B>;
+export declare const brand: <S extends Schema.Any, B extends string | symbol>(brand: B, annotations?: Annotations.Schema<Schema.Type<S> & Brand<B>>) => <SubS extends S>(self: SubS) => brand<SubS, B>;
 /**
  * @category combinators
  * @since 3.10.0
diff --git a/src/Schema.ts b/src/Schema.ts
index ab2e298242e0d2e8a93eceb0f6b7622efcf2a8f4..6e024c127e33f1ee024b45c18463470548ae804d 100644
--- a/src/Schema.ts
+++ b/src/Schema.ts
@@ -3403,7 +3403,7 @@ export const brand = <S extends Schema.Any, B extends string | symbol>(
   brand: B,
   annotations?: Annotations.Schema<Schema.Type<S> & Brand<B>>
 ) =>
-(self: S): brand<S, B> => {
+<SubS extends S>(self: SubS): brand<SubS, B> => {
   const annotation: AST.BrandAnnotation = option_.match(AST.getBrandAnnotation(self.ast), {
     onNone: () => [brand],
     onSome: (brands) => [...brands, brand]

@nikelborm
Copy link
Contributor Author

Hello @gcanti!

May I please ask you for a few minutes of your time dedicated to the review of this tiny PR? I would greatly appreciate your attention and time, if you have the capacity and available resources for this effort.

If you would like me to add it to effect-smol too, I can do it too

@gcanti
Copy link
Contributor

gcanti commented Aug 26, 2025

@nikelborm actually I have been thinking about this for a few days, since it would be useful (for all APIs, not just for brand).

As for the implementation in this PR, it doesn't play well with annotations:

import { pipe, Schema } from "effect"

const UserIdBrandSchema = Schema.brand("UserId", {
  examples: ["a"] // UserIdBrandSchema should only apply to strings now
})

const UserIdSchema = pipe(
  Schema.Number, // no error
  UserIdBrandSchema
)

const UserIdSchema2 = pipe(
  Schema.Number, // no error
  Schema.brand("UserId", {
    examples: ["a"]
  })
)

@nikelborm
Copy link
Contributor Author

nikelborm commented Aug 26, 2025

for all APIs, not just for brand

I noticed that too. The same thing is in Schema.compose too.

const addAsdField = Schema.compose(Schema.Struct({
  asd: Schema.String
}))

// addAsdField: (from: Schema.Schema.Any) => Schema.transform<Schema.Schema.Any, Schema.Struct<{
//     asd: typeof Schema.String;
// }>>

// Has hardcoded Schema.Schema.Any here


const TestSchema = Schema.asSchema(pipe(
  Schema.Struct({sdf: Schema.Number}),
  addAsdField
))

// TestSchema: Schema.Schema<
//   { readonly asd: string; },
//   any,
//   unknown
// >

So it's generally applicable that there are 2 kinds of ways of writing generics. I noticed that some generics provide better inference in pipes, and some outside of pipes, and the user has to either choose, or there should be some clever thing that automatically determines if it is inside the pipe or outside. That's what I'm talking about: #4148 (comment)

The important change is on lines 12-14:
Screenshot From 2025-08-26 18-11-53

So in the end, could you please help me better understand how I interpret your response?

I have a few interpretations in mind:

  1. This PR won't fit well into the project just by itself, even if I resolve the problem with annotations, and you want me to find a way to fix it generally for every place where this kind of bug occurs.
  2. Same as above, but you want to fix the problem yourself, because you know the codebase better
  3. I have to fix the problem with annotations, because that's the only blocker for now, preventing the PR from being merged
  4. You want to start a discussion on how this problem could be solved generally
  5. You want me to close the PR and wait because it may/will be fixed by effect-smol

And thank you for the feedback! ❤️

@nikelborm
Copy link
Contributor Author

nikelborm commented Aug 26, 2025

Also, I figured there's even yet another thing that could be done:

Writing tests to catch that case.

import { pipe, Schema } from "effect"

const UserIdBrandSchema = Schema.brand("UserId", {
  examples: ["a"] // UserIdBrandSchema should only apply to strings now
})

const UserIdSchema = pipe(
  Schema.Number, // no error
  UserIdBrandSchema
)

const UserIdSchema2 = pipe(
  Schema.Number, // no error
  Schema.brand("UserId", {
    examples: ["a"]
  })
)

Because the CI says it's all clear, and it wasn't caught.

gcanti added a commit that referenced this pull request Aug 26, 2025
@gcanti
Copy link
Contributor

gcanti commented Aug 26, 2025

As a baseline since Schema.brand is meant to be used in a pipeline, this should raise an error (as it currently does in main):

const UserIdSchema2 = pipe(
  Schema.Number,
  Schema.brand("UserId", {
    examples: ["a"] // should raise an error
  })
)

(I added a type-level test for this)

Regarding the general issue, I have not found a way to achieve this behavior while also being able to define a generic function that applies a brand (within the same API). I have not found a solution for this in v4 either, although this is less of a problem there because brands are reified instead of being schema combinators, which makes them easier to reuse.

Maybe I just have not found the right trick yet, any attempt to solve the issue is welcome.

gcanti added a commit that referenced this pull request Aug 26, 2025
@nikelborm nikelborm force-pushed the fixed_brand_inference branch from 4ab07d6 to 725edda Compare August 26, 2025 16:38
@nikelborm nikelborm changed the title 🥺👉🏻👈🏻 Made Schema.brand not any-fy piped schemas Made Schema.brand not any-fy piped schemas Aug 26, 2025
subtleGradient pushed a commit to effect-native/effect-native that referenced this pull request Aug 29, 2025
subtleGradient added a commit to effect-native/effect-native that referenced this pull request Aug 29, 2025
* Fix effect peer dependency for bun-test

Replace invalid "workspace:^" with "^3.17.9" and set release to patch
(0.1.1) in changeset.

* --example

* Update AGENTS.md

* Support native packages and update release config

* Configure CI to publish only packages-native packages

- Fix changeset ignore pattern from './packages/**/*' to '@effect/*'
- Update release workflow to trigger on effect-native/main branch
- Add native-specific build and publish scripts
- Remove conflicting changeset with mixed packages
- Add pre-push hook to prevent upstream contamination

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>

* Test pre-push hook protection

* Revert test hook change

* Add comprehensive GitHub Copilot instructions for effect-native repository (#54)

* add type level test for PR Effect-TS#5360 (Effect-TS#5435)

* ensure Effect.promise captures span on defect (Effect-TS#5437)

* Fix `InferenceConfiguration` schema in the Amazon Bedrock AI provider package (Effect-TS#5438)

* Update GITHUB_TOKEN to use CHANGESETS_TOKEN

* Relax effect peer dependency range

* Remove bun peer dependency from bun-test

* Create new-rivers-shake.md

* Update package.json

* Restrict workspace to native packages

Remove packages/* entries and related tsconfig references. Update
scripts to target only packages-native. Switch scratchpad deps and
@effect/vitest to latest published versions. Add effect to changeset
ignore list.

* Delete pnpm-lock.yaml

* lock ++

---------

Co-authored-by: Claude <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Giulio Canti <[email protected]>
Co-authored-by: Tim <[email protected]>
Co-authored-by: Maxwell Brown <[email protected]>
subtleGradient added a commit to effect-native/effect-native that referenced this pull request Aug 29, 2025
* Fix effect peer dependency for bun-test

Replace invalid "workspace:^" with "^3.17.9" and set release to patch
(0.1.1) in changeset.

* --example

* Update AGENTS.md

* Support native packages and update release config

* Configure CI to publish only packages-native packages

- Fix changeset ignore pattern from './packages/**/*' to '@effect/*'
- Update release workflow to trigger on effect-native/main branch
- Add native-specific build and publish scripts
- Remove conflicting changeset with mixed packages
- Add pre-push hook to prevent upstream contamination

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>

* Test pre-push hook protection

* Revert test hook change

* Add comprehensive GitHub Copilot instructions for effect-native repository (#54)

* add type level test for PR Effect-TS#5360 (Effect-TS#5435)

* ensure Effect.promise captures span on defect (Effect-TS#5437)

* Fix `InferenceConfiguration` schema in the Amazon Bedrock AI provider package (Effect-TS#5438)

* Update GITHUB_TOKEN to use CHANGESETS_TOKEN

* Relax effect peer dependency range

* Remove bun peer dependency from bun-test

* Create new-rivers-shake.md

* Update package.json

* Restrict workspace to native packages

Remove packages/* entries and related tsconfig references. Update
scripts to target only packages-native. Switch scratchpad deps and
@effect/vitest to latest published versions. Add effect to changeset
ignore list.

* Delete pnpm-lock.yaml

* lock ++

* changesets configuration only ignores the scratchpad package, which
actually exists in the workspace. The @effect/* and effect packages
don't exist in this fork since we removed all the upstream packages from
the workspace, so they shouldn't be in the ignore list.

---------

Co-authored-by: Claude <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Giulio Canti <[email protected]>
Co-authored-by: Tim <[email protected]>
Co-authored-by: Maxwell Brown <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: Discussion Ongoing

Development

Successfully merging this pull request may close these issues.

3 participants