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.

Sign up for GitHub

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

Jump to bottom

refactored pysa_server.py to stage for Pysa's Language Server specific changes #409

Closed
wants to merge 18 commits into from
Closed

refactored pysa_server.py to stage for Pysa's Language Server specific changes #409

wants to merge 18 commits into from

Conversation

m0mosenpai
Copy link
Contributor

@m0mosenpai m0mosenpai commented Apr 16, 2021 •
edited

Expected outcome for this PR is to display and highlight errors for .pysa files, via the Pysa VSCode Extension.
This is part of the larger issue of creating a VSCode extension for Pysa in the MLH-Fellowship program listed here and has been discussed with @gbleaney before.

Possible milestones:

  • Make template extension code for Pysa by cloning Pyre
  • Add new functions necessary for handling model validation errors to pysa_server.py and persistent.py
  • Connect everything and call get_invalid_taint_models() in query.py
  • Make relevant changes on the extension side
  • Clean up the server of old Pyre code

Output so far:
image

Errors are being calculated but are not being published due to server_state.opened_documents list being empty as show in the output. The publishing diagnostics function iterates over this list and sends the errors to VSCode. It's empty right now and hence, the errors are never published. (Opening/closing documents is not being detected)

@facebook facebook deleted a comment from facebook-github-bot Apr 19, 2021
@gbleaney
Copy link
Contributor

Errors are being calculated but are not being published due to server_state.opened_documents list being empty as show in the output. The publishing diagnostics function iterates over this list and sends the errors to VSCode. It's empty right now and hence, the errors are never published. (Opening/closing documents is not being detected)

It looks like the server state is modified here:

pyre-check/client/commands/v2/pysa_server.py

Line 380 in 3a61444

self.state.opened_documents.add(document_path)

Can you add some debugging statements to see if the process_open_request / process_close_request functions are being called when you open and close files in the plugin?

Also, as a workaround, can you try just publishing all errors, regardless of open and close status?

@m0mosenpai
Copy link
Contributor Author

m0mosenpai commented May 3, 2021 •
edited

Errors are being calculated but are not being published due to server_state.opened_documents list being empty as show in the output. The publishing diagnostics function iterates over this list and sends the errors to VSCode. It's empty right now and hence, the errors are never published. (Opening/closing documents is not being detected)

It looks like the server state is modified here:

pyre-check/client/commands/v2/pysa_server.py

Line 380 in 3a61444

self.state.opened_documents.add(document_path)

Can you add some debugging statements to see if the process_open_request / process_close_request functions are being called when you open and close files in the plugin?

Also, as a workaround, can you try just publishing all errors, regardless of open and close status?

I don't think the functions are being called. Can we skip open/close ? I mean, the publishing of errors relies on document's path to know which file we are talking about.

Also, I was wondering if .pysa files are even being tracked in the first place. Maybe it doesn't know which files to monitor for open/close status. Anyway I can make sure if that part is working or not?

EDIT:
image

I bypassed the opened_documents list and hardcoded the path to the .pysa file and I'm now seeing the errors being published. Though they aren't proper as they should be. I'm guessing this is a typeshed related issue or something?

@facebook-github-bot
Copy link
Contributor

@m0mosenpai has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@m0mosenpai has updated the pull request. You must reimport the pull request before landing.

6 similar comments
@facebook-github-bot
Copy link
Contributor

@m0mosenpai has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@m0mosenpai has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@m0mosenpai has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@m0mosenpai has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@m0mosenpai has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@m0mosenpai has updated the pull request. You must reimport the pull request before landing.

@m0mosenpai m0mosenpai changed the title added support for model validation errors for Pysa VSCode Extension added support for model validation errors in Pysa VSCode Extension May 24, 2021
@facebook-github-bot
Copy link
Contributor

@m0mosenpai has updated the pull request. You must reimport the pull request before landing.

@m0mosenpai m0mosenpai mentioned this pull request May 24, 2021
Closed
@m0mosenpai
Copy link
Contributor Author

Current working outputs:

image
image
image

Where it fails/ doesn't work as expected:

#431

@m0mosenpai m0mosenpai marked this pull request as ready for review May 24, 2021 12:39
gbleaney
gbleaney requested changes May 26, 2021
View reviewed changes
Copy link
Contributor

@gbleaney gbleaney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Giving a preliminary review here, pending the split into multiple diffs that we discussed yesterday

client/commands/v2/persistent.py Outdated Show resolved Hide resolved
client/commands/v2/start.py Outdated Show resolved Hide resolved
documentation/pysa_tutorial/exercise2/.pyre_configuration Outdated Show resolved Hide resolved
documentation/pysa_tutorial/exercise2/views.py Outdated Show resolved Hide resolved
tools/ide_plugins/pysa-vscode/src/main.ts Show resolved Hide resolved
client/commands/v2/pysa_server.py Outdated Show resolved Hide resolved
client/commands/v2/pysa_server.py Show resolved Hide resolved
client/commands/v2/pysa_server.py Outdated Show resolved Hide resolved
client/commands/v2/pysa_server.py Outdated Show resolved Hide resolved
client/commands/v2/pysa_server.py Outdated Show resolved Hide resolved
@facebook-github-bot
Copy link
Contributor

@m0mosenpai has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@m0mosenpai has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@m0mosenpai has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@m0mosenpai has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@m0mosenpai has updated the pull request. You must reimport the pull request before landing.

@m0mosenpai m0mosenpai changed the title added support for model validation errors in Pysa VSCode Extension refactored pysa_server.py to stage for Pysa's Language Server specific changes May 29, 2021
@facebook-github-bot
Copy link
Contributor

@m0mosenpai has updated the pull request. You must reimport the pull request before landing.

gbleaney
gbleaney reviewed Jun 1, 2021
View reviewed changes
client/commands/v2/pysa_server.py Outdated Show resolved Hide resolved
client/commands/v2/pysa_server.py Outdated Show resolved Hide resolved
client/commands/v2/pysa_server.py Outdated Show resolved Hide resolved
@facebook-github-bot
Copy link
Contributor

@gbleaney has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@m0mosenpai @gbleaney
Update client/commands/v2/pysa_server.py
2ed97a1 
Co-authored-by: Graham Bleaney <gbleaney@gmail.com>
@facebook-github-bot
Copy link
Contributor

@m0mosenpai has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@m0mosenpai has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@gbleaney has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@m0mosenpai m0mosenpai mentioned this pull request Jun 1, 2021
Open
arthaud
arthaud requested changes Jun 1, 2021
View reviewed changes
client/commands/v2/pysa_server.py Outdated Show resolved Hide resolved
@facebook-github-bot
Copy link
Contributor

@m0mosenpai has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@gbleaney has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@gbleaney merged this pull request in 4ac80be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gbleaney gbleaney gbleaney requested changes

@arthaud arthaud arthaud requested changes

Assignees
No one assigned
Labels
CLA Signed Merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@m0mosenpai @gbleaney @facebook-github-bot @arthaud