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

Inconsistent Warning Behavior in LSP #8875

Open
lyleunderwood opened this issue Apr 27, 2022 · 3 comments
Open

Inconsistent Warning Behavior in LSP #8875

lyleunderwood opened this issue Apr 27, 2022 · 3 comments

Comments

@lyleunderwood
Copy link
Contributor

lyleunderwood commented Apr 27, 2022

Flow version:

Expected behavior

All warnings generated by flow are consistently either sent to the IDE via LSP or not.

Actual behavior

Warnings from lints get to the IDE via LSP but "built-in" warnings such as "Unused suppression comment" and "Suppression is missing a code" do not (EUnusedSuppression and ECodelessSuppression).

image

Screenshot from VS Code. Note that line 5 will generate a warning in the CLI output when running npx flow but no warning has made it to the IDE.

@mroch
Copy link
Contributor

mroch commented May 1, 2022

this is a bug with include_warnings=true. I attempted to fix this in 0.160.0 and 1.160.1 but it kept breaking stuff so I reverted the attempts in 0.160.2.

the bug is related to how we store and clear errors during a recheck. we detect suppression comments in one phase, and errors in another. sometimes during a recheck we can skip the second phase if we know the inputs didn't change, but this causes us to see suppressions but no errors, leading to "unused" suppressions.

internally, we decided to turn off include_warnings=true to mitigate this. we run a nightly cron job that uses tool update-suppressions --only remove (tool is in packages/flow-dev-tools) to remove unused suppressions, instead of warning and making users deal with it.

@mroch
Copy link
Contributor

mroch commented May 10, 2024

@SamChou19815 do you think your recent collation fixes might've fixed the issues detecting unused suppressions, making it safe to use include_warnings?

@SamChou19815
Copy link
Contributor

No, it remains risky due to the potential presence of misplaced errors + lazy mode, but we can at least start to show all other warnings.

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

3 participants