JS-158 Geneate Java API for ESTree #4725
Conversation
sonar-plugin/api/src/main/java/org/sonar/plugins/javascript/api/estree/ESTree.java
Outdated
Show resolved
Hide resolved
sonar-plugin/api/src/main/java/org/sonar/plugins/javascript/api/estree/ESTree.java
Outdated
Show resolved
Hide resolved
sonar-plugin/api/src/main/java/org/sonar/plugins/javascript/api/estree/ESTree.java
Outdated
Show resolved
Hide resolved
sonar-plugin/api/src/main/java/org/sonar/plugins/javascript/api/estree/ESTree.java
Outdated
Show resolved
Hide resolved
sonar-plugin/api/src/main/java/org/sonar/plugins/javascript/api/estree/ESTree.java
Show resolved
Hide resolved
sonar-plugin/bridge/src/main/java/org/sonar/plugins/javascript/bridge/ESTreeFactory.java
Outdated
Show resolved
Hide resolved
saberduck
requested review from
quentin-jaquier-sonarsource and
ilia-kebets-sonarsource
hashicorp-vault-sonar-prod
bot
assigned quentin-jaquier-sonarsource and ilia-kebets-sonarsource and unassigned quentin-jaquier-sonarsource
sonar-plugin/api/src/main/java/org/sonar/plugins/javascript/api/estree/ESTree.java
Show resolved
Hide resolved
hashicorp-vault-sonar-prod
bot
assigned saberduck and unassigned ilia-kebets-sonarsource
| public record Location(Position start, Position end) {} | ||
|
|
||
| public sealed interface CallExpression extends Node { | ||
| Node callee(); |
There was a problem hiding this comment.
I don't understand why it's a Node. The spec says that callee and arguments are all Expressions.
This would result in a million unnecessary type casts.
There was a problem hiding this comment.
no problem to change, for some reason .d.ts says that it can be also Super https:/DefinitelyTyped/DefinitelyTyped/blob/master/types/estree/index.d.ts#L389
I am not sure which one is wrong, spec or dts
There was a problem hiding this comment.
There was a problem hiding this comment.
Ah, sorry, yeah, estree/index.d.ts is the ultimate source of truth here.
hashicorp-vault-sonar-prod
bot
assigned andrey-tyukin-sonarsource and unassigned saberduck
There was a problem hiding this comment.
Scrolled through it, added a few comments here and there.
Some types of some child nodes seem unnecessarily lax, lots of child nodes are declared just as Node, even though they have a much more specific type in the .d.ts.
Note that Java allows to implement multiple sealed interfaces:
public class Multisealed {
sealed interface A {}
sealed interface B {}
public record Foo() implements A, B {}
}Could we maybe collect all | type unions, and convert them into sealed interfaces like ExpressionOrPropertyDefinition, and then let both Expression and PropertyDefinition implement the ExpressionOrPropertyDefinition? This should preserve the tight bounds on all types.
sonar-plugin/api/src/main/java/org/sonar/plugins/javascript/api/estree/ESTree.java
Outdated
Show resolved
Hide resolved
sonar-plugin/api/src/main/java/org/sonar/plugins/javascript/api/estree/ESTree.java
Outdated
Show resolved
Hide resolved
sonar-plugin/api/src/main/java/org/sonar/plugins/javascript/api/estree/ESTree.java
Outdated
Show resolved
Hide resolved
sonar-plugin/api/src/main/java/org/sonar/plugins/javascript/api/estree/ESTree.java
Outdated
Show resolved
Hide resolved
sonar-plugin/api/src/main/java/org/sonar/plugins/javascript/api/estree/ESTree.java
Outdated
Show resolved
Hide resolved
sonar-plugin/api/src/main/java/org/sonar/plugins/javascript/api/estree/ESTree.java
Outdated
Show resolved
Hide resolved
sonar-plugin/api/src/main/java/org/sonar/plugins/javascript/api/estree/ESTree.java
Outdated
Show resolved
Hide resolved
| @@ -591,4 +591,4 @@ message SourceLocation { | |||
| message Position { | |||
There was a problem hiding this comment.
Having a comment somewhere whether it's all 0-based or 1-based certainly wouldn't hurt.
|
Missing coverage is expected, it will be addressed after JS-159 and JS-160