-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Swift: synthesize MethodRefExpr
#10111
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
Conversation
This introduces a `MethodRefExpr` node synthesized out of `DotSyntaxCallExpr` under the `LookupExpr` hierarchy. This means that much like ```free_function(1, 2)``` is a `CallExpr` with `getFunction` giving a `DeclRefExpr`, ```foo.method(1, 2)``` is now a `CallExpr` with `getFunction` giving a `MethodRefExpr`. `ApplyExpr::getStaticTarget` has been made work with it (as well as `ConstructorRefCallExpr` which for the moment has been left where it is), a new `MethodApplyExpr` has been introduced deriving from it, and control and data flow libraries have adapted. A small but was fixed in `qlgen` where the default constructor for DB types was not correctly subtracting derived IPA types depending on the order of definitions in `schema.yml`. There are still some occurrences of `DotSyntaxCallExpr`, and as already mentioned the other `SelfApply` class (`ConstructorRefCallExpr`) was left alone. Their treatment is left for a future PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! A couple of minor comments/suggestions, but nothing blocking this PR.
or | ||
result = f.(MethodRefExpr).getMethod() | ||
or | ||
result = f.(ConstructorRefCallExpr).getFunction().(DeclRefExpr).getDecl() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I imagine we can remove this case once MethodRefExpr
includes ConstructorRefCallExpr
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it depends how we want to do that. If we want constructors to be in the same bag as methods, then yes, though it will mean having to make MethodRefExpr
a superclass of two distinct concrete IPA classes (one synthesized from DotSyntaxCallExpr
and the other from ConstructorRefCallExpr
). The other approach would be to make ConstructorRefExpr
a sibling of MethodRefExpr
under LookupExpr
, then we would edit this line rather than suppress it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. In this case, I think we should follow whatever precedent Java/C# has set. Do you know if their version of MethodRef
also includes constructors (assuming you can actually do this in those languages 🤔)?
Anyway, this is all for a future PR (as you noted as well!)
Also rename `getMethod` to `getMethodDeclaration` to clear up possible confusion with `getFunction`.
`getStaticTarget` gives the same result.
swift/ql/test/extractor-tests/generated/expr/DotSyntaxCallExpr/dot_syntax_call.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
This introduces a
MethodRefExpr
node synthesized out ofDotSyntaxCallExpr
under theLookupExpr
hierarchy. This means thatmuch like
free_function(1, 2)
is a
CallExpr
withgetFunction
giving aDeclRefExpr
,foo.method(1, 2)
is now a
CallExpr
withgetFunction
giving aMethodRefExpr
.ApplyExpr::getStaticTarget
has been made to work with it (as well aswith
ConstructorRefCallExpr
which for the moment has been left where itis), a new
MethodApplyExpr
has been introduced deriving from it,and control and data flow libraries have adapted.
A small bug was fixed in
qlgen
where the default constructor for DBtypes was not correctly subtracting derived IPA types depending on the
order of definitions in
schema.yml
.There are still some occurrences of
DotSyntaxCallExpr
, and as alreadymentioned the other
SelfApply
class (ConstructorRefCallExpr
) wasleft alone. Their treatment is left for a future PR.