Skip to content

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.

Foresight Additions

There are some additions that are in-progress for foresight purposes. The old refersTo code makes extensive use of predicates like theIntType() which modernizes to ClassValue::int_(). Not all of the the...Type() predicates have been translated, presumably because they're being moved over on an as-needed basis. It seems useful to just move everything over since most of it is formulaic.

@BekaValentine BekaValentine requested a review from a team as a code owner February 12, 2020 11:00
@BekaValentine BekaValentine changed the title ObjectAPI to ValueAPI: Foresight Additions Python: ObjectAPI to ValueAPI: Foresight Additions 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.

Think this is a great addition, it's kind of annoying having to go in and cherry pick these off one-by-one.

Would also be great if you could follow the naming convention. For example, listType should just be list. Like it is on master), so I guess something tricky happened when you rebased against master.

I have a few suggestions, but otherwise it looks good 👍

@BekaValentine
Copy link
Contributor Author

BekaValentine commented Feb 13, 2020

I would like to propose actually that we move everything over to the ...Type() naming convention to be consistent with the the...Type() predicates, and also so we don't have these strange problems with int_ and float_ etc. which are necessarily atypical b/c they conflict w/ QL keywords. I think it'd be better for everything to follow the same convention even if its more verbose b/c then people will be able to figure it out more easily, than to have special cases for the sake of brevity. Changing the already-existing predicates will require separate effort tho, since they're already used in places, so maybe that's unwise?

BekaValentine and others added 2 commits February 13, 2020 11:12
Co-Authored-By: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>
Co-Authored-By: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>
@tausbn
Copy link
Contributor

tausbn commented Feb 17, 2020

I think for the moment, it's best to stick with the scheme we have. As you point out, if we were to change the API, we would have to deprecate the old predicates, wait a while, and then finally remove them.

Also, I wonder if it would be more confusing to have all of these methods end in Type. This would mean we had ClassValue::nonetypeType (and ClassValue::typeType, even). Note also that we're still forced to keep the first character lowercase, lest the compiler complain.

We should probably clean up in these predicates at some point in the future. I note, for instance, that at present it's nonetype and staticmethod but also nameError, so we're currently not consistent in how we style these (though one could argue that this is closer to what you would write in Python, within the constraints of the CodeQL lexer).

@BekaValentine
Copy link
Contributor Author

I've swapped out the names, such that the predicate name is whats in the string-y name. Some of the pre-existing things are not consistent with this pattern tho, so we might want to push to that convention instead? I've also had to add _ to some of them to avoid conflicts w/ QL naming.

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.

to me it looks good now! 👍

I didn't find any usage of ClassValue::function(), so from my point of view we should go ahead with your renaming that follows the conventions 💯 but let's see if @tausbn has a strong opinion about this

@tausbn
Copy link
Contributor

tausbn commented Feb 18, 2020

I think my opinion on naming can be summed up as "as close to what you would get in Python as possible, lowercase the first letter if necessary, and add a _ suffix if it clashes with a built-in". We don't quite follow this consistently -- builtinFunction would have to be called either builtin_function_or_method (to match what type(len) returns) or builtinFunctionType (to match the types module).

Some of these I'm not sure we need at all, though, or will ever need, for that matter. As far as I can see, theInstanceType, theClassType, and others are never used in our codebase, so there's no need to port them over. (This is in part the reason for why this was done in a piecemeal fashion up to now.)

(Note that some of them are used elsewhere, like the ObjectInternal classType() predicate in ObjectInternal.qll, but this is fine. We're only going to deprecate ...Object, not ...ObjectInternal.)

Perhaps what we really need is a different interface for this. It seems to be that just piggy-backing onto the types module might be the right approach. That is, if people want ClassType, they can write Value::named("ClassType"). Or, at the very least, this can act as a way to access seldom-used types, while still keeping the common ones (e.g. ClassValue::str) close at hand.

(I note in passing that there may be a bug in how we calculate the result of type in our libraries. If you do Value::named("types.InstanceType"), it'll tell you it's a class named _C. This is the class that's used in the types module to define InstanceType, but not InstanceType itself, which seems wrong.)

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

Successfully merging this pull request may close these issues.

3 participants