TypeScript icon indicating copy to clipboard operation
TypeScript copied to clipboard

Isolated declarations does not handle object getters and setters with different types

Open lucacasonato opened this issue 1 year ago • 5 comments

🔎 Search Terms

  • isolated declarations
  • object getter
  • object setter

🕗 Version & Regression Information

5.5.0-dev.20240514

⏯ Playground Link

https://www.typescriptlang.org/play/?target=99&isolatedDeclarations=true&ts=5.5.0-dev.20240514#code/KYDwDg9gTgLgBAYwgOwM7wIZwLxwN4BQcxcA5sPAEYAUAlAFxzpQCWyp+RJ3UFArlGRwARMK7EAvgBpxTCnBoA3DABtGzNqVqduEghIDcBAjACeYYHADKMVuwCCKlhlQ4mtzUdCRYiFOkQ3Qm5yeAATOkYbO1JHZ1dg7hJeGAEhUVlpWVR5COU1dxjtRMl9I2NvaHgkNHhLXBKyeQAzSMLNHSTiFLSRMV0Zbhz4VvzGZD4AW0pgKGLMsqA

💻 Code

export const a = {
    get b(): string {
        return ""
    },
    set b(val: string) {
    }
};

type StringAlias = string;
export const c = {
    get d(): StringAlias {
        return ""
    },
    set d(val: string) {
    }
};


export const e = {
    get f(): string {
        return ""
    },
    set f(val: number) {
    }
};

🙁 Actual behavior

TypeScript emits:

export declare const a: {
    b: string;
};
export declare const c: {
    d: string;
};
export declare const e: {
    get f(): string;
    set f(val: number);
};

This is behaviour is not possible to reproduce for isolated declarations emitters, because it requires type comparisons for the types of the setter and getter.

🙂 Expected behavior

One of:

Emit based on syntax, do not collapse a getter/setter pair with the same type to a property

export declare const a: {
    get b(): string;
    set b(val: string);
};
export declare const c: {
    get d(): StringAlias;
    set d(val: string);
};
export declare const e: {
    get f(): string;
    set f(val: number);
};

OR

Error on both c.d and e.f because return type of getter and setter do not match

Additional information about the issue

cc @dragomirtitian

lucacasonato avatar May 14 '24 11:05 lucacasonato

Hm. We didn't allow get/set in types for a looong time, and only added it to support differing getter/setter type pairs - and to avoid accidentally breaking backcompat for people, we only emitted getter/setter pairs if the types of the getter and setter actually differed.

@RyanCavanaugh do you think we've supported getter-setter pairs in .d.ts files for long enough that we can just swap our emit to always retaining the getters/setters, rather than doing the property coalescing it does now?

weswigham avatar May 14 '24 22:05 weswigham

I feel like we just talked about this in a recent design meeting or something; IMO we should be preserving things as-written and not be doing our merging thing anymore.

jakebailey avatar May 14 '24 23:05 jakebailey

I am actually surprised this does error. I remember adding an error if there is a type on both getter and setter, and only collapsing them if the type was specified only on one of them.

dragomirtitian avatar May 15 '24 08:05 dragomirtitian

Just found another problem related to the current emit:

export const x = {
    set foo(str: string) {}
}

emits as:

export declare const x: {
    foo: string;
};

even though it should probably be

export declare const x: {
    set foo(str: string);
};

or

export declare const x: {
    get foo(): void;
    set foo(str: string);
};

lucacasonato avatar May 15 '24 11:05 lucacasonato

Yeah, TS internally has no notion of a "set only property", see https://github.com/microsoft/TypeScript/issues/58112 and https://github.com/microsoft/TypeScript/issues/58119

(Void is definitely not the right answer, though)

jakebailey avatar May 15 '24 14:05 jakebailey

I don't think this is actually an issue with the errors on second thought. The way I implemented this in the sample declaration emitter was by collapsing the accessor pair only if there had one type annotation between them and preserving the get/set in the type otherwise. So the errors are fine, but the TSC emit is not yet something another tool can reproduce.

As we discussed though for 5.5 emit will not yet be fully aligned with what another tool can reproduce, so this ticket should be deferred for 5.6

dragomirtitian avatar May 22 '24 23:05 dragomirtitian