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

[ hh_client --lint | Lint[5607] ] False negative, comparing an interface to a scalar does not emit a warning #9563

Open
lexidor opened this issue Dec 26, 2024 · 3 comments

Comments

@lexidor
Copy link
Collaborator

lexidor commented Dec 26, 2024

Describe the bug
Comparing a scalar to an interface (other than Stringish and XHPChild), should emit a warning Lint[5607] Invalid comparison: This expression will always return false..

Standalone code, or other way to reproduce the problem

interface SomeInterface {}
class SomeClass {}

function caught(string $string, SomeClass $class)[]: bool {
    return $string === $class;
}

function uncaught(string $string, SomeInterface $interface)[]: bool {
    return $string === $interface;
}

Steps to reproduce the behavior:

  1. hh_client --lint filename.hack

Expected behavior

Two lint errors (one in caught, and one in uncaught)

Actual behavior

Lint[5607] Invalid comparison: This expression will always return false.
A value of type string can never be equal to a value of type SomeClass [1]

src/bug.hack:5:12
    3 | 
    4 | function caught(string $string, SomeClass $class)[]: bool {
[1] 5 |     return $string === $class;
    6 | }
    7 | 

Environment

  • Operating system 'Ubuntu'
  • Installation method 'hhvm/hhvm on dockerhub'
  • HHVM Version
HipHop VM 4.168.2 (rel) (non-lowptr)
Compiler: 1674167390_239081398
Repo schema: b20d78caba71435cd4028326fcf28ee798279c8e
hackc-af2b2722e8cac3d0bc5fe213eeb16d7296da458f-4.168.2

Additional context
I think there may be a special case for Stringish and/or XHPChild
that is tripping this linter up.
Stringish === string is a valid comparison,
since string ""implements"" Stringish.
Even though interfaces are non-exclusive, string only implements
XHPChild and Stringish, so for the purposes of this check,
implementing SomeInterface implies not string.

This issue also reproduces for all scalars (int, bool, float, string),
so int === SomeInterface should always be false.
The only common interface between a value of SomeInterface and int
is XHPChild, which can be eliminated with the same logic that
Stringish could for the string case above.

@mszabo-wikia
Copy link
Contributor

This appears to be fixed on the latest master:

$ ../hhvm/_build/hphp/hack/bin/hh_client --lint test.hack 
Server launched with the following command:
        'systemd-run' '--scope' '--user' '--quiet' '--slice=hack.slice' '/these/are/not/the/paths/you/are/looking/for/_build/hphp/hack/bin/hh_server' '-d' '/these/are/not/the/paths/you/are/looking/for/htest' '--from' '[sh]' '--waiting-client' '7'
Running in daemon mode
warning: Lint[5607] Invalid comparison: This expression will always return false.
A value of type string can never be equal to a value of type SomeClass [1]

test.hack:5:12
    3 | 
    4 | function caught(string $string, SomeClass $class)[]: bool {
[1] 5 |     return $string === $class;
    6 | }
    7 | 


warning: Lint[5607] Invalid comparison: This expression will always return false.
A value of type string can never be equal to a value of type SomeInterface [1]

test.hack:9:12
    7 | 
    8 | function uncaught(string $string, SomeInterface $interface)[]: bool {
[1] 9 |     return $string === $interface;
   10 | }

@lexidor
Copy link
Collaborator Author

lexidor commented Jan 6, 2025

@mszabo-wikia Am I ever glad I checked my notifications this evening. You have made master@HEAD buildable. I am in a state of disbelief, but immensely happy. I struggle to find to words to thank you and everyone who helped you, since "I am ever grateful" and "thank you all a thousand" don't cover my inner childlike excitement. I have seen your PR and will check this out as soon as I have an opportunity. I would very much like to get my hands on a build as soon as possible. Thank you for this amazing present. I am on cloud 9.

@mszabo-wikia
Copy link
Contributor

@mszabo-wikia Am I ever glad I checked my notifications this evening. You have made master@HEAD buildable. I am in a state of disbelief, but immensely happy. I struggle to find to words to thank you and everyone who helped you, since "I am ever grateful" and "thank you all a thousand" don't cover my inner childlike excitement. I have seen your PR and will check this out as soon as I have an opportunity. I would very much like to get my hands on a build as soon as possible. Thank you for this amazing present. I am on cloud 9.

Thanks, it has indeed been a journey.

This is all still very preliminary but at least better than nothing. The CI specifically is still rather unstable, but worst case the PR should be buildable now with an LLVM 18 toolchain provided other dependencies are met.

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

No branches or pull requests

2 participants