Skip to content

Commit

Permalink
fix(declarations): suppress errors after merge, not during
Browse files Browse the repository at this point in the history
Fixes facebook#6631, facebook#6717, facebook#7803.

There are two major flaws in the original approach:

1. Silencing errors at the merge site does not work for builtins,
as those errors are computed later, and
2. Putting that suppression in an if/else where the else block
parsed comment error suppressions can cause [declarations] to
actually create *more* errors, which was misleading a lot of people
and masked the source of bugs.

We now suppress entire files in error collation, at the very end
of the error reporting process.
  • Loading branch information
STRML committed Sep 27, 2019
1 parent 5875a36 commit 27fa53f
Show file tree
Hide file tree
Showing 9 changed files with 132 additions and 2 deletions.
9 changes: 7 additions & 2 deletions src/typing/errors/error_suppressions.ml
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,11 @@ let in_node_modules ~root ~file_options loc =
| None -> false
| Some (file, options) -> Files.is_within_node_modules ~root ~options (File_key.to_string file)

let in_declarations ~file_options loc =
match Option.both (Loc.source loc) file_options with
| None -> false
| Some (file, options) -> Files.is_declaration options (File_key.to_string file)

let check ~root ~file_options (err : Loc.t Errors.printable_error) (suppressions : t) (unused : t)
=
let locs =
Expand All @@ -176,11 +181,11 @@ let check ~root ~file_options (err : Loc.t Errors.printable_error) (suppressions
* without a source. *)
|> List.filter (fun loc -> Option.is_some (Loc.source loc))
in
(* Ignore lint errors from node modules. *)
(* Ignore lint errors from node modules, and all errors from declarations directories. *)
let ignore =
match Errors.kind_of_printable_error err with
| Errors.LintError _ -> in_node_modules ~root ~file_options (Errors.loc_of_printable_error err)
| _ -> false
| _ -> in_declarations ~file_options (Errors.loc_of_printable_error err)
in
if ignore then
None
Expand Down
14 changes: 14 additions & 0 deletions tests/config_declarations_react/.flowconfig
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
[ignore]

[include]

[libs]

[declarations]
<PROJECT_ROOT>/node_modules/react-native/.*

[options]
suppress_comment=.*\\$FlowFixMe
suppress_comment=.*\\$FlowIssue
suppress_comment=\\(.\\|\n\\)*\\$FlowNewLine
include_warnings=true
14 changes: 14 additions & 0 deletions tests/config_declarations_react/A.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// Due to [declarations], this won't error
const str2str = require('react-native/str2str')

// But this identical one will
const str2str_loud = require('react-d3/str2str')

// Calling str2str with a string is correct
const foo: string = str2str(' bar ');

// But not with a number
const fooNum: number = str2str(39); // Would not error if using [untyped]!

// trim2 should still error as well
str2str(39);
5 changes: 5 additions & 0 deletions tests/config_declarations_react/B.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
const React = require('React');

// Will error. Ensuring this still errors properly despite the react-native decl
const AsyncMode: boolean = React.unstable_AsyncMode;

63 changes: 63 additions & 0 deletions tests/config_declarations_react/config_declarations_react.exp
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
Error ------------------------------------------------------------------------------------------------------- A.js:11:24

Cannot assign `str2str(...)` to `fooNum` because string [1] is incompatible with number [2].

A.js:11:24
11| const fooNum: number = str2str(39); // Would not error if using [untyped]!
^^^^^^^^^^^

References:
node_modules/react-native/str2str.js:7:40
7| module.exports = function str2str(foo: string) {
^^^^^^ [1]
A.js:11:15
11| const fooNum: number = str2str(39); // Would not error if using [untyped]!
^^^^^^ [2]


Error ------------------------------------------------------------------------------------------------------- A.js:11:32

Cannot call `str2str` with `39` bound to `foo` because number [1] is incompatible with string [2].

A.js:11:32
11| const fooNum: number = str2str(39); // Would not error if using [untyped]!
^^ [1]

References:
node_modules/react-native/str2str.js:7:40
7| module.exports = function str2str(foo: string) {
^^^^^^ [2]


Error -------------------------------------------------------------------------------------------------------- A.js:14:9

Cannot call `str2str` with `39` bound to `foo` because number [1] is incompatible with string [2].

A.js:14:9
14| str2str(39);
^^ [1]

References:
node_modules/react-native/str2str.js:7:40
7| module.exports = function str2str(foo: string) {
^^^^^^ [2]


Error -------------------------------------------------------------------------------------------------------- B.js:4:28

Cannot get `React.unstable_AsyncMode` because property `unstable_AsyncMode` is missing in exports [1].

4| const AsyncMode: boolean = React.unstable_AsyncMode;
^^^^^^^^^^^^^^^^^^^^^^^^


Error ---------------------------------------------------------------------------- node_modules/react-d3/str2str.js:4:19

Cannot get `React.unstable_AsyncMode` because property `unstable_AsyncMode` is missing in exports [1].

4| const AsyncMode = React.unstable_AsyncMode;
^^^^^^^^^^^^^^^^^^^^^^^^



Found 5 errors

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Empty file.
11 changes: 11 additions & 0 deletions tests/config_declarations_react/node_modules/react/package.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit 27fa53f

Please sign in to comment.