Inspection to detect fields accessed without being queried in SOQL

[Eyal Pinkas]

1. Inspect field acces where query is in same scope

An inspection that checks every read-access of sObject, to see if the field was retrieved in SOQL query - that would be nice:

Contact[] customers = [ SELECT Id, Name];
//following line will produce Exception in runtime, since FirstName was not retrieved in query:
String firstName = customers[0].FirstName;

in above snippet, I would expect FirstName to be underlined with red line, with pop saying something like 'FirstName was not queried in SOQL query - this will produce Exception in runtime'

2. Inspect parameters of method calls for field-access

Sometimes a method accepts a sObject as parameter, which is queried before the method was called. In that case above inspection would not be usefull. For this case (which is common and confusing, IMHO), I would like an inspection that gathers all required fields for method parameters, and check every call of this method as inspection 1 does. That would be very valuable!

3. Generate info about required fields of method

Using meta-data from inspection 2, one could even generate java-doc for method saying which fields are mandatory for method, or have this data available to developer some other way

Example for suggestions 2 and 3:

/**
 * Auto-Generated field requirements:
 *   c1:
 *     Id, Name, Email 
 *   c2:
 *     Name
 */
private String getSomeString( Contact c1, Contact c2 ){
  return c1.Id + '_' + c1.Name + '_' + c1.Email + '___' + c2.Name; 
}

public static foo(){
  Contact contacts[] = [ SELECT Id, Name FROM Contact ];
  String myString = getSomeString( contacts[0], contacts[1] );
  //inspection 2 should mark above line, because contacts[0] were not queried with Email field
}

[Scott Wells]

Thanks for logging, and thanks especially for all of the thoughtful detail! This is something I've considered for a while, and as you've pointed out, it's not something that can be 100% reliable due to the fact that queried data often comes from indirect sources (think about Visualforce controllers and the underlying record which is populated based on things used in the presentation tier) and/or dynamic SOQL queries.

I think that the first option is very doable and would highlight the problem a reasonable portion of the time.

The second option is doable but isn't likely going to perform well on-the-fly because for a given method that receives SObject params, you'd have to check ALL callers transitively to find the various root sources of those params. It's basically a caller graph traversal (which IC2 does now via the call hierarchy, and it can be quite expensive).

As for the third option, I don't want to abuse ApexDoc. If Salesforce adds a more generalized facility for annotations, that would work well. For what it's worth, I would LOVE to have @NotNull and @Null (or @Nullable) annotations in Apex. I've become VERY reliant on them in Java now to know when I do and don't need to check for null values, and IntelliJ IDEA has supporting code inspections to "detect nullity" based on these annotations and tell you when you're vulnerable to an NPE and/or performing superfluous null checks. As it is now, I always feel like I have to consult the implementation or be overly-defensive against nulls.

Good thoughts! I'll definitely put it on the queue for consideration.

[Eyal Pinkas]

Hey Scott Thanks for the comment! I appreciate it.

Sorry for the long response:

IMHO:

1. Iterative improvement is the way to go in this case - maybe you guys implement suggestion 1, and later consider suggestion 2 and/or 3. Once 1 is implemented, 2 and 3 are much easier.

2. About performance of #2 - Maybe that can be done gradually, in steps. My key point is that I don't have to have the result immediatly - getting warnings a few minutes late is better than not getting them at all. My idea is something like this -

   1. You store meta-data for each method-call, containing required fields, and some kind of flag (timestamp probably) to signal if this needs re-evaluation.
   2. Scanning all method-calls can be done in background periodically, and preferably - when recources are available.
   • So let's say I have 5 transitive method calls that leads to required fields, and evaluation happens once a minute (fixed) - after 5 minutes I will have a notification that a field is missing from SOQL query - still better than nohing.

   • My guess is that at least for my use-cases it would be much faster:

     1. Most of my use-cases have only 1-2 method-calls between query and field-access.
     2. I believe evealuation cycle wouldn't be that expensive and happen quicker than once a minute, especially after you optimize this process (e.g. timestamps that tell you if evaluation is needed; maybe joining this analysis with some other process that happens anyway, etc.).

   • I am aware that this way false-positives might be detected and stay for a long time (actual method does not require field anymore, but this would take 5 minutes to propagate to 5th caller up the tree).

   • So as a developer, I would like to be able to tell IC "re-evaluate this now", when I think the warning is wrong, and want to clear the warning.

3. Maybe ApexDoc is indeed not the right way, I was actually thinking something more on-the-fly - you hover over the method and IDE tells you field dependencies, or you press CTRL+Q and the method documentation that apears somehow includes this information. Generating it in method documentation (as I suggested...) might be annoying actually - you change the method implementation, and have to remember to re-generate documentation.

Yes - @Nullable and @NotNull would be great! Love them.

[Scott Wells]

Regarding the second item, IntelliJ IDEA/WebStorm code inspections work in one of two modes (they can support both), either on-the-fly or batch.

On-the-fly inspections must be VERY fast and have little overhead because they're run (lazily) pretty much any time that code changes. I've had to do some significant caching and optimizations to keep the unused code inspection from causing performance issues. In fact, the unused code inspection doesn't apply all logic during on-the-fly processing that it does during batch inspection. Neither does the standard Java unused code inspection. It checks first to see whether it's being run on-the-fly or not, and if so, it then checks to see whether a search for the current identifier would be prohibitively expensive during on-the-fly processing or not. If so, it's omitted until the user explicitly runs the inspection in batch mode.

What we're talking about here would be like a recursive/transitive unused code inspection and would be very computationally expensive. Unfortunately there's no way to defer those inspections so that they run later. In fact, the IDE will terminate a long-running process automatically in these cases.

As a result, this would need to be a batch inspection that you run explicitly/manually. I think that's probably fine. I encourage people to run a full batch static code analysis before committing/merging code to a releasable branch.

Anyway, just providing some context on how the plugin SDK in general (and code inspections in particular) work so that you understand what options are available.

[Eyal Pinkas]

Thanks Scott I've got a lot to learn about IDE plugin development.

Having a feature like this implemented as batch inspection would be great too.

[Scott Wells]

Issue #1301 was marked as a duplicate of this issue.

[Scott Wells]

Removing version: Unassigned (automated comment)

[Scott Wells]

Duplicate of #695.