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 inconsistent about banning default assignment patterns for function parameters #58549

Closed
lucacasonato opened this issue May 16, 2024 · 2 comments Β· Fixed by #58637
Labels
Bug A bug in TypeScript Domain: Isolated Declarations Related to the --isolatedDeclarations compiler flag Fix Available A PR has been opened for this issue

Comments

@lucacasonato
Copy link

lucacasonato commented May 16, 2024

πŸ”Ž Search Terms

  • isolated declarations
  • function parameter
  • default value

πŸ•— 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/KYDwDg9gTgLgBAMwK4DsDGMCWEVwJ4AUAhlFEXgFxwpIC2ARsFANoC6cAvHGwDRxoRaYEkRjQqxKjQZM+9KXUZQAlJwB81RU2VUAbhEwATOAG8AvgG4AUFdCRY-HAGd4ITqatwv+YqXIKZFnYuXkchETEoCSIApTlY7XVNQJ04fSNTMyszIA

πŸ’» Code

export function y(array: number[] = [], comparator: (a: number, b: number) => number): void {};

export const x = {
    y(array: number[] = [], comparator: (a: number, b: number) => number): void {}
}

πŸ™ Actual behavior

For function x and y no diagnostic is raised on the property array at all.

For method z.x, a diagnostic is raised on array regarding the type annotation of array not including undefined. For method z.y, no diagnostic is raised.

πŸ™‚ Expected behavior

I would expect that in all four cases to raise an ID diagnostic on array, with the only possible fix being marking comparator as optional, or adding a default value assignment.

This is for multiple reasons:

  1. The diagnostic raised for z.x suggests changing the type annotation to T | undefined. This is because without ID, TypeScript would automatically add | undefined to the emitted declarations. However, this is a bad suggestion, as doing so does not just change the external signature of the function, but also changes the type for the parameter of the function, in the function body, to a new type that does not match with the actual type of the parameter in the function (it can never be undefined in the body itself, because any undefined value would be replaced with the default argument).
  2. An isolated declarations emitter can not determine whether it is safe to emit this or not, because knowing that requires knowing whether the type of array contains undefined. See

Additional information about the issue

cc @dragomirtitian

@weswigham weswigham added Domain: Isolated Declarations Related to the --isolatedDeclarations compiler flag Bug A bug in TypeScript labels May 17, 2024
@weswigham weswigham added this to the TypeScript 5.5.1 milestone May 23, 2024
@typescript-bot typescript-bot added the Fix Available A PR has been opened for this issue label May 23, 2024
@dragomirtitian
Copy link
Contributor

dragomirtitian commented May 24, 2024

I opened #58637. This PR ensures we give errors for function declaration/class methods as well. It also only errors if the type doesn't syntactically contain undefined, and if the type can't be proven to not contain undefined. This means that number[] or T | undefined will not error, since for the first we know it does not contain undefined (so we could add it) and for the second we know it does. The only case that will error is where the type of the parameter is a type reference such as T for which we would need to lookup type information to determine if it contains undefined or not.

There is an argument to be made that this need not be an error at all. If we always add | undefined one of two things can happen: either T already contains undefined and thus the extra union is redundant but semantically equivalent, or T does not contain undefined in which case it should have been added. This does create a situation where if we align emit, TSC will seem silly. The bugs write themselves: Why is TS not smart enough to realize that T already has undefined in it and is adding this redundant union to my parameter type.

I would be happy if we were to not need an error here. @weswigham do you think adding | undefined to the type regardless would be a big source of bugs with resolution: "we did this intentionally"?

@weswigham
Copy link
Member

Obviously it's semantically equivalent to add an | undefined onto a type that already has an undefined, so it'd do nothing. But at the same time, we don't have a good reason to regress our emit and blindly add | undefined in all cases. It should certainly be fine for an emitter to do so, and should simplify things for it greatly, but we don't have a reason to do so in the general case. transpileDeclaration might need to, since it has to handle non-resolving type nodes that it can't determine the undefined-ness of, but I can't really see a compelling reason to adjust normal declaration emit output. It'd just produce stuff like Alias | undefined even when type Alias = Foo | undefined, which just.... isn't gonna feel great.

So I can see adjusting the fallback behavior for non-resolving types to be adding | undefined to the existing node - that'll probably help transpileDeclaration. I'm just less sure about changing the behavior more broadly. We've said a couple times that while we're happy to take improvements to declaration emit that happen to cause alignment (preserving original intent more by copying more nodes as originally written being the big one), we're not really interested in making our normal output match 1:1 with what something limited could produce just for the sake of it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Domain: Isolated Declarations Related to the --isolatedDeclarations compiler flag Fix Available A PR has been opened for this issue
Projects
None yet
4 participants