-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Rust: Add Callable
as a base class of Function
and ClosureExpr
#17736
Conversation
@@ -21,6 +21,7 @@ | |||
import codeql.rust.elements.BreakExpr | |||
import codeql.rust.elements.CallExpr | |||
import codeql.rust.elements.CallExprBase | |||
import codeql.rust.elements.Callable |
Check warning
Code scanning / CodeQL
Redundant import Warning
codeql.rust.elements.ClosureExpr
Redundant import, the module is already imported inside
codeql.rust.elements.Function
*/ | ||
|
||
private import internal.CallableImpl | ||
import codeql.rust.elements.AstNode |
Check warning
Code scanning / CodeQL
Redundant import Warning
codeql.rust.elements.Attr
Redundant import, the module is already imported inside
codeql.rust.elements.ParamList
@@ -278,7 +279,7 @@ def __or__(self, other: _schema.PropertyModifier): | |||
drop = object() | |||
|
|||
|
|||
def annotate(annotated_cls: type, replace_bases: _Dict[type, type] | None = None) -> _Callable[[type], _PropertyAnnotation]: | |||
def annotate(annotated_cls: type, add_bases: _List[type] | None = None, replace_bases: _Dict[type, type] | None = None) -> _Callable[[type], _PropertyAnnotation]: |
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.
the type annotation can be typing.Iterable
here.
Also, could you add a unit test in test_schemaloader.py
? Should be easy copying and adapting test_annotation_replace_bases
.
@@ -295,6 +296,8 @@ def decorator(cls: type) -> _PropertyAnnotation: | |||
_ClassPragma(p, value=v)(annotated_cls) | |||
if replace_bases: | |||
annotated_cls.__bases__ = tuple(replace_bases.get(b, b) for b in annotated_cls.__bases__) | |||
if add_bases: | |||
annotated_cls.__bases__ = tuple(annotated_cls.__bases__) + tuple(add_bases) |
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.
Equivalent and shorter:
annotated_cls.__bases__ = tuple(annotated_cls.__bases__) + tuple(add_bases) | |
annotated_cls.__bases__ += tuple(add_bases) |
""" | ||
param_list: optional["ParamList"] | child | ||
attrs: list["Attr"] | child | ||
|
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.
you can add body: optional[Expr] | child
here and drop it on the two derived classes, and then reintroduce the more specific return type in the FuncionImpl.qll
file with
override BlockExpr getBody() { result = Generated::Function.super.getBody() }
This technique might get problematic with list
fields, as the generated getA...
is final
and its type cannot be overridden, but for optional
the only generated auxiliary predicate is has...
which remains valid.
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.
hmm, turns out that doesn't work out of the box, as the extractor would need an into()
through an optional inserted into generated code, which is probably not worth the hassle. I have some ideas, but they're better off for a follow-up
param_list: optional["ParamList"] | child | ||
attrs: list["Attr"] | child |
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.
minor nit, string type references are only required for "forward" references
param_list: optional["ParamList"] | child | |
attrs: list["Attr"] | child | |
param_list: optional[ParamList] | child | |
attrs: list[Attr] | child |
I'll merge this as-is and open a follow-up PR. |
Function
andClosureExpr
now sharegetAttr
andgetParamList
. Ideally, they should also sharegetBody
, but that currently doesn't work because they have different return types (BlockExpr
andExpr
, respectively).