Repository navigation
SONARJAVA-7283 Decouple the frontend AST from Eclipse JDT core - #6449
aurelien-coet-sonarsource wants to merge 5 commits into
Conversation
hashicorp-vault-sonar-prod
Bot
changed the title
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
dc20e3c to
8c95c70
Compare
0f76276 to
7e18c5f
Compare
|
/pvf |
|
@aurelien-coet-sonarsource ❌ Your
|
7e18c5f to
e14396f
Compare
|
/pvf |
|
📊 Performance Validation complete Projects with unexpected PVF results:
|
e14396f to
87e3f83
Compare
rombirli
left a comment
rombirli
left a comment
There was a problem hiding this comment.
Need to continue this review later, just adding early bugbot findings yet
| for (JProblem.Type type : JProblem.Type.values()) { | ||
| if (problem.getID() == IDS.get(type)) { | ||
| LineColumnConverter.Pos start = positions.toPos(problem.getSourceStart()); | ||
| LineColumnConverter.Pos end = positions.toPos(problem.getSourceStart()); |
There was a problem hiding this comment.
End position incorrectly uses getSourceStart(), producing zero-width warning ranges.
87e3f83 to
e0b30c5
Compare
e0b30c5 to
2d2821f
Compare
Code Review ✅ Approved 2 closed / 2 findings🟡 Medium risk · Refactoring Java semantic bindings into lazy suppliers changes type and symbol resolution across the frontend, so consumers could receive incorrect analysis results. Decouples the frontend from Eclipse JDT core by replacing shared-tree compiler bindings with lazy semantic suppliers, adding session-based parser lifecycle and a separate diagnostics provider. Resolved: JSP tag-file compilation no longer fails on JRE-only scanners. Open minor suggestion: undefined-type reporting ( ✅ 2 closed✅ Bug: JSP tag-file compilation now needs a JDK; fails on JRE-only scanners
Closed: Cross-PR: Undefined-type reporting lost or broken when combined with #6437
Verification Rules 1/1 met PRs 2 inspectedFunctional validation No results Rules 1 passed 2 not applicable
Broader impact PRs 2 inspectedCross-PR 2 PRs inspected 0 open impacts
Auto-approval Not enabledFor more verification details, comment OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
|
aurelien-coet-sonarsource
changed the title
rombirli
left a comment
rombirli
left a comment
There was a problem hiding this comment.
I didn't review yet from JParser to the end, will do it later
| private ListTree<TypeTree> permittedTypes; | ||
| @Nullable | ||
| public ITypeBinding typeBinding; | ||
| public Supplier<Symbol.TypeSymbol> typeSymbol = SemanticValues.UNKNOWN_TYPE_SYMBOL; |
There was a problem hiding this comment.
this public non-final supplier field looks like a code smell to me, a property with getter/setter would be clearer
There was a problem hiding this comment.
I agree. I don't know why these fields were public in the first place, but let's move them behind getter/setters.
| import javax.annotation.Nullable; | ||
| import org.eclipse.jdt.core.dom.IBinding; | ||
|
|
||
| final class EcjSemanticAttachment<B extends IBinding, T> implements Supplier<T> { |
There was a problem hiding this comment.
If i'm correct we can constrain T to Symbol and its subclasses only
| final class EcjSemanticAttachment<B extends IBinding, T> implements Supplier<T> { | |
| final class EcjSemanticAttachment<B extends IBinding, T extends Symbol> implements Supplier<T> { |
| private SyntaxToken endToken; | ||
|
|
||
| public IVariableBinding variableBinding; | ||
| public Supplier<Symbol> variableSymbol = SemanticValues.UNKNOWN_SYMBOL; |
There was a problem hiding this comment.
again public mutable fields seem not very conventional to me
| private List<AnnotationTree> annotations; | ||
|
|
||
| public IBinding binding; | ||
| public Supplier<Symbol> symbolSupplier = SemanticValues.UNKNOWN_SYMBOL; |
|
|
||
| @Nullable | ||
| public IMethodBinding methodBinding; | ||
| public Supplier<Symbol.MethodSymbol> methodSymbol = SemanticValues.UNKNOWN_METHOD; |
There was a problem hiding this comment.
As i see it everywhere, could JavaTree become a generic class JavaTree, and contain a symbol property (getter+setter)?
There was a problem hiding this comment.
I think the issue is that not all classes that inherit from JavaTree have an associated symbol, so this would add a field and getter/setter with some default value to a lot of classes where it doesn't actually make sense.
| import org.sonar.java.model.expression.MethodInvocationTreeImpl; | ||
| import org.sonar.java.model.pattern.AbstractPatternTree; | ||
|
|
||
| final class EcjTreeBindings { |
| public JSema sema; | ||
| public Sema sema; | ||
|
|
||
| final Set<JProblem> undefinedTypes = new HashSet<>(); |
There was a problem hiding this comment.
is default visibility expected here?
Should it be public instead, packages might move
| public Set<JProblem> undefinedTypes() { | ||
| return Collections.unmodifiableSet(undefinedTypes); | ||
| } | ||
|
|
There was a problem hiding this comment.
I would go for either
- property style :
- getter return immutable view on the set as there
- public functions to edit the set (add, empty, addall...)
- public field style
- just a public final field
But this undefinedTypes currently mixes public immutable getter, with default-visible final field
| private Object convertAnnotationValue(Object value) { | ||
| return EcjSymbolMetadata.convertAnnotationValue(sema, value); | ||
| } | ||
| } | ||
|
|
||
| static Object convertAnnotationValue(JSema sema, Object value) { | ||
| return switch (value) { | ||
| case IVariableBinding iVariableBinding -> sema.variableSymbol(iVariableBinding); | ||
| case ITypeBinding iTypeBinding -> sema.typeSymbol(iTypeBinding); | ||
| case IAnnotationBinding iAnnotationBinding -> sema.annotation(iAnnotationBinding); | ||
| case Object[] a -> { | ||
| Object[] result = new Object[a.length]; | ||
| for (int i = 0; i < a.length; i++) { | ||
| result[i] = convertAnnotationValue(sema, a[i]); | ||
| } | ||
| yield result; | ||
| } | ||
| case null, default -> value; | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
These 2 return types are too broad to me, the return type could be symbol to be bounded and easier to understand
| @Override | ||
| @Nullable | ||
| public Object defaultValue() { | ||
| return EcjSymbolMetadata.convertAnnotationValue(sema, methodBinding().getDefaultValue()); | ||
| } |
There was a problem hiding this comment.
the return type could also be bounded to Symbol
Summary by Gitar
FrontendSession,FrontendParser, andFrontendResourcesso ECJ environment cleanup is owned deterministically per sessionJParserConfigbatch and file-by-file parsing with session-based parsing and cleanupEcjTreeBindingsandEcjSemanticAttachmentso trees hold lazySuppliervalues instead of rawIBindingfieldsEcjSymbolMetadataandJSymbolMetadatasuppliersEcjDiagnosticProviderand moved warning mapping and compiler option handling out ofJProblemandJParserConfigJasperCompilerto compile tag helpers with the running JDK compilerTestResourceScopeand frontend lifecycle, semantic fixture, and identity testsThis will update automatically on new commits.