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

Java: PrimitiveType.getADefaultValue() is misleading and might not work correctly #6615

Open
Marcono1234 opened this issue Sep 4, 2021 · 2 comments · May be fixed by #6796
Open

Java: PrimitiveType.getADefaultValue() is misleading and might not work correctly #6615

Marcono1234 opened this issue Sep 4, 2021 · 2 comments · May be fixed by #6796
Labels
acknowledged GitHub staff acknowledges this issue Java question Further information is requested

Comments

@Marcono1234
Copy link
Contributor

There are several issues with PrimitiveType.getADefaultValue():

  • The documentation says:

    Gets a default value for this primitive type, as assigned by the compiler for variables that are declared but not initialized explicitly.

    This is misleading; unless the source actually contains literals with the default value, the predicate will have no result (except for some BooleanLiterals which are found in the JDK code as part of annotations). For example for this Java code:

    class DefaultLiteral {
        byte b;
        short s;
        int i;
        long l;
        float f;
        double d;
        boolean bool;
        char c;
        
        Object o;
    }

    the following query only finds BooleanLiterals:

    import java
    
    from PrimitiveType t, Literal defaultValue
    where
    defaultValue = t.getADefaultValue()
    select t, defaultValue, defaultValue.getLocation()
  • The way it matches literals by using getLiteral() misses some cases where getValue() would actually represent the default value (relates to Java: Replace incorrect usage of Literal.getLiteral() #6612).

Would it make sense to deprecate the predicate (for removal) and instead adjust the only query using it (DeadStoreOfLocal.ql)?
It appears that query actually wants to find out if an assigned literal has a default value; so maybe it should be rewritten to explicitly test that. (Note that adding a predicate Literal.hasDefaultValue() might not make much sense because technically for StringLiterals type String the default value is null; and NullLiteral has no type which could occur as field type.)

@Marcono1234 Marcono1234 added the question Further information is requested label Sep 4, 2021
@smowton
Copy link
Contributor

smowton commented Sep 6, 2021

To avoid surprising existing users we could redocument this to note what it really does (enumerate actual literals in source code that so happen to match the default the compiler would have assigned for an expression of this type), and perhaps add a different predicate that gives a string rather than a Literal and therefore doesn't rely on having a real entity to point at for a source location.

@aschackmull
Copy link
Contributor

Yeah, that predicate is indeed confusing to the point of near-uselessness.

Would it make sense to deprecate the predicate (for removal) and instead adjust the only query using it (DeadStoreOfLocal.ql)?
It appears that query actually wants to find out if an assigned literal has a default value; so maybe it should be rewritten to explicitly test that.

Yes, I'd be in favour of that solution.

@Marcono1234 Marcono1234 linked a pull request Oct 2, 2021 that will close this issue
@hmakholm hmakholm added Java acknowledged GitHub staff acknowledges this issue labels Oct 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
acknowledged GitHub staff acknowledges this issue Java question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants