Codemod design improvement#28
Conversation
| if (imports.contains(newImport)) { | ||
| return; | ||
| } | ||
| for (ImportDeclaration existingImport : imports) { |
There was a problem hiding this comment.
No need for this since we have ASTTransforms::addImportIfMissing.
| logger.error("Unexpected node encountered in {}:{}", context.path(), region); | ||
| return; | ||
| } | ||
| if (JavaParserUtils.regionMatchesNodeStart(node, region)) { |
There was a problem hiding this comment.
This may match undesired nodes since it only checks for overlapping. For example in n1().n2().n3() the range of n1() overlaps n1().n2().n3(). This may be a particular problem if n1(), n2() and n3() are calls to the same method. I'd rather do exact matching.
Also you need to check if semgrep's region in the SARIF matches JavaParser ranges exactly. I know CodeQL does not (it includes an extra column).
| List<Result> results = sarif.getResultsByPath(context.path()); | ||
| List<? extends Node> allNodes = cu.findAll(nodeType); | ||
|
|
||
| for (Result result : results) { |
There was a problem hiding this comment.
It's fine this way but a small suggestion using streams:
List<Region> regions = results.stream().map(r -> r.getLocations().get(0).getPhysicalLocation().getRegion()).collect(Collectors.toList())
var allMatchedNodes = allNodes.stream().filter(n -> regions.stream().anyMatch(r -> JavaParserUtils.regionMatchesNode(n, r))); Then iterate/filter/collect the resulting Stream allMatchedNodes.
| } | ||
|
|
||
| /** | ||
| * Creates a visitor for the given context and locations. |
There was a problem hiding this comment.
Update the javadoc here, It does not create a visitor anymore.
| */ | ||
| public abstract ModifierVisitor<FileWeavingContext> createVisitor( | ||
| CodemodInvocationContext context, List<Region> regions); | ||
| public abstract void onSemgrepResultFound( |
There was a problem hiding this comment.
Small suggestion: make it return CompilationUnit. This makes it clear that the main goal is change the AST.
* Fully offline resolution * cleanup * [no ci] Autogenerated JaCoCo coverage badge --------- Co-authored-by: Jacoco Coverage Update Action <pixee@users.noreply.github.com>
2 participants
Although we didn't end up choosing to do the "AST pointer collection" strategy, we still liked some of the API design we built as part of brainstorming, so we decided to implement it.