Skip to content
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

LGTM.com - false positive #2620

Closed
ljharb opened this issue Jan 10, 2020 · 4 comments
Closed

LGTM.com - false positive #2620

ljharb opened this issue Jan 10, 2020 · 4 comments
Assignees

Comments

@ljharb
Copy link

@ljharb ljharb commented Jan 10, 2020

Description of the false positive

It complains that https://github.com/es-shims/es5-shim/blob/master/es5-shim.js#L2024 has no effect. However, in JavaScript, concatenating with a string has always had a potential effect - it invokes the operand's toString() method, which might have side effects and might throw. In this case, it's explicitly being used inside a try/catch to trigger an exception when the operand is a Symbol.

https://lgtm.com/projects/g/es-shims/es5-shim/snapshot/19e189f143ae56896a49a02c4edb02505310a1e3/files/es5-shim.js?sort=name&dir=ASC&mode=heatmap#x229e38c0aa84b3d4:1

@asgerf

This comment has been minimized.

Copy link
Contributor

@asgerf asgerf commented Jan 13, 2020

Hi @ljharb,

Thanks for your report. We deliberately flag code whose only potential effect looks unintentional, and the cases where a coercion happens for its side effects alone are so rare that it's best to clarify the intent with a suppression comment. I can see you've already done so for a few other tools, but unfortunately LGTM does not understand ESLint or jscs suppression comments.

So if you're willing to add yet another a suppression comment to your collection, it would look like this:

'' + str; // jscs:ignore disallowImplicitTypeConversion lgtm[js/useless-expression]

Alternatively, the whole query can be disabled by checking in an lgtm.yml file with these contents:

queries:
  - exclude: js/useless-expression
@asgerf asgerf closed this Jan 13, 2020
@ljharb

This comment has been minimized.

Copy link
Author

@ljharb ljharb commented Jan 13, 2020

I'm not willing to add either, unfortunately; it might be wise for LGTM to add support for eslint suppression comments, since that's the defacto standard in the ecosystem.

In this specific case, however, since it's inside a try/catch, i don't think your heuristic should flag it.

@asgerf

This comment has been minimized.

Copy link
Contributor

@asgerf asgerf commented Jan 13, 2020

The other issue you opened (#2621) has a try/catch surrounding the statement, and I'm considering how best to fix the FP there. There isn't a try/catch that will obviously catch the exception here.

@ljharb

This comment has been minimized.

Copy link
Author

@ljharb ljharb commented Jan 13, 2020

Ah, that's true, I'm intentionally trying to trigger the exception so users see it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.