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.

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

Making QDots in IdQualified resolvable #10174

Closed
wants to merge 1 commit into from
Closed

Making QDots in IdQualified resolvable #10174

wants to merge 1 commit into from

Conversation

ihji
Copy link
Contributor

@ihji ihji commented Apr 27, 2024

test plan: make test

QDots in IdQualified need to be resolvable if we want to use a metavariable-type filter with the method annotation in Python, as shown in the example below:

from flask import Flask
 
app = Flask(__name__)

@app.get("/items/<item_id>")
def read_item(item_id: str):
    return {"item_id": item_id}
rules:
  - id: test
    message: Test
    languages:
      - python
    severity: WARNING
    patterns:
      - pattern: |
          @$APP.get(...)
          def $FUNC(..., $PARAM, ...):
            ... 
      - metavariable-type:
          metavariable: $APP
          type: Flask

@app.get("/items/<item_id>") is translated in AST as:

attrs=[NamedAttr((),
                IdQualified(
                  {name_last=(("get", ()), None); name_top=None; 
                   name_middle=Some(QDots([(("app", ()), None)])); 
                   name_info={id_info_id=7; id_flags=Ref(0); 
                              id_resolved=Ref(Some((GlobalName(
                                                      ["/Users/heejong/Works/semgrep-proprietary/temp/p";
                                                       "app"; "get"],
                                                      [["app"; "get"]]),
                                                    12)));
                              id_type=Ref(None); id_svalue=Ref(None); };
                   }), [Arg(L(String(("/items/<item_id>", ()))))])];

QDots is a list of ident so we can't infer the type.

Copy link
Contributor

PR checklist:

  • Purpose of the code is evident to future readers
  • Tests included or PR comment includes a reproducible test plan
  • Documentation is up-to-date
  • A changelog entry was added to changelog.d for any user-facing change
  • Change has no security implications (otherwise, ping security team)

If you're unsure about any of this, please see:

@ihji ihji marked this pull request as ready for review May 2, 2024 16:32
@ihji ihji requested a review from IagoAbal as a code owner May 2, 2024 16:32
@IagoAbal IagoAbal requested a review from aryx May 2, 2024 17:36
@@ -124,7 +124,7 @@ let rec map_qualified_name (env : env) (x : CST.qualified_name) :

Copy link
Collaborator

Choose a reason for hiding this comment

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

We need a test plan that actually shows something, some code that one can -dump_ast and see that now those annotations are getting resolved.

@@ -581,7 +581,7 @@ and qualified_info = {

and qualifier =
(* Java/C++/Rust *)
| QDots of (ident * type_arguments option) list
| QDots of (ident * type_arguments option) list * id_info
Copy link
Collaborator

@IagoAbal IagoAbal May 3, 2024

Choose a reason for hiding this comment

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

No strong opinion, but I do wonder if it wouldn't be simpler and more general to just have NameAttr take an expr. We have another ticket/request for supporting sym-prop for attributes. The idents in the QDots are accompanied by type_arguments which to me suggests that they are not meant to encode expressions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes maybe the issue here is how we currently encode app.get as a qualified name when really it's a DotAccess
One issue is that in Python it's not super clear when you see foo.bar.z
whether bar is a module, or a field.
So probably we should go more general and use DotAccess here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe we can have an ExprAttr of ...
or encode it via OtherAttribute.

@IagoAbal
Copy link
Collaborator

IagoAbal commented May 3, 2024

@aryx do you have an opinion on this one?

@aryx
Copy link
Collaborator

aryx commented May 6, 2024

looking

Copy link
Collaborator

@aryx aryx left a comment

Choose a reason for hiding this comment

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

Let's fix how we translate Python instead. Let's not use QDots for
what is really a DotAccess.

@@ -581,7 +581,7 @@ and qualified_info = {

and qualifier =
(* Java/C++/Rust *)
| QDots of (ident * type_arguments option) list
| QDots of (ident * type_arguments option) list * id_info
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes maybe the issue here is how we currently encode app.get as a qualified name when really it's a DotAccess
One issue is that in Python it's not super clear when you see foo.bar.z
whether bar is a module, or a field.
So probably we should go more general and use DotAccess here.

@@ -581,7 +581,7 @@ and qualified_info = {

and qualifier =
(* Java/C++/Rust *)
| QDots of (ident * type_arguments option) list
| QDots of (ident * type_arguments option) list * id_info
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe we can have an ExprAttr of ...
or encode it via OtherAttribute.

@ihji
Copy link
Contributor Author

ihji commented May 9, 2024

superseded by #10221

@ihji ihji closed this May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants