[Java] Rule S3077 should (I think) be reworded

In sonarlint, the summary text for the rule violation is “remove the ‘volatile’ keyword from this field”. In a context where this is appearing as in-IDE guidance, I think that wording is actively harmful. It provides seemingly clear instructions for a fix that will, at best, leave the code no better off but may make things worse. I think something like ‘volatile is insufficient for mutable non-primitive fields’ or ‘volatile should be used with caution on Objects’ or really anything that alludes to something more going on would be better.

Hi @cmbalin,

Thank you for your feedback. I agree that removing volatile is only part of the fix.

I’ll replace the message with “replace this ‘volatile’ keyword on an object with a more thread-safe mechanism”. Let me know if you think that it is not clear enough.

Ticket here: https://jira.sonarsource.com/browse/SONARJAVA-3243

Cheers,

1 Like

I was recently asked to evaluate this rule by a colleague who had it flagged on some of his code. The rule appears to be overly cautious and basically suggests that using volatile for references is never safe. That is simply not true, but even the updated error text makes it appear so. It would be better to suggest that the safety of the use is dependent on the implementation of the referenced object, i.e. the referenced object itself should be either immutable of thread-safe.

The current suggestion states “For mutable objects, the volatile should be removed, and some other method should be used to ensure thread-safety, such as synchronization, or ThreadLocal storage.” The volatile isn’t the problem here, the problem is the potential lack of thread safety on the target object. So I’d suggest a corrected suggestion of “For mutable objects, the volatile can be retained, but the referenced object itself must be internally thread-safe.”

I’d also say that the severity should be lowered as well since sonar isn’t capable of identifying safe vs unsafe usages, and blindly assuming that all volatile refs are bugs is overly cautious and will erroneously flag all correct code as having safety issues.

1 Like