Skip to content
This repository has been archived by the owner on Dec 1, 2024. It is now read-only.

Suppression comments for HHClientLinter don't mesh well with /**/ comments #407

Open
lexidor opened this issue Nov 25, 2021 · 3 comments
Open

Comments

@lexidor
Copy link
Contributor

lexidor commented Nov 25, 2021

/* HHAST_FIXME[DontAwaitInALoop]
   Why I can't fix this now.
   HHAST_FIXME[5583]*/
    await ...;

The numbered FIXME must be on the literal preceding line. Fixmes for other linters such as DontAwaitInALoopLinter can live anywhere in the preceding comment.

I am expecting this to be a wontfix. I just had to bring it up to be sure.

@Atry
Copy link
Contributor

Atry commented Nov 25, 2021

The reason is that the HHClientLinter uses File::errorCodesToSuppress to determine the error codes suppressed, which is line based instead of range based, therefore only the marker in the previous line is considered.

hhast/src/File.hack

Lines 42 to 45 in 01f1e17

public function errorCodesToSuppress(): shape(
'whole_file' => keyset<this::TErrorCode>,
'single_instance' => dict<this::TLineNumber, keyset<this::TErrorCode>>,
) {

To fix this issue, we can update the implementation of File::errorCodesToSuppress to let it actually parse the source file and produce range based suppression information.

I think this is doable, it just needs us to move the existing AST based linters suppression implementation into File::errorCodesToSuppress.

@fredemmott
Copy link
Contributor

This behavior is shared with line based linters; there's a few solutions.

  • Moving the AST-based solution into one and re-using it is one, but would also mean that these linters would need to construct an AST, find the node, and check trailing markup. That's another process call to hh_parse + the overhead of parsing the JSON and constructing the nodes. Might be worth it though
  • The line-based suppression could be extended to understand multiline comments without a full AST, and treat it as if all suppressions were in the last line of the comment

@lexidor
Copy link
Contributor Author

lexidor commented Nov 26, 2021

Something quite lazy and text based would probably do the trick.

if supp on line
  use supp
else if lastline contains "*/"
  scan until "/*" or until start of file
else if last line contains "//"
  scan while line contains "//"
else
  no supp

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants