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

Move error reporting and initialisation clean-up #135

Merged
merged 13 commits into from
Feb 17, 2020
Merged

Move error reporting and initialisation clean-up #135

merged 13 commits into from
Feb 17, 2020

Conversation

amandasystems
Copy link
Contributor

@amandasystems amandasystems commented Oct 15, 2019 •
edited
Loading

This PR mainly implements move error reporting. It also:

  • No longer assumes child is transitive (i.e. actually maps some ancestor relation); children are now direct children, i.e child(Daughter, Mother) but not child(Daughter, Grandmother). This means it is now safe to simplify the generation code in Rust. This closes the Polonius part of child facts should only contain immediate children #120.
  • Does the elaboration of move paths mentioned by Niko.
  • Removes the strictly unnecessary imprecision with regards to move paths (addressed in the review of my previous PR).
  • Updats the names of all facts in accordance with the new formulation, and uses the logic described there (with minor modifications).
  • Removes a lot of unused input facts
  • Does some slight renaming of inputs in polonius-parser

Left to do:

As mentioned in one of the comments below, polonius-parser and the unit tests need a lot of work to function with this set of new facts. The current version of polonius-parser is essentially a re-skin of the old version with the new fact names. It has no facilities for generating any of the new facts, which means that it cannot exercise any of the new logic. Additionally, there's an open and ongoing discussion about what precisely the model language used by the parser should look like, how much work it should do on its own, etc.

@amandasystems
Copy link
Contributor Author

amandasystems commented Nov 26, 2019 •
edited
Loading

Facts left to add to polonius-parser:

  • var_used_at
  • var_dropped_at
  • var_drops_region
  • var_defined_at
  • use_of_var_derefs_origin
  • drop_of_var_derefs_origin
  • child_path (I don't use Niko's reversed parent_path as I need it in this reversed form for joins due to first-tuple-part-join-limitations in Datafrog)
  • path_begins_with_var
  • path_assigned_at_base
  • path_accessed_at_base
  • path_moved_at_base

There are probably also additionally renamings.

I think we should abbreviate the names of dops, uses, and moves to just drop(x), use(x), etc. I also think there should be a single assigned(x) for both initialization-assignment and move analysis-assignment. The question then becomes how smart we should be about e.g. automatically finding variables and their paths (if I use(x.y), does that automatically declare x.y with their entire hierarchy including the root var, and emit path_moved_at_base for e.g. drops?

This was referenced Feb 16, 2020
Closed
Open
Open
@amandasystems amandasystems changed the title [WIP] Move error reporting and initialisation clean-up Move error reporting and initialisation clean-up Feb 16, 2020
@amandasystems
Copy link
Contributor Author

I have removed the WIP marker because I think this is is perhaps ready to merge now.

@lqd
Copy link
Member

lqd commented Feb 17, 2020

The existing tests in polonius-parser seem to fail compiling, probably fallout from the fact renaming.

@lqd
Copy link
Member

lqd commented Feb 17, 2020

Sorry but it's hard to tell from the GH interface: it seems most/all of the unused input facts are removed by this PR, but some are still referenced in the inputs/collect-facts.sh script ?

@amandasystems
Copy link
Contributor Author

The existing tests in polonius-parser seem to fail compiling, probably fallout from the fact renaming.

That's really weird! Or are they not run by cargo test in the root? They all pass on my machine (tm).

@amandasystems
Copy link
Contributor Author

Sorry but it's hard to tell from the GH interface: it seems most/all of the unused input facts are removed by this PR, but some are still referenced in the inputs/collect-facts.sh script ?

Correct; that's an oversight I'll have to fix!

@amandasystems amandasystems changed the title Move error reporting and initialisation clean-up [WIP] Move error reporting and initialisation clean-up Feb 17, 2020
@lqd
Copy link
Member

lqd commented Feb 17, 2020

As the tests are in a subcrate, its tests are run on demand with cargo test --all. This is the command ran by CI and about which it is complaining, for example here https://travis-ci.com/rust-lang/polonius/jobs/287849345 which has the compile error messages around the bottom.

@amandasystems
Copy link
Contributor Author

amandasystems commented Feb 17, 2020 via email •
edited
Loading

Albin Stjerna added 4 commits February 17, 2020 19:20
Clean up and update fact files
80a3ca7 
This removes a LOT of unused facts that we don't need for the unit tests, and
renames and updates those we do need.
Version-bump and update RELEASES.md
b05a8ba
Update collect-facts.sh to only collect the needed facts
ed924db
Update the tests for polonius-parser to use new names
2f60ff2 
Also drive-by update: replace repeated assert `.is_ok()` followed by unwrap with
more succinct `.expect()` to catch (and more importantly print) parser errors.
@amandasystems amandasystems changed the title [WIP] Move error reporting and initialisation clean-up Move error reporting and initialisation clean-up Feb 17, 2020
@lqd
Copy link
Member

lqd commented Feb 17, 2020

🚀

@lqd lqd merged commit d24bac8 into rust-lang:master Feb 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@amandasystems @lqd