Skip to content

Always-false-condition in editorServices.ts #17892

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

Closed
vilchik-elena opened this issue Aug 18, 2017 · 6 comments · Fixed by #18180
Closed

Always-false-condition in editorServices.ts #17892

vilchik-elena opened this issue Aug 18, 2017 · 6 comments · Fixed by #18180
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue Help Wanted You can do this

Comments

@vilchik-elena
Copy link

The condition on line 1663 in editorServices.ts:
https://github.com/Microsoft/TypeScript/blob/eef7d8bd3d974528464c7968deab78b747568c06/src/server/editorServices.ts#L1650-L1666

is always false as the called method closeExternalProject always returns void. That way the if-block with shouldRefreshInferredProjects = true; is never executed (for more information see the linter rule detecting this).

@RyanCavanaugh RyanCavanaugh added the Bug A bug in TypeScript label Aug 18, 2017
@ghost
Copy link

ghost commented Aug 18, 2017

See also https://palantir.github.io/tslint/rules/no-void-expression/ -- but this would require us to set up linting with type information.

@mhegazy
Copy link
Contributor

mhegazy commented Aug 18, 2017

thanks for the report. a PR would be appreciated.

@mhegazy mhegazy added the Help Wanted You can do this label Aug 18, 2017
@RyanCavanaugh
Copy link
Member

@mhegazy seems like using a void value in a truthiness position is hyper-sketchy - thoughts on making that an error in TS?

@mhegazy
Copy link
Contributor

mhegazy commented Aug 21, 2017

thoughts on making that an error in TS?

i thought you had experimented with this a while back; I think it is a check worth adding, baring any breaks that we might find from RWC tests.

@RyanCavanaugh
Copy link
Member

I had a test for always-truthy values that attempted to find bugs like if (x.f) (in lieu of if (x.f()) {) but the problem was that non-strictNullChecks .d.ts file gave tons of false positives. Checking for void values in control flow test positions should be a home run

@mhegazy
Copy link
Contributor

mhegazy commented Aug 22, 2017

i think the void check should only flag errors. seems like a low risk-high yield change to make.

NaridaL added a commit to NaridaL/TypeScript that referenced this issue Aug 31, 2017
Fixes microsoft#17892
The if condition around the return value of that method in closeExternalProject indicates that this was the expected behavior.
@mhegazy mhegazy added the Fixed A PR has been merged for this issue label Sep 7, 2017
@mhegazy mhegazy added this to the TypeScript 2.6 milestone Sep 7, 2017
mhegazy pushed a commit that referenced this issue Sep 7, 2017
…ss. (#18180)

Fixes #17892
The if condition around the return value of that method in closeExternalProject indicates that this was the expected behavior.
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue Help Wanted You can do this
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants