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

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

Open
lucacasonato opened this issue May 14, 2024 · 6 comments
Labels
Domain: Declaration Emit The issue relates to the emission of d.ts files Domain: Isolated Declarations Related to the --isolatedDeclarations compiler flag Experience Enhancement Noncontroversial enhancements

Comments

@lucacasonato
Copy link

lucacasonato commented May 14, 2024

πŸ”Ž 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

@weswigham weswigham added the Domain: Isolated Declarations Related to the --isolatedDeclarations compiler flag label May 14, 2024
@weswigham
Copy link
Member

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?

@jakebailey
Copy link
Member

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.

@dragomirtitian
Copy link
Contributor

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.

@lucacasonato
Copy link
Author

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);
};

@jakebailey
Copy link
Member

jakebailey commented May 15, 2024

Yeah, TS internally has no notion of a "set only property", see #58112 and #58119

(Void is definitely not the right answer, though)

@dragomirtitian
Copy link
Contributor

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

@weswigham weswigham added Domain: Declaration Emit The issue relates to the emission of d.ts files Experience Enhancement Noncontroversial enhancements labels May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Domain: Declaration Emit The issue relates to the emission of d.ts files Domain: Isolated Declarations Related to the --isolatedDeclarations compiler flag Experience Enhancement Noncontroversial enhancements
Projects
None yet
Development

No branches or pull requests

4 participants