Skip to content

Python: ObjectAPI to ValueAPI: TruncatedDivision #2817

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.

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

Merged

Conversation

BekaValentine
Copy link
Contributor

This PR is part of the effort to modernize the Expressions queries, moving away from the use of refersTo and associated APIs, and towards pointsTo and its associated APIs.

Issues with Truncated Division

Truncated Division is informally specified as being applicable in a broad sense, with the example of the average function given, but the way pointsTo and refersTo work, there are no values for function calls to sum and len and maybe others. As a consequence, the canonical examples given for this query also don't produce any results when the query is run on them.

The long term solution is to take a look at why function calls don't have values associated with them and fix this. I suspect that it has to do with the complexity of global flow. We may need to loosen some constraints for functions that are well known? For instance, it seems reasonable to say that while we maybe don't know precisely what value sum([1,2,3]) points to, we know that [1,2,3] is a list of integers so sum([1,2,3]) is also some integer or other. I'm not sure how feasible this is tho.

@BekaValentine BekaValentine requested a review from a team as a code owner February 12, 2020 10:55
@BekaValentine BekaValentine changed the title ObjectAPI to ValueAPI: TruncatedDivision Python: ObjectAPI to ValueAPI: TruncatedDivision Feb 12, 2020
@tausbn tausbn self-assigned this Feb 12, 2020
Copy link
Member

@RasmusWL RasmusWL left a comment

Choose a reason for hiding this comment

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

I think you may have missed https://github.com/Semmle/ql/blob/52ddd79cab6cae2e0c7b3c3691655829b58c8bca/python/ql/test/2/query-tests/Expressions/TruncatedDivision.qlref and https://github.com/Semmle/ql/blob/52ddd79cab6cae2e0c7b3c3691655829b58c8bca/python/ql/test/3/query-tests/Expressions/TruncatedDivision/TruncatedDivision.qlref, which are the Python 2 and Python 3 specific tests for this query.

I think you should remove the tests you added to test/query-tests, and use the version specific ones instead.

The reason it worked on your machine is that when you run qltest locally, it will either use Python 2 or Python 3, but not both 😉 I recently got a change merged so qltest will use Python 3 by default. For tests that need to be in a specific version (like the ones in test/2 and test/3), we can force the version with options files (see #2780) or by running qltest --language python2 or qltest --language python3. (but options files will take precedence).

Except for this, there a few things I would like to see changed before merging this commit.

Copy link
Contributor

@tausbn tausbn left a comment

Choose a reason for hiding this comment

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

A few comments, otherwise LGTM.

@BekaValentine BekaValentine force-pushed the objectapi-to-valueapi-truncateddivision branch from 0438b60 to 28be3b4 Compare February 21, 2020 00:04
Copy link
Member

@RasmusWL RasmusWL left a comment

Choose a reason for hiding this comment

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

Nice! 👍

I think it would be really great if you could consolidate the test files into one, so merging your examples into https://github.com/Semmle/ql/blob/2f3ea10cf8526f52237dca7f81d89b85fab6382f/python/ql/test/2/query-tests/Expressions/TruncatedDivision_test.py otherwise it looks good to me 😊

Copy link
Contributor

@tausbn tausbn left a comment

Choose a reason for hiding this comment

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

I think this is pretty much done. Just one final comment to address.

@BekaValentine
Copy link
Contributor Author

@RasmusWL Test files combined and cleaned up. Let me know what you think!

Copy link
Member

@RasmusWL RasmusWL left a comment

Choose a reason for hiding this comment

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

I'm happy 🥇

I'm actually very happy that you explained the test-cases, so the next person doesn't have to spend time starting at the code trying to figure out if the query should give an alert or not 👍 ✨

@tausbn
Copy link
Contributor

tausbn commented Feb 27, 2020

Good stuff. Let's get this in. 🚀

@tausbn tausbn merged commit e099078 into github:master Feb 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants