Safety data-flow #2143
Safety data-flow #2143
Conversation
Borrowed heavily from error-prone null checks
Generate changelog in
|
| if (target instanceof FieldAccessNode) { | ||
| FieldAccessNode fieldAccess = (FieldAccessNode) target; | ||
| updates.set(fieldAccess, safety); | ||
| } |
There was a problem hiding this comment.
Is this a complete list or should we handle the case when target is not an instance of these three types?
There was a problem hiding this comment.
I guess with the current impl, we just update the store with an empty update and just stop propagating safety information which sounds fine!
There was a problem hiding this comment.
Yep, it's an exhaustive list:
https://github.com/typetools/checker-framework/blob/090d02424e6d825d45406b959010dd0ac81fbac2/dataflow/src/main/java/org/checkerframework/dataflow/cfg/node/AssignmentNode.java#L37-L39
We might as well fail in the case of an unknown target, that way we're forced to update the code to handle new types if they're added.
| private Safety fieldInitializerSafetyIfAvailable(VarSymbol accessed) { | ||
| if (!traversed.add(accessed)) { | ||
| // Initializer circularity | ||
| return Safety.UNKNOWN; |
There was a problem hiding this comment.
Can you elaborate a bit on this traversed guard? Is it to avoid loops when running the flow on the field initializer?
| } | ||
|
|
||
| @Override | ||
| public TransferResult<Safety, AccessPathStore<Safety>> visitNarrowingConversion( |
There was a problem hiding this comment.
nit: visitWideningConversion and visitNarrowingConversion uses the same code. Maybe we can extract or delegate?
There was a problem hiding this comment.
👍 I'll include visitTypeCast as well
| } | ||
|
|
||
| @SuppressWarnings("checkstyle:CyclomaticComplexity") | ||
| private Safety fieldSafety( |
There was a problem hiding this comment.
nit: These two methods are a bit out-of-place. Maybe we move them further down below the method that uses them?
| } | ||
|
|
||
| @CheckReturnValue | ||
| private static TransferResult<Safety, AccessPathStore<Safety>> updateRegularStore( |
There was a problem hiding this comment.
This took me a while to grok so think a comment on when we need to update the store and when we don't would be helpful!
|
👍 |
|
Released 4.84.0 |
==COMMIT_MSG==
Implement Safety flow checks
==COMMIT_MSG==
This is likely fairly slow, and probably needs optimization.
Needs more test coverage.