feat(60442): show deprecation warnings on implementations of a deprecated property #60454
Conversation
|
Any update on merging this change? |
|
@typescript-bot pack this |
|
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
| const symbol = getSymbolOfDeclaration(declaration); | ||
| if ( | ||
| isDeprecatedSymbol(symbol) && symbol.declarations && | ||
| isTypeAssignableTo(type, getTypeOfSymbol(symbol)) |
There was a problem hiding this comment.
Why does this need to check assignability? Isn't the name sufficient?
I think that this code leads to this incorrectly flagging a deprecation:
type I = {
/**
* @deprecated
*/
text: string;
} | {
text: string | number;
};
function f(i: I) { return i; }
f({ text: 'a' });
const a: I = { text: 'a' }
a.text;- nightly: Playground Link
- this PR: Playground Link
Nightly shows what I think is "correct", which is that it's not deprecated.
There was a problem hiding this comment.
@jakebailey thanks for the additional case. I'll check it.
Why does this need to check assignability?
Since the prop has the same name but different types, I thought comparing the prop types was necessary here — am I wrong?
There was a problem hiding this comment.
I suppose there is, but I would think the logic would proceed similarly to other places that do deprecation; I believe there's another place that issues deprecations on contextual types which looked different (but I'm replying on my phone so can't find it easily 😅)
There was a problem hiding this comment.
The function I'm thinking of is createJsxAttributesTypeFromAttributesProperty, but it doesn't check assignability at all!
There was a problem hiding this comment.
You are right, it doesn't, and doesn't support this case 😅. Anyway, I’ll explore a better solution...
There was a problem hiding this comment.
Isn't that correct behavior? (Or at least, the behavior I said was better?)
There was a problem hiding this comment.
Why does this need to check assignability? Isn't the name sufficient?
interface A {
a: number;
}
interface B {
/** @deprecated */
a: string;
}
const a: A | B = { a: 'a' } // has to be deprecated
const b: A | B = { a: 1 }At this case, the contextual type is a union type (A | B), and the initializer type is string. The getPropertyOfType returns a single symbol with combined declarations for each PropSignature. To properly handle deprecated declarations, need to identify the type used in the initializer and compare it with one of the union types. If we could resolve the appropriate type at this stage (e.g., B for 'a' and A for 'b'), it would eliminate the need for assignability/relation checks. If you have any thoughts or suggestions, I’d be grateful to hear them. thanks
Fixes #60442