Hide function type missing props if return and target type same. #36746
Conversation
|
|
openorclose
changed the title
openorclose
force-pushed
the
hide-fn-props
branch
from
8f01728 to
8dcba5f
Compare
|
Not sure why the reviewers list isn't loading, @DanielRosenwasser hope you can take a look! |
|
@DanielRosenwasser you are probably interested in this as well. |
| @@ -14074,8 +14074,8 @@ namespace ts { | |||
| source: Type, | |||
| target: Type, | |||
| relation: Map<RelationComparisonResult>, | |||
| headMessage: DiagnosticMessage | undefined, | |||
| containingMessageChain: (() => DiagnosticMessageChain | undefined) | undefined, | |||
| _headMessage: DiagnosticMessage | undefined, | |||
There was a problem hiding this comment.
If these parameters are unused, personally, I'd remove them.
| addRelatedInfo(diagnostic, createDiagnosticForNode( | ||
| node, | ||
| signatures === constructSignatures ? Diagnostics.Did_you_mean_to_use_new_with_this_expression : Diagnostics.Did_you_mean_to_call_this_expression | ||
| )); | ||
| if (errorOutputContainer && errorOutputContainer.skipLogging) { |
There was a problem hiding this comment.
I'm not really familiar with errorOutputContainer, but it sort of looks like this should have an else case that adds the diagnostic to diagnostics instead. @sandersn should be able to confirm.
There was a problem hiding this comment.
In the else clause, you'd add the diagnostic to the headMessage or containingMessageChain, which is was checkTypeAssignableTo does. The logic there is somewhat complicated so I'm not sure it's a good idea to replicate it here either. You still need to call checkTypeAssignableTo.
| @@ -1,8 +1,6 @@ | |||
| tests/cases/conformance/jsx/file.tsx(24,28): error TS2551: Property 'NAme' does not exist on type 'IUser'. Did you mean 'Name'? | |||
| tests/cases/conformance/jsx/file.tsx(36,15): error TS2322: Type '(user: IUser) => Element' is not assignable to type 'string | number | boolean | any[] | ReactElement<any>'. | |||
There was a problem hiding this comment.
Wasn't there supposed to be some new text about "did you mean to call?"? Or does that not apply in this case?
There was a problem hiding this comment.
Replacing the call to checkTypeAssignableTo with your own error in elaborateDidYouMeanToCallOrConstruct is not sufficient. That function basically just tags a "Did you mean" error on to the end of the real error.
Instead, you'll need to change the behaviour of signatures in structuredTypeRelatedTo, probably to special-case them either in propertiesRelatedTo or signaturesRelatedTo, or some third code path.
| addRelatedInfo(diagnostic, createDiagnosticForNode( | ||
| node, | ||
| signatures === constructSignatures ? Diagnostics.Did_you_mean_to_use_new_with_this_expression : Diagnostics.Did_you_mean_to_call_this_expression | ||
| )); | ||
| if (errorOutputContainer && errorOutputContainer.skipLogging) { |
There was a problem hiding this comment.
In the else clause, you'd add the diagnostic to the headMessage or containingMessageChain, which is was checkTypeAssignableTo does. The logic there is somewhat complicated so I'm not sure it's a good idea to replicate it here either. You still need to call checkTypeAssignableTo.
|
This PR hasn't seen any activity for quite a while, so I'm going to close it to keep the number of open PRs manageable. Feel free to open a fresh PR or continue the discussion here. |
Fixes #35735.
Whenever we find a source function type whose return type matches the target type, we only produce the error message "did you mean to call this expression" without checking their properties.
As mentioned in the linked issue, this should provide a more relevant error message.