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

Incorrect Generic Constraint with Union when Nested #51969

Closed
wilwade opened this issue Dec 19, 2022 · 4 comments
Closed

Incorrect Generic Constraint with Union when Nested #51969

wilwade opened this issue Dec 19, 2022 · 4 comments
Labels
Duplicate An existing issue was already created

Comments

@wilwade
Copy link

wilwade commented Dec 19, 2022

Bug Report

🔎 Search Terms

Generic

🕗 Version & Regression Information

  • This is the behavior in every version I tried, and I reviewed the FAQ for entries about generics and unions

⏯ Playground Link

Playground link with relevant code

💻 Code

type FormProps<T extends TempModel | CreateTempModel> = {
  setModel: (model: T) => void;
  model: T;
}

type CreateFormProps = FormProps<CreateTempModel>;
type EditFormProps = FormProps<TempModel>;

const Form = <C extends CreateFormProps | EditFormProps>({
  setModel,
  model
}: C) => {
    const handleOnChange = <M extends C["model"], K extends keyof M>(value: M[K], key: K) => {
        setModel({ ...model, [key]: value });
    };
}

🙁 Actual behavior

The compiler resolves the type of setModel as (model: TempModel) => void;

🙂 Expected behavior

The compiler resolves the type of setModel as (model: TempModel | CreateTempModel) => void;

Additional Complexity

The playground code DOES work in another case.

If we add an additional constraint of isEditing this will NOT throw the error:

interface CreateFormProps extends FormProps<CreateTempModel> {
  isEditing: false;
}

interface EditFormProps extends FormProps<TempModel> {
  isEditing: true;
}

const Form = <C extends CreateFormProps | EditFormProps>({
  setModel,
  model,
  isEditing,
}: C) => {

    const handleOnChange = <M extends C["model"], K extends keyof M>(value: M[K], key: K) => {
        // FAIL
        setModel({ ...model, [key]: value });

        // WORKS, but there is no code difference between the branches!
        if (isEditing) {
          setModel({ ...model, [key]: value });
        } else {
          setModel({ ...model, [key]: value });
        }
    };
}
@andrewbranch
Copy link
Member

Ah, this is a classic. Duplicate of #35873/#30581. The first thing to note is that the nested generic is a red herring. You can remove it completely and see the same puzzling behavior:

const Form = <C extends CreateFormProps | EditFormProps>({
  setModel,
  model,
  isEditing,
}: C) => {
  setModel(model); // error
  if (isEditing) {
    setModel(model) // works
  } else {
    setModel(model) // works
  }
}

The error stems from the fact that

These should not have an error as setModel SHOULD have type (model: TempModel | CreateTempModel) => void;

is incorrect. setModel is a union of functions which is decidedly not the same as a function whose parameter is a union of the constituent functions’ parameters. When you union functions, you intersect their parameters:

setModel: ((model: TempModel) => void) | ((model: CreateTempModel) => void)
// reduces to
setModel: (model: TempModel & CreateTempModel) => void

Since the function could have either signature, you have to call it with an argument that would satisfy either signature—that is, a type that satisfies both TempModel and CreateTempModel. You don’t have that. What you have is a value that will either satisfy TempModel or CreateTempModel, but never both at the same time.

What the compiler doesn’t understand, the way you’ve written it here, is that these two unions are correlated. I think if you study #30581 and the PR that closed it, you can probably find a way to rewrite this that works.

@andrewbranch andrewbranch added the Duplicate An existing issue was already created label Dec 22, 2022
@wilwade
Copy link
Author

wilwade commented Dec 22, 2022

@andrewbranch Interesting and thank you! I got two versions working (see the second one as it is very interesting)

The model of the union of functions resolving to the intersection makes sense.

I think the best resolution here is something along the lines of this Playground Link

type UpsertMap = { "create": CreateTempModel, "update": TempModel };

type FormProps<K extends keyof UpsertMap = keyof UpsertMap> = {
  setModel: (model: UpsertMap[K]) => void;
  model: UpsertMap[K];
  formType: K;
}

I still don't quite understand how the if (isEditing) is assists the compiler in working in the original version, but I think this works for my needs.

That said. This also works! Playground Link

type FormProps<K extends CreateTempModel | TempModel = CreateTempModel | TempModel> = {
  setModel: (model: K) => void;
  model: K;
  isEditing: boolean;
}

const Form = ({
  setModel, model, isEditing,
}: FormProps) => {
  setModel(model);
}

That really doesn't make sense to me, but I guess somehow it is correctly constraining?

@typescript-bot
Copy link
Collaborator

This issue has been marked as a 'Duplicate' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

@fatcerberus
Copy link

fatcerberus commented Jan 10, 2023

@wilwade I know this issue is 3 weeks old, but I felt I should answer this:

I still don't quite understand how the if (isEditing) is assists the compiler in working in the original version

From the compiler's point of view, you have a ((model: TempModel) => void) | ((model: CreateTempModel) => void) and you're trying to call it with a TempModel | CreateTempModel. These two unions are correlated (the values involved will always be compatible, as they come from the same discriminated union), but the compiler is only looking at the types in isolation so it doesn't know that. By adding the if (isEditing) check, you narrow down the original discriminated union to a single constituent in each branch, in turn making it so that model and setModel are themselves no longer unions and the calls therefore succeed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

4 participants