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

QL-for-QL: Add a redundant aggregate query #7472

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

@erik-krogh
Copy link
Contributor

@erik-krogh erik-krogh commented Dec 22, 2021

Inspired by #5160

I haven't fixed the issues identified by this query (I got a patch ready, but I wanted to reduce the scope of the PR).


This query identifies two patterns:

1: A redundant aggregate like:

exists(PredicateExpr alias | alias = this.(ClasslessPredicate).getAlias() |
  result = alias.getArity()
)

Can be simplified to:

result = this.(ClasslessPredicate).getAlias().getArity()

2: Aggregates that can be simplified to a type-cast:
exists(Type t | t = foo()) can be simplified to foo() instanceof Type.
(And any(Type t | t = foo()) can be simplified to foo().(Type)).


Note: an exists(..) is actually a qualifier and not an aggregate, but I'm unsure what else to call something that is either an exists(..) or an any(..).

@erik-krogh erik-krogh force-pushed the redundant-aggregate branch 2 times, most recently from d0ccfd2 to c870d87 Dec 23, 2021
@erik-krogh erik-krogh force-pushed the redundant-aggregate branch from 491cc76 to ad11fec Jan 4, 2022
@erik-krogh erik-krogh marked this pull request as ready for review Jan 4, 2022
@erik-krogh erik-krogh requested a review from tausbn as a code owner Jan 4, 2022
@aschackmull
Copy link
Contributor

@aschackmull aschackmull commented Jan 5, 2022

I disagree with simplifying the exists case in general. There are many cases where it's nice to name an intermediate result to make code more readable. Changing a single-use exists-variable to an instanceof is probably a good thing, though.

Copy link
Contributor

@aschackmull aschackmull left a comment

The exists case catches too much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants