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

Using DotAccess instead of IdQualified for Python decorators #10221

Merged
merged 1 commit into from
May 8, 2024
Merged

Conversation

ihji
Copy link
Contributor

@ihji ihji commented May 8, 2024

test plan: make test

This PR modifies the translation of Python decorators. In Python, it's generally impossible to distinguish whether a given fully qualified name points to a specific program entity or if it's a field access based on the object instance. Previously, we used IdQualified for all Python decorators with multiple components separated by dots. However, this approach encountered an issue when attempting to use a metavariable-type filter on the middle component of the Python decorator. This is because IdQualified doesn't have id_info for its QDot middle component, meaning that the middle component of IdQualified cannot be resolved by the naming phase.

To address this problem, we now translate Python decorators into DotAccess instead of IdQualified. Each field name in DotAccess can be an Id, which has id_info, allowing us to resolve its type information in the naming phase.

Copy link
Contributor

github-actions bot commented May 8, 2024

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 requested a review from aryx May 8, 2024 01:43
@ihji ihji marked this pull request as ready for review May 8, 2024 01:43
@aryx aryx requested review from akuhlens, IagoAbal and nmote May 8, 2024 17:01
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.

some tests/patterns/python/xxx.sgrep test file would be nice.
Otherwise LGTM.

@r2c-argo
Copy link
Contributor

r2c-argo bot commented May 8, 2024

semgrep-compare-github-brzbo results

Ran benchmark on 38 repositories

The number of files checked differs for 1 repos

The number of findings differs for 2 repos

Whole benchmark is 0.1% slower (a bit of noise is expected)

Relative speed improvement is 1.01 on average

  • 3% of scans are significantly faster

Relative memory improvement is 1.00 on average

@ihji ihji merged commit be1c668 into develop May 8, 2024
47 of 49 checks passed
@ihji ihji deleted the code-7006 branch May 8, 2024 23:21
@@ -0,0 +1,3 @@
@$X.route(...)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Didn't this test already work before?

|> G.e)
base
in
G.OtherAttribute
Copy link
Collaborator

Choose a reason for hiding this comment

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

TBH I would have added ExprAttr.

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.

3 participants