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

Lint: sketchy-null-string: partially wrong error message? #4608

Closed
stouf opened this issue Aug 16, 2017 · 3 comments
Closed

Lint: sketchy-null-string: partially wrong error message? #4608

stouf opened this issue Aug 16, 2017 · 3 comments

Comments

@stouf
Copy link

stouf commented Aug 16, 2017

The following code

'use strict';

// @flow

function sayHi(info: { name?: string }) {
  return `Hi ${info.name ? info.name : ''}!`;
}

sayHi({});
sayHi({ name: 'foo' });

with the following configuration

[ignore]

[include]

[libs]

[lints]
all=warn

[options]
include_warnings=true

gives me the following warning with Flow v0.52.0:

» flow
Warning: index.js:6
  6:   return `Hi ${info.name ? info.name : ''}!`;
                    ^^^^^^^^^ sketchy-null-string: Sketchy null check on string value. Perhaps you meant to check for null instead of for existence?
    6:   return `Hi ${info.name ? info.name : ''}!`;
                      ^^^^^^^^^ Potentially null/undefined value.
    6:   return `Hi ${info.name ? info.name : ''}!`;
                      ^^^^^^^^^ Potentially "" value.


Found 1 warning

Am I missing something? Flow seems to be confusing maybe type and optional property here doesn't it?

@TrySound
Copy link
Contributor

@stouf The message is quite descriptive. Potentially "" value means that you should check empty string which can be valid value.

@stouf
Copy link
Author

stouf commented Aug 17, 2017

@TrySound Thanks for replying :)
The message is indeed descriptive, but seems wrong to me.

sketchy-null-string: Sketchy null check on string value. Perhaps you meant to check for null instead of for existence?

Given that name is an optional property on info and not a maybe type, I think I must check for the existence of that property first.

Potentially null/undefined value.

As written above, name is not a maybe type, so it should not null nor undefined.

Potentially "" value.

Yep, that's correct, but I don't think this should be a warning.
It could be that Flow thinks name is maybe type and therefore, since the expression info.name would be falsy if the property name doesn't exist, is an empty string, is null or is undefined, Flow is suggesting us to deal with all these cases independently.

Perhaps I'm missing something here?

@stouf
Copy link
Author

stouf commented Aug 17, 2017

According to the documentation, it seems that this is just the expected behavior since empty strings are evaluated as a falsy values.

Triggers when you do an existence check on a value that can be either null/undefined or falsey.

At first, I didn't get why Flow would show warnings for possible empty values but it's actually easy to imagine situations where it would be useful. For example,

function sayHi(info: { name?: string }) {
  if (!info.name) {
    throw new Error('Please provide a name');
  }
  if (info.name.length === 0) {
    // This case will always be caught by the if previous statement
    throw new Error('Empty name not allowed');
  }
  return `Hi ${info.name}!`;
}

The message about info.name potentially being null or undefined still seems wrong to me though..?
(I'm renaming the issue to re-center the discussion on this).

@stouf stouf changed the title Lint: sketchy-null-string Lint: sketchy-null-string: partially wrong error message? Aug 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants