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

Modifier reused - use code flow analysis #454

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

Conversation

aasitnikov
Copy link

Improves ModifierReusedDetector

  1. Use DataFlowAnalyzer: It replaces the use of obtainAllModifierNames(), and directly tracks references, not just names. This catches more cases of modifier uses, such as tricky Row(modifier = Modifier.then(modifier))
  2. Report only modifier reference location: Instead of reporting the whole method call, report the specific locations where the modifier is being used. This reduces visual clutter and makes problematic modifier uses more visible.
  3. Code flow analysis: Use CFA to detect if modifier is being used multiple times. This allows to write code that was previously considered incorrect:
@Composable
fun Image(modifier: Modifier = Modifier) {
    if (LocalInspectionMode.current) {
        PreviewImage(modifier)
        return
    }
    AsyncImage(modifier)
}

CFA treats blocks and lambdas like simple blocks of code, and also tracks the control flow of return statements and if/when statements. If control flow graph contains path that has two or more such calls, they will be reported.

Copy link

Thanks for the contribution! Unfortunately we can't verify the commit author(s): Alexander Sitnikov <a***@y***.ru>. One possible solution is to add that email to your GitHub account. Alternatively you can change your commits to another email and force push the change. After getting your commits associated with your GitHub account, sign the Salesforce Inc. Contributor License Agreement and this Pull Request will be revalidated.

@aasitnikov aasitnikov changed the title Modifier reused code flow analysis Modifier reused - use code flow analysis Nov 14, 2024
@aasitnikov aasitnikov force-pushed the modifier-reused-code-flow-analysis branch from 0fcf35d to 50fd716 Compare November 14, 2024 18:20
Copy link

Thanks for the contribution! Before we can merge this, we need @aasitnikov to sign the Salesforce Inc. Contributor License Agreement.

is KtReferenceExpression -> {
modifierNames.contains(expression.text)
// Convert UAST into simplified AST, that will be used in code flow analysis
private fun UMethod.toSimplifiedCfaAst(references: Set<PsiElement>): CfaBlock {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we pull this custom AST code out to its own file or package? And ideally with a lot more docs? This looks like it's adding quite a bit of complexity to the check on the face of it

Copy link
Author

Choose a reason for hiding this comment

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

Sure

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

Successfully merging this pull request may close these issues.

2 participants