Fix [declarations] not suppressing errors correctly & rename to [silence]

[STRML]

Overview

In #4916, @LegNeato wrote a feature to block reporting of errors in files matching regex patterns as [silence] in .flowconfig. This sat for about 9 months as an open PR despite strong public support. Just before merging, this was renamed to [declarations].

Why do we need it? As you can read in #4916's comments, and as I wrote in #4916 (comment), nearly every Flow version introduces breaking changes. A library written for a given Flow version is unlikely to work with any other version. This is especially true of very complex libraries like react-native that seem to break on a monthly basis.

Unfortunately, by default, Flow reports errors internal to a module, even if these internal errors don't affect any user code. This means that, if a library uses a new feature that is not in the user's current version of Flow, or uses a feature that has been changes (like object types moving to exact-by-default), internal errors will abound. These errors make it difficult to distinguish actual user type errors, and make it impossible to rely on the flow binary's exit code for CI.

The current alternatives are [ignore], which causes import errors, and [untyped], which casts to any. [declarations]has the crucial distinction of leaving exported typedefs intact while ignoring internal errors, making it much easier to keep typedefs active while your project and its dependencies ebb and flow across many Flow versions.

Unfortunately, #4916 did not actually implement [silence] correctly, and doubly unfortunate was the naming pushed through in the final hour. This PR fixes the implementation. Additionally, it renames [declarations] back to [silence]. This was not taken lightly but has been done due to the overwhelming majority of feedback preferring [silence] and the confusion surrounding [declarations] as it relates to "declaration files" (i.e. .js.flow files) and "type declarations" generally. This becomes even more confusing when comparing to other .flowconfig directives such as [include].

What Was Wrong

There are two major flaws in the original approach:

1. Silencing errors at the merge site (Context.remove_all_errors cx) 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 in this feature.

Commits

Fixes #6631, #6717, #7803.

27fa53f is the first commit, which fixes [declarations] as written.

We now suppress entire files in error collation, at the very end
of the error reporting process.

---

The second commit is a0aa29e.

This backs out some of the code
in #4916 related to file error suppression in the merge phase.

This has the benefit of fixing erroneous ignoring of comment
suppressions within those files, which was causing unexpected
consequences as listed in 2) above.

---

The last commit is a2e11e1.

We rename [declarations] to [silence]
This name has not gone over well: #6631 (comment)

and is confusing in multiple places, such as:
https://github.com/facebook/flow/blob/dd93de0a3796897fe07cca8a3bdc621c992a9880/website/en/docs/declarations/index.md
and

flow/tests/lib_interfaces/.flowconfig

Lines 5 to 6 in dd93de0

	[libs]
	declarations

It is difficult to grep for, difficult to distinguish versus type declarations,
and is widely used interchangeably with "interfaces".

For these reasons and many more, it is best to rename this option now that it
is usable.

Pinging @mroch and @mrkev who helped get the original version of this in.

[STRML]

Ping @nodkz @LegNeato

[FezVrasta]

Personally I would have loved to see two separate PRs, one with the fix, and one with the rename.

I don't want to see the fix blocked because of the time it will take to decide if the team wants to rename the feature or not... (considered it took months to finally ship [declarations])

[STRML]

I go both ways on that. I see wanting to ship the feature asap, but [declarations] is a terrible name for describing what this feature actually does. The feature itself has been unusable since it shipped, so the day it is usable is a good day to rename it.

[FezVrasta]

I get your point, but fixing the existing feature doesn't need much discussions around, it's a merge and ship.

Renaming could potentially take years if we take in consideration similar PRs in the history of this project.

So I'd be grateful to be able to use this feature with a inaccurate name, rather than not being able to use it at all.

[STRML]

Let's see how they respond. This is why I contained the rename to the last commit.

[STRML]

could potentially take years

I sure hope not. The team expressed a new commitment toward the community in https://medium.com/flow-type/what-the-flow-team-has-been-up-to-54239c62004f, let's see how that manifests.

[goodmind]

/cc @gabelevi

[mroch]

Thanks for this! The fix looks good to me. Beyond builtins, this should also fix it in cycles, which was broken because we check cycles in the "leader's" context, which might not be in [declarations], causing those errors to still show up.

I pretty firmly disagree with renaming it back to [silence], though. the current implementation, checking these files just to throw away all the work, is not where we want to be. in Types First mode, which will eventually be the default/only mode, we should not even check these files -- just extract its signatures, aka declarations, aka .js.flow files. (I don't think we've made that optimization yet but we should). so, (ideally) it's not silencing anything because there are no errors, because we don't actually check it.

I think we should merge the first 2 commits (again, they look great) and can continue the naming debate in an issue.

[mrtnzlml]

@mroch Could you please consider importing it without the last commit? This would be a great DX improvement. Thanks! :)

[STRML]

@mroch Thanks for responding - I am more interested in getting this merged than in spending time bikeshedding the name. I am focused on the DX of using Flow, which is why this feature should exist in the first place. To be fair, the name is part of putting Flow users first and addressing their needs (i.e. "this file I don't own is throwing errors, I would like to silence them"). But they will get used to whatever feature is put in front of them, if it is genuinely useful.

I will remove the last commit from this PR so it can be merged wholesale.

[STRML]

Last commit is removed, merge conflict fixed, assuming build goes green we are clear for landing.

[facebook-github-bot]

@mroch is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[mroch]

here we go!

since this PR is going to get closed and be hard to find, I'd love to continue the naming discussion in an issue.

"this file I don't own is throwing errors, I would like to silence them"

that's a pretty good point. i think [declarations] could include */node_modules/* by default, so hopefully you'd never run into that bad experience.

[facebook-github-bot]

@mroch merged this pull request in d37a1ce.