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

[apex] UnusedLocalVariable flags for variables which are using in SOQL/SOSL binds #4556

Closed
nwcm opened this issue May 12, 2023 · 7 comments · Fixed by #4562
Closed

[apex] UnusedLocalVariable flags for variables which are using in SOQL/SOSL binds #4556

nwcm opened this issue May 12, 2023 · 7 comments · Fixed by #4562
Labels
a:false-positive PMD flags a piece of code that is not problematic
Milestone

Comments

@nwcm
Copy link
Contributor

nwcm commented May 12, 2023

Affects PMD Version: 7.0.0-rc2

Rule: UnusedLocalVariable

Description:

Code Sample demonstrating the issue:

    String unusedFlag = 'this string';
    String query = 'SELECT Id FROM Account WHERE Name =: unusedFlag';

Expected outcome:

PMD reports a violation at line 1, but that's wrong. That's a false positive.

Running PMD through: [CLI]

image

@nwcm nwcm added the a:false-positive PMD flags a piece of code that is not problematic label May 12, 2023
@adangel adangel changed the title [APEX] UnusedLocalVariable flags for variables which are using in SOQL/SOSL binds [apex] UnusedLocalVariable flags for variables which are using in SOQL/SOSL binds May 12, 2023
@adangel
Copy link
Member

adangel commented May 12, 2023

Thanks for reporting.

The rule should support binding variables already. The implementation of the rule expects, that between the colon (:) and the variable name is no space, you have a space, though. Is this still valid syntax?

    String unusedFlag = 'this string';
    String query = 'SELECT Id FROM Account WHERE Name =: unusedFlag';
    //                                                  ^
    //                                                  | is this space allowed?

@adangel
Copy link
Member

adangel commented May 12, 2023

Oh, and one more thing: You don't use the string yet in a query, so PMD can't know, that this string is supposed to be a SOQL/SOSL query...
You need at least something like Database.query(query);

See also #2669

@nwcm
Copy link
Contributor Author

nwcm commented May 15, 2023

@adangel Thanks for the comments,

Sorry I must have missed it in my example. Regardless of =: or =: which are both valid at least in v56 as below it will report unused

image

@nwcm
Copy link
Contributor Author

nwcm commented May 15, 2023

If we update the BINDING_VARIABLE regex to (?i):(?:\s|)([_a-z0-9]+) it will capture all the possible

I have yet to test it, but i have opened a draft PR for it. Though even =:foo does incorrect throw unused variable in 7.0.0rc2

#4562

@adangel
Copy link
Member

adangel commented May 16, 2023

Note: Your screenshot shows, that it's never working - with or without a space. The problem seems to be, that https://github.com/ChuckJonas/vscode-apex-pmd still uses PMD 6.42.0 (see https://github.com/ChuckJonas/vscode-apex-pmd/tree/master/bin/pmd/lib) and support for binding vars in dynamic queries was added only with PMD 6.54.0 (see #2669).

Though even =:foo does incorrect throw unused variable in 7.0.0rc2

That I can't reproduce. Are you sure, you are using PMD 7.0.0-rc2?

The false positive with using a space before the colon is still valid though and needs to be fixed...

@nwcm
Copy link
Contributor Author

nwcm commented May 17, 2023

Good catch @adangel I was on another device and I hadn't configured the pmd bin path.

So this issue and suggestions are more for variable names with _ and the space in bindings

image

@dschach
Copy link

dschach commented Jun 1, 2023

I'm reading this thread, and I'm not sure that @adangel question was answered:

I write SOQL as: WHERE AccountId = :a.Id. From what I'm seeing here, it looks like that would NOT be acknowledged by PMD, but it should be. So I write false positives?

Technically, the spaces between AccountId and a are ALL OPTIONAL, if we're getting pedantic about what SOQL allows.

(FWIW, most of the code I see types like I do, but that may be confirmation bias 😄 )

@adangel adangel added this to the 7.0.0 milestone Mar 14, 2024
adangel added a commit to nwcm/pmd that referenced this issue Mar 14, 2024
adangel added a commit to nwcm/pmd that referenced this issue Mar 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:false-positive PMD flags a piece of code that is not problematic
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants