feat(code-action): add ignore comment to suppress error

[michaelfortunato]

This PR address #675.

It works by

1. By gathering all the errors whose range contains the current cursor.
2. Finding the column position of the first non-whitespace character of the line the cursor is on. Let us call this column offset.
3. Inserting an ignore comment on the line above the cursor; the comment is preceded by column offset single-spaces to match the prior indentation.

In my tests I have found this simple scheme to be surprisingly robust. My first attempt used the ast exclusively, but the ast does not contain comments, nor a way to insert comments into the tree, meaning, just for this one use case of programmatically adding ignore comments, I cannot update the document using the ast, and I found myself having to calculate whitespace offsets in my first ast only attempt.

It is also worth pointing out that some LSPs might not want to provide this kind of code action--they want to encourage the user to address the underlying cause, not ignore it. But because Pyrefly is new, there are many false positives and so I think this will be a welcome addition. Plus, the suppression comments in this code action reference specific error codes, so the user in the future can address the lints precisely.

Future work is needed to address different types of ignore comments and a code action to

• Add all ignore comments to a module
• Add a top-level ignore comment to a module

Both should be relatively straightforward to do by using the utility functions I added in pyrefly/lib/state/ide.rs

Let me know your thoughts on this.

Thanks!

[meta-cla]

Hi @michaelfortunato!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

[meta-cla]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

[michaelfortunato]

FYI Ruff's wording for their equivalent action is
Ruff (F100): Disable for this line

[connernilsen]

Thanks for doing this @michaelfortunato! @kinto0 thoughts?

[kinto0]

thanks for the contribution! this looks great - a few suggestions

[kinto0]

this argument name is kinda confusing: could we name it something else?

and could you add a comment for why we ned to include ones that are non-empty in the call?

[kinto0]

could you add a test for the difference in only_all true vs false? (I'm still confused as to why it's necessary)

[kinto0]

could this test instead be multiple smaller tests that each test something different? I don't know if you need multiple files either for any of these

• add_ignore_comment_basic
• add_ignore_comment_multiline
• add_ignore_indentation (what happens when the indentation changes?)
• add_to_existing_ignore_basic
• add_to_existing_ignore_multiline
• add_to_existing_ignore_indentation (what happens here when indentation is different?)

[michaelfortunato]

Certainly!

[kinto0]

why do we need insert_import? if we need it for all of these below, could you add a comment clarifying why?

[michaelfortunato]

The get_test_report function in upstream right now applies all code actions returned by local_quickfix_code_actions to the module in question. So for future code actions and the code action in this PR, it would be hard to predict the form of the resulting module if all possible code actions were applied (ignore comment, add import). So I figured its better to have a dedicated report function for each possible code action type. But really it might be best to paramterized get_test_report with a filter function and have the caller determine which code actions it wants applied on the module referenced by the given Handle.

Upshot is I do think get_test_report needs updating to allow us to selectively apply code actions to the associated module.

[kinto0]

In my tests I have found this simple scheme to be surprisingly robust. My first attempt used the ast exclusively, but the ast does not contain comments, nor a way to insert comments into the tree, meaning, just for this one use case of programmatically adding ignore comments, I cannot update the document using the ast, and I found myself having to calculate whitespace offsets in my first ast only attempt.

I wonder if we have any way to get access to the CST... @SamChou19815 did you use this at all for the auto import work or do you have any suggestions?

[SamChou19815]

In my tests I have found this simple scheme to be surprisingly robust. My first attempt used the ast exclusively, but the ast does not contain comments, nor a way to insert comments into the tree, meaning, just for this one use case of programmatically adding ignore comments, I cannot update the document using the ast, and I found myself having to calculate whitespace offsets in my first ast only attempt.

I wonder if we have any way to get access to the CST... @SamChou19815 did you use this at all for the auto import work or do you have any suggestions?

Auto-import does use AST, but only used it to figure out where to insert the import.

[SamChou19815]

Also note that we do have error suppression infra: https://github.com/facebook/pyrefly/blob/main/pyrefly/lib/error/suppress.rs

so we should try to reuse the code over there.

[michaelfortunato]

Hey everyone thanks for taking the time to look and comment on this. I'll address the comments individually and update the pr when I can.

[michaelfortunato]

Also note that we do have error suppression infra: https://github.com/facebook/pyrefly/blob/main/pyrefly/lib/error/suppress.rs

so we should try to reuse the code over there.

Yes, I had looked into that. The issue is the error suppression infra works on files themselves at the moment--so its server side. In contrast, this code action sends the required range and content for the lsp client (IDE) to update. We should probably update the existing suppression system to handle this use case.

[michaelfortunato]

In my tests I have found this simple scheme to be surprisingly robust. My first attempt used the ast exclusively, but the ast does not contain comments, nor a way to insert comments into the tree, meaning, just for this one use case of programmatically adding ignore comments, I cannot update the document using the ast, and I found myself having to calculate whitespace offsets in my first ast only attempt.

I wonder if we have any way to get access to the CST... @SamChou19815 did you use this at all for the auto import work or do you have any suggestions?

So originally I had used pyrefly_python::ast::Ast::locate_node to get the node at the bottom of the parse tree. I then iterated through the list moving up until I found the first statement node, and used that to get the offset and line number. Problem is I found statement to be too large a container--the ignore comment was being applied too far away from the location of the error itself.

  use pyrefly_python::ast::Ast;
  let nodes = Ast::locate_node(module, position);
  let node = nodes.iter().find(|node| node.is_statement()).unwrap();
  let loc = node.range();
  // etc...

Plus, since each error in let errors = self.get_errors(vec![handle]).collect_errors().shown; gives contains its location, I figured it probably best to use that.
Then comes the question of given the precise error location (again, we have that for free with let errors = self.get_errors(vec![handle]).collect_errors().shown;), where do we insert the comment? If we insert the comment right at at this point, we can get some issues.

For instance

X = 4 + Y
#      ^ Error point is here

After insertion (assume we append a carriage return to the comment) becomes

X = 4 + # pyrefly: ignore[code]
Y

Which is not ideal. Hence, instead of inserting the comment right at the location of the error, I found it better to insert it a line above. I emphasize insert as the key point is the comment will be on its own line, so its less likely it'll interfere with surrounding code.

Of course doing this requires us knowing what level of indentation we are at, so at best we case insert the comment right at that level or beyond it so as not to cause a parse error (and let an autoformatter take care of aligning).

Thanks

[samwgoldman]

Maybe we can learn from how Ruff adds noqa comments to the end of lines? Especially since we are using their parser and their AST.