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

So many false positives that I doubt it actually works #18359

Open
Scoppio opened this issue Dec 22, 2024 · 2 comments
Open

So many false positives that I doubt it actually works #18359

Scoppio opened this issue Dec 22, 2024 · 2 comments

Comments

@Scoppio
Copy link

Scoppio commented Dec 22, 2024

Description of the false positive

I am constantly getting the following in my Java project.

Useless parameter
The parameter '<A PARAMETER>' is never used. 
Unread local variable
Variable '<A VARIABLE>' is never read. 

This is so frequente that I am at a point that I am seriously doubting the capacity of this tool of really working for whatever it proposes itself to do.

All parameters and variables it points as being useless or unread are always read, sometimes in the literal next line.

Code samples or links to source code

https://github.com/Scoppio/mekhq/blob/79a0f780ba5b70b46deea320962b9f6a4d8bdb19/MekHQ/src/mekhq/campaign/autoresolve/acar/handler/StandardUnitAttackHandler.java#L173

private int[] calculateDamage(
    Formation attacker, // CodeQL - Useless parameter
    StandardUnitAttack attack, // CodeQL - Useless parameter
    SBFUnit attackingUnit, Formation target) {
        int bonusDamage = 0; // CodeQL - Unread local variable 
        if (attack.getManeuverResult().equals(StandardUnitAttack.ManeuverResult.SUCCESS)) {
            bonusDamage += 1;
        }

        var damage = attackingUnit.getElements().stream().mapToInt(e -> e.getStandardDamage().getDamage(attack.getRange()).damage).toArray();
        return processDamageByEngagementControl(attacker, target, bonusDamage, damage);
    }

As you can see, attacker parameter is used as parameter for processDamageByEngagementControl (it uses attacker inside), attack parameter is used when accessing attack.getRange(), and the variable bonusDamage is also used being passed as a parameter to processDamageByEngagementControl which then consumes it.

This is just a sample of my common experience with CodeQL, I often from 12 to 20 false positives sauing a parameter is useless or a variable is never read in all my pull requests.

@jketema
Copy link
Contributor

jketema commented Dec 22, 2024

Hi @Scoppio,

Thanks for your question. This is quite odd; I would expect the mentioned queries to produce hardly any false positives.

I do see in your codeql-analysis.yml workflow file that you're using build-mode: none, and looking at the CodeQL jobs from https://github.com/MegaMek/mekhq/actions/workflows/codeql-analysis.yml, I see quite some errors/exceptions in the output. This can happen, e.g., when a project depends on a lot of generated files. This might cause the created databases to have all kinds of gaps, which might lead to problems like those you describe above.

I would suggest you first of all try one of the other available build modes, i,e,. autobuild or manual. In these cases an actual build will be ran to create a database, which will include all the generated files. Note that if you decide to use the manual mode, you will need to take care to pass --no-deamon to gradle.

Let me know if this helps and whether you have any further questions.

@smowton
Copy link
Contributor

smowton commented Jan 6, 2025

Specifically in this case there are sibling projects that are needed for the code to make sense:

includeBuild('../megamek')
includeBuild('../megameklab')

That means a lot of type information is missing, which stumbles the name-binding analysis, which in turn leads to false unused-variable warnings. While it will be possible to quieten those particular false positives, fundamentally knowing expression types and name bindings is crucial to most CodeQL analyses, so build-mode: none will perform very poorly here without the sibling projects also checked out.

As noted above, using a manual build (and an Action that checks out the needed sibling projects ready for that build) is the best course of action in a situation like this.

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

No branches or pull requests

3 participants