flow [nfc]: Mark specific rules that are being suppressed. #4433

chrisbobbe · 2021-01-25T19:17:30Z

When we start using Flow v0.137+, along with React Native v0.64
(that's issue #4426), we'll be asked to specify what rule(s) we're
suppressing with each suppression comment. Conveniently, the
warnings will tell us the applicable rule(s) we need to write down.

So, having peeked ahead at the warnings seen in the RN 0.64.0-rc.2
world, get a head start on doing so. There will probably be more of
these (we haven't yet started enforcing the requirement now, so some
unmarked $FlowFixMes will naturally accumulate), but we might as
well get this chunk out of the way.

gnprice

Thanks! Will be glad to start using this feature. Comments below.

gnprice · 2021-01-26T01:54:08Z

src/compose/ComposeMenu.js

+    // Upstream API is unclear.
+    // $FlowFixMe[sketchy-null-bool]
    const error: string | null = response.error || null;


Let's try to keep the explanatory text after the $FlowFixMe, so between the $FlowFixMe and the code it's about. That helps keep it clear what text is there to explain the fixme, and what's there to explain something else about the code.

In particular that will help us delete the explanatory text when we delete the fixme, because of either a Flow upgrade or an improvement in the types. Otherwise it'd be easy to pretty often leave it behind because the person deleting the fixme didn't fully understand the comment at that moment, and then there'd be a puzzling comment lingering there to confuse people.

When it's too long for one line, we can use a /* … */ comment, like in a few of our existing comments and in the docs at https://flow.org/en/docs/errors/ .

gnprice · 2021-01-26T01:55:27Z

src/boot/store.js

  // Store data through our own wrapper for AsyncStorage, in particular
  // to get compression.
-  // $FlowFixMe: https://github.com/rt2zz/redux-persist/issues/823
+  // https://github.com/rt2zz/redux-persist/issues/823
+  // $FlowFixMe[incompatible-variance]
  storage: ZulipAsyncStorage,


This is another nice illustration of how it's helpful to keep the text explaining the fixme explicitly attached to the fixme. As is, it looks like this issue link is probably there to add more context on the "Store data through … compression" comment, rather than to explain the fixme.

gnprice · 2021-01-26T01:58:20Z

src/webview/html/template.js

@@ -14,7 +14,7 @@ import escape from 'lodash.escape';
 *   template`Hello $\!${&<world}` -> 'Hello $!&amp;&lt;world'
 */
 export default (strings: string[], ...values: Array<string | number>) => {
-  // $FlowFixMe github.com/facebook/flow/issues/2616
+  // $FlowFixMe[prop-missing] github.com/facebook/flow/issues/2616


Separately: this change in Flow called my attention to a feature/convention that's long existed, and would be handy to use here!

$FlowIssue: for a type error that you suspect is an issue with Flow

https://flow.org/en/docs/errors/

No rush to do this at the same time as adding the error codes, but probably we should use $FlowIssue for at least the cases where we actually link to an issue in the Flow tracker 🙂

chrisbobbe · 2021-01-27T19:32:58Z

Thanks for the review! Revision pushed.

gnprice

Thanks for the revision! Small comments below -- otherwise looks good.

gnprice · 2021-01-27T23:17:03Z

src/compose/ComposeBox.js

+        {/*
+          `MentionWarnings` should use a type-checked `connect`
+          $FlowFixMe[incompatible-use]
+        */}


Here's one remaining comment that gets out of order.

Ah, indeed, thanks (and for the other one too).

gnprice · 2021-01-27T23:17:19Z

src/utils/__tests__/internalLinks-test.js

+  /* realm_ is URL | void, but complains of out-of-bounds access
+     $FlowFixMe[invalid-tuple-index] */


Here's another (I think the only other.)

gnprice · 2021-01-27T23:18:07Z

src/boot/replaceRevive.js

    // Flow bug: https://github.com/facebook/flow/issues/6110
-    case (ZulipVersion.prototype: $FlowFixMe):
+    case (ZulipVersion.prototype: $FlowIssue):


nit in commit message:

flow [nfc]: Better indicate flow bugs in a few suppressions.

Should read "Flow bugs".

When we start using Flow v0.137+, along with React Native v0.64 (that's issue zulip#4426), we'll be asked to specify what rule(s) we're suppressing with each suppression comment. Conveniently, the warnings will tell us the applicable rule(s) we need to write down. So, having peeked ahead at the warnings seen in the RN 0.64.0-rc.2 world, get a head start on doing so. There will probably be more of these (we haven't yet started enforcing the requirement now, so some unmarked `$FlowFixMe`s will naturally accumulate), but we might as well get this chunk out of the way.

See https://flow.org/en/docs/errors/.

chrisbobbe · 2021-01-27T23:37:25Z

Thanks for the review! Merged, after fixing those nits.

Like with the other Flow upgrades in this series, we're departing from our usual practice of only upgrading Flow along with React Native. But we seem to get away with this one -- it doesn't flag any new errors in node_modules/react-native! One highlight of this version in the changelog is "Standardized error suppression syntax, added ability to suppress errors based on error codes" [1]. We couldn't have taken this upgrade without first removing our use of the `suppress_comment` option in our .flowconfig [2]. One slightly annoying feature of the standardized syntax (still present in v0.146, the latest) is that, when you put more than one error-code-specific suppression in a multi-line comment, Flow will ignore some of them. Neither the doc [3] nor the suppression-syntax tests [4] suggest that multiple suppressions in one multi-line comment is supported. We'd been using multi-line comments to keep a suppression's explanatory text (when it's too long for one line) between the suppression and the code that it's about [5]. So, to keep doing that: work around by using single-line suppression comments for all but the last error-code-specific suppression (chosen arbitrarily), and use a multi-line comment for that last one, squeezing the explanatory text in there. Apparently this Flow version finds more to complain about at some of our existing suppressions, so this workaround gets a good bit of exercise, especially in src/boot/store.js. [1] https://github.com/facebook/flow/blob/master/Changelog.md#01270 [2] https://flow.org/en/docs/config/options/#toc-suppress-comment-regex [3] https://flow.org/en/docs/errors/ [4] https://github.com/facebook/flow/blob/v0.146.0/tests/error_codes/test.js [5] zulip#4433 (comment)

This is another round in the spirit of zulip#4433, to prepare for Flow v0.132 and later; there's this in the changelog entry [1]: > Added warnings against suppressions without error codes As we noted there, we still haven't enforced that suppressions have error codes, but at least we can clean up these ones that have crept in. [1] https://github.com/facebook/flow/blob/master/Changelog.md#01320

chrisbobbe requested a review from gnprice January 25, 2021 19:17

chrisbobbe force-pushed the pr-mark-suppression-comments branch from 7397e21 to e5379a2 Compare January 25, 2021 19:19

gnprice mentioned this pull request Jan 26, 2021

Upgrade to RN v0.64 #4426

Closed

gnprice reviewed Jan 26, 2021

View reviewed changes

chrisbobbe force-pushed the pr-mark-suppression-comments branch from e5379a2 to d2d8630 Compare January 27, 2021 19:32

gnprice reviewed Jan 27, 2021

View reviewed changes

chrisbobbe added 2 commits January 27, 2021 18:34

flow [nfc]: Better indicate Flow bugs in a few suppressions.

c0ffc37

See https://flow.org/en/docs/errors/.

chrisbobbe force-pushed the pr-mark-suppression-comments branch from d2d8630 to c0ffc37 Compare January 27, 2021 23:36

chrisbobbe merged commit c0ffc37 into zulip:master Jan 27, 2021

chrisbobbe mentioned this pull request Jul 9, 2021

Flow-related work on the path to RN v0.64 upgrade, part 4. #4891

Merged

chrisbobbe mentioned this pull request Sep 8, 2021

Upgrade to RN v0.64! #4991

Merged

chrisbobbe deleted the pr-mark-suppression-comments branch November 4, 2021 22:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

flow [nfc]: Mark specific rules that are being suppressed. #4433

flow [nfc]: Mark specific rules that are being suppressed. #4433

chrisbobbe commented Jan 25, 2021

gnprice left a comment

gnprice Jan 26, 2021

gnprice Jan 26, 2021

gnprice Jan 26, 2021

chrisbobbe commented Jan 27, 2021

gnprice left a comment

gnprice Jan 27, 2021

chrisbobbe Jan 27, 2021

gnprice Jan 27, 2021

gnprice Jan 27, 2021

chrisbobbe commented Jan 27, 2021 •

edited

Loading

		/* realm_ is URL \| void, but complains of out-of-bounds access
		$FlowFixMe[invalid-tuple-index] */

flow [nfc]: Mark specific rules that are being suppressed. #4433

flow [nfc]: Mark specific rules that are being suppressed. #4433

Conversation

chrisbobbe commented Jan 25, 2021

gnprice left a comment

Choose a reason for hiding this comment

gnprice Jan 26, 2021

Choose a reason for hiding this comment

gnprice Jan 26, 2021

Choose a reason for hiding this comment

gnprice Jan 26, 2021

Choose a reason for hiding this comment

chrisbobbe commented Jan 27, 2021

gnprice left a comment

Choose a reason for hiding this comment

gnprice Jan 27, 2021

Choose a reason for hiding this comment

chrisbobbe Jan 27, 2021

Choose a reason for hiding this comment

gnprice Jan 27, 2021

Choose a reason for hiding this comment

gnprice Jan 27, 2021

Choose a reason for hiding this comment

chrisbobbe commented Jan 27, 2021 • edited Loading

chrisbobbe commented Jan 27, 2021 •

edited

Loading