-
Notifications
You must be signed in to change notification settings - Fork 12.8k
feat(47698): Detect uncalled function statements #47719
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
Conversation
Curious |
Heya @RyanCavanaugh, I've started to run the extended test suite on this PR at 5fb24d1. You can monitor the build here. |
Heya @RyanCavanaugh, I've started to run the parallelized community code test suite on this PR at 5fb24d1. You can monitor the build here. |
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
@RyanCavanaugh Looks like a bug in |
The TypeScript team hasn't accepted the linked issue #47698. If you can get it accepted, this PR will have a better chance of being reviewed. |
@typescript-bot user test this inline |
Heya @jakebailey, I've started to run the diff-based community code test suite on this PR at 5fb24d1. You can monitor the build here. Update: The results are in! |
@jakebailey Here they are:Comparison Report - main..refs/pull/47719/merge fp-ts3 of 4 projects failed to build with the old tsc /mnt/ts_downloads/fp-ts/dtslint/ts3.5/tsconfig.json
|
This seems great, though I have no clue where the above code is in fp-ts to check that user test diff; AFAIK the user tests pull whatever's latest in their git repo and line 12 is a comment. |
@jakebailey I think the error is related to this uncalled function |
Oh, duh, I was in the wrong folder. I guess that would turn into a message now, even though they are using it just for checking expected types. I wonder if this will cause oddities for the DT tests? |
@typescript-bot run dt |
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at 5fb24d1. You can monitor the build here. |
DT run finished, and it does seem like this causes loads of issues with the DT tests, like:
|
Also, if merged, I think this PR also closes #47699, right? |
All errors seem to be related to uncalled functions and located in tests, not in |
If we're going to accept this, I'm assuming we'll need some sort of change in dtslint to ignore it (since ExpectType is clearly not going away any time soon). Is that doable, given this check isn't a compiler option (so we may read the build as "failing"?). I'm also sort of wary of the change where someone's using ExpectType externally, since now their build will end up failing too. |
What about adding a new |
Discussed at the design meeting and we'd prefer to leave this Awaiting More Feedback for now -- the sorts of errors this can detect can be equally picked up by a non-type-aware linter, this interacts quite badly with dts-lint, and more annoyingly it would bring up a lot of false errors as you're typing. |
Fixes #47698