Skip to content

Weak type errors on signature-only types #17660

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

Merged
merged 4 commits into from
Aug 8, 2017

Conversation

sandersn
Copy link
Member

@sandersn sandersn commented Aug 7, 2017

Now source types that only have a call signature (like functions) or construct signature will get a weak type error too. This is really good for catching uncalled functions:

functionTakingWeakType(returnWeakType);
// OOPS. Forgot to call `returnWeakType()`. That's an error!

Fixes #17656

Previously I didn't think enough about these types and assumed that we couldn't catch errors like this without breaking something else. Nothing breaks in our test suite, though, so I'm going to run it on DefinitelyTyped and see what breaks.

Now source types that only have a call signature (like functions) or construct
signature will get a weak type error too. This is really good for
catching uncalled functions:

```ts
functionTakingWeakType(returnWeakType);
// OOPS. Forgot to call `returnWeakType()`. That's an error!
```
@sandersn
Copy link
Member Author

sandersn commented Aug 7, 2017

Definitely Typed didn't turn up any new breaks. That's good, although a little surprising that it didn't find any mistakes in the tests anywhere.

@sandersn sandersn requested a review from weswigham August 8, 2017 17:24
Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice addition, but could we also add an extra bit to the error where we check if the function or ctor return type is compatible with the weak type, and if so, then suggest calling/constructing it? (And maybe give it a quick fix if it takes no arguments?)

@sandersn
Copy link
Member Author

sandersn commented Aug 8, 2017

"Did you forget to call this?"

Sure, I can do that.

@sandersn
Copy link
Member Author

sandersn commented Aug 8, 2017

I think I'll merge without a quick fix. I don't think it's high-value when it only applies to nullary functions, and quick fixes are quite finicky to get right. I can do it after the 2.5 release if we decide it's worth it.

@weswigham
Copy link
Member

Alright, sounds fine.

Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm... Its a little odd to reuse call for the constructor version. Maybe a version that says "Did you forget to construct this with new?". I'm not too hung up on it, its just that the fix implied by the suggestion would lead to another error.

@sandersn
Copy link
Member Author

sandersn commented Aug 8, 2017

I thought about that and decided I'd rather make the implementation a little simpler and the error slightly less accurate.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants