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

Soundness: this type is computed with respect to the object it is declared on, rather than the object it is called on #49991

Closed
carlpaten opened this issue Jul 21, 2022 · 4 comments
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed

Comments

@carlpaten
Copy link

carlpaten commented Jul 21, 2022

Bug Report

This prevents the type-safe construction of path-dependent builders (e.g. a builder where a certain method can only be called once, or where calling a certain method prevents calling other methods)

🔎 Search Terms

this type chained fluent builder generic

🕗 Version & Regression Information

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

⏯ Playground Link

Playground link with relevant code

💻 Code

const toppings = Symbol();
const burgerBuilder = {
  [toppings]: [] as string[],

  withMayo() {
    const {withMayo, ..._this} = this;
    return {..._this, [toppings]: [...this[toppings], 'mayo']};
  },

  withKetchup() {
    const {withKetchup, ..._this} = this;
    return {..._this, [toppings]: [...this[toppings], 'ketchup']};
  },

  order() {
    return `bun-patty-${this[toppings].map(topping => `${topping}-`).join('')}bun`
  }
};


console.log(burgerBuilder.order()); // [LOG]: "bun-patty-bun"
console.log(burgerBuilder.withMayo().order()); // [LOG]: "bun-patty-mayo-bun"
console.log(burgerBuilder.withMayo().withKetchup().order()); // [LOG]: "bun-patty-mayo-ketchup-bun"
console.log(burgerBuilder.withMayo().withKetchup().withMayo().order()); // [ERR]: burgerBuilder.withMayo(...).withKetchup(...).withMayo is not a function

🙁 Actual behavior

console.log(burgerBuilder.withMayo().withKetchup().withMayo().order()) correctly fails at runtime, but passes type-checking

🙂 Expected behavior

console.log(burgerBuilder.withMayo().withKetchup().withMayo().order()) should fail type-checking.

@DanielRosenwasser
Copy link
Member

Sounds like #7968.

@carlpaten
Copy link
Author

carlpaten commented Jul 22, 2022

@DanielRosenwasser I actually do not recognize the problem described in #7968, I can't be 100% sure since there is no repro code but I suspect the problem in #7968 is already solved.

Here the issue is that when the return type of the function depends on the type of this, it is computed too early. Here is a semi-absurd variation that I think is less likely to come up in practice but which illustrates the problem:

const item = {
    x: 1,
    getX: function() {
        return this.x;
    },
    setX: function(v: string) {
        return {...this, x: v};
    }
}

const x1 : number = item.getX(); // 1
const x2 : number = item.setX("foo").getX(); // "foo"

@RyanCavanaugh RyanCavanaugh added the Design Limitation Constraints of the existing architecture prevent this from being fixed label Jul 27, 2022
@RyanCavanaugh
Copy link
Member

You'd need to make the methods generic to detect this correctly, since the return type depends on the input type. But once it's generic, you'd need to name the core shared type to eliminate the circularity. A working version of this code looks like this:

interface Builder {
  [toppings]: string[];
  order(): string;
}

const burgerBuilder = {
  [toppings]: [] as string[],

  withMayo<T extends Builder>(this: T & { withMayo: unknown }) {
    const {withMayo, ..._this} = this;
    return {..._this, [toppings]: [...this[toppings], 'mayo']};
  },

  withKetchup<T extends Builder>(this: T & { withKetchup: unknown }) {
    const {withKetchup, ..._this} = this;
    return {..._this, [toppings]: [...this[toppings], 'ketchup']};
  },

  order() {
    return `bun-patty-${this[toppings].map(topping => `${topping}-`).join('')}bun`
  }
};


console.log(burgerBuilder.order()); // [LOG]: "bun-patty-bun"
console.log(burgerBuilder.withMayo().order()); // [LOG]: "bun-patty-mayo-bun"
console.log(burgerBuilder.withMayo().withKetchup().order()); // [LOG]: "bun-patty-mayo-ketchup-bun"
// Errors as desired
console.log(burgerBuilder.withMayo().withKetchup().withMayo().order()); // [ERR]: burgerBuilder.withMayo(...).withKetchup(...).withMayo is not a function

@carlpaten
Copy link
Author

Brilliant, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed
Projects
None yet
Development

No branches or pull requests

3 participants