[Fall 2021] Step 3: VSCode Plugin - Handle model validation errors #4
Assignees
Projects
Comments
Open
5 tasks
facebook-github-bot
pushed a commit
to facebook/pyre-check
that referenced
this issue
Jun 8, 2021
…c changes (#409) Summary: 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](MLH-Fellowship#4) and has been discussed with gbleaney before. Possible milestones: - [X] Make template extension code for Pysa by cloning Pyre - [X] Add new functions necessary for handling model validation errors to `pysa_server.py` and `persistent.py` - [X] Connect everything and call `get_invalid_taint_models()` in `query.py` - [X] Make relevant changes on the extension side - [x] Clean up the server of old Pyre code Output so far:  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) Pull Request resolved: #409 Reviewed By: arthaud Differential Revision: D27857795 Pulled By: gbleaney fbshipit-source-id: b9e8b8026aa48ee5d79db2ed9643ea7e70148e04
m0mosenpai
pushed a commit
that referenced
this issue
Aug 24, 2021
Summary: More detailed discussion about allowing refinement to stick if there are no interleaving calls: https://fb.workplace.com/groups/657557118131740/permalink/907322746488508/ I'd like to get some input on a few different ways we could store "temporary" annotation information, ie., local type annotations that we know but want to be able to wipe away the next time we see any call. There are a few layers where this might live. 1. Inside typecheck.ml as a new field in `State.t` 2. Inside resolution.ml as a new field in `t`. 3. Inside `annotation_store`, a field of `Resolution.t` - so instead of keeping one `RefinementUnit.t Reference.Map.t`, we'd keep around two of these. 4. Inside RefinementUnit.t as a flag on each base annotation 5. Inside Annotation.t as a new flag on immutable annotations. I believe that #1 and #2 are too high level - we treat `annotation_store` as capturing all of the local type information at any state during type checking, and it seems messy to spill local typing information outside of this object. I could see this leading to bugs where we aren't joining everything necessary or remembering to check in two places for relevant local type info. I believe #5 is considerable but potentially so low-level that it introduces too many headaches having to match out this extra complexity every time we deal with a basic annotation object. I think #3 and #4 are the best potential options, and this diff sketches #3. Some tradeoffs: #3 Pros/Cons: (+) `annotation_store` still contains the full world local type information (-) duplicate information needs to be stored. ie., if you have `a.b.c` as a temporary refinement, whereas `a.b` is a non-temporary local known type, you'll have an entry in both tables with the same refinement unit structure, just with the temporary table going a layer deeper. Any time the annotation store changes you'd want to wipe the corresponding entry in the temporary annotation store (+) invalidating the whole thing is constant time - you can just set the temporary map to empty #4 Pros/Cons: (+) no data duplication (+) simpler interface for `resolution.ml` - the most you'll need to expose is a flag on `set_local` on whether or not the set we're doing is a temporary refinement (-) really expensive to invalidate, which honestly will be the overwhelming bulk of the operations done against this data store. We'll invalidate on every single call. As a result, I think the only way this is tenable is to add some kind of unfortunate extra complexity to make invalidation constant time, such as keeping a "validation counter" that ticks up by 1 when we want to invalidate, and keeping the "is_temporary" flag as an int instead of a bool. When we do the read, we can check whether the temporary data is still valid or not. I think this complexity makes this option a lot less appealing. Thoughts on what the best option is? Is there a good model for this I didn't think of? Thanks in advance! Reviewed By: grievejia Differential Revision: D30351486 fbshipit-source-id: c25ae7bc663e625f5d7cfb84ad226d0456359cf8
onionymous
changed the title
onionymous
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Now that we have a separate running server for Pysa, let's start customizing it for Pysa. This is where the real engineering work comes in, so this task and the tasks going forward will be a bit more open ended. Have fun, and don't be afraid to ask for help!
At a high level, our goal is to get model validation errors that you can access through
pyre query "validate_taint_models()"to start showing up in.pysafiles. We can start by running the validation every time.pysafiles are saved, and reporting the results back to VSCode.A few notes:
PysaServer, to remove anything unrelated to the pysa functionality you're addingvalidate_taint_modelsfunctionality is available through the query API, so you'll probably want to get the model validation data back through there, rather than by trying to do the parsing yourself.The text was updated successfully, but these errors were encountered: