[jssrc2cpg] Fix TS decorator handling #5701
Conversation
Not actually a fix but as we cannot handle annotation parameter w.r.t. data-flow this at least stops crashing the backend.
maltek
left a comment
maltek
left a comment
There was a problem hiding this comment.
Seems that the minimal example I provided you was a bit too minimal: the original customer code used some complex expressions inside the NgModule() annotation, which contained a lambda somewhere deep down. This check won't catch that.
I think the root cause is putting arbitrary expressions below Annotation nodes to begin with. These nodes were only meant for pure compile-time constructs like in Java, and should not contain arbitrary expressions with side-effects. E.g. pythonsrc lowers decorators to method calls and doesn't use annotation nodes at all.
That's a larger change where we have to tread carefully with compatibility of backend passes, but it may be unavoidable.
|
I've been looking at how decorators are transpiled... and it's complicated :( For years, typescript has supported decorators via the From what I can tell, the experimental legacy variant is still more widely used. It gets transpiled like this: |
|
That's right. It makes no sense to replicate the exact transpilation semantics. |
|
Right, we don't need to slavishly follow the transpilers. But we should still follow the same semantics. I.e. decorators are evaluated in the surrounding scope of what's being decorated. But the PR here instead pushes the decorator evaluation inside of the decorated element. And we do have some existing backend passes looking for the transpiled patterns, so there is some duplicated effort to be saved there if and where it's easy to lower things similarly in the frontend. |
|
I still have no idea where to put the expressions though. The initial problem was dangling method refs but it could be anything I guess. |
|
In the same place as the transpilers are emitting their |
max-leuthaeuser
changed the title
maltek
left a comment
maltek
left a comment
There was a problem hiding this comment.
the new lowering for the class decorators looks good to me
| annotationLambdaRef.methodFullName shouldBe "Test0.ts::program:<lambda>0" | ||
| annotationLambdaRef._containsIn.collectAll[Method].fullName.l shouldBe List("Test0.ts::program") | ||
|
|
||
| val List(decoratorAssignment) = cpg.call.codeExact("MyClass = __decorate([NgModule(() => { })], MyClass)").l |
There was a problem hiding this comment.
this is totally fine, and you don't have to change this at all. I just want to share since it's relatively new and I like it a lot for tests like this: our DSL has a .loneElement that returns the single element in an iterator, and throws an error if the iterator contains more or less values than one.
maltek
left a comment
maltek
left a comment
There was a problem hiding this comment.
The changes for decorators of classes in the PR look good. But to me it looks incomplete for the other decorators: it's still pulling in the decorator call into decorated methods instead of into the outer scope.
Yeah, thats why the title of the PR is "Fix TS class annotations" |
Well, it changes the representation of all decorators, not just class decorators. And that of non-class decorators is changed to a weird and unfinished representation that I don't want to have to support in the backend. If a follow-up will be coming up right after this is merged, then OK. But this isn't releasable like this as far as I'm concerned. |
|
Ok, I will add it directly to this PR then. |
maltek
left a comment
maltek
left a comment
There was a problem hiding this comment.
about the __metadata calls adding type reflection metadata - they could be useful. Having the type in the CPG as an expression that's valid in the context does enable us to do more type resolution than a simple string name like e.g. in the possibleTypes could. It's not pretty though, and if we want to rely on them we should emit them independently from decorators.
The current trigger conditions are not quite the same as for typescript, e.g. I tried with this example where tsc generates parameter types but this PR doesn't.
I think I can live with any option:
- removing the reflection metadata calls
- leaving them as-is
- leaving them, but aligning the conditions when they are emitted closer to what
tsc does - adding them for all classes
...i/frontends/jssrc2cpg/src/main/scala/io/joern/jssrc2cpg/astcreation/AstForTypesCreator.scala
Outdated
Show resolved
Hide resolved
...i/frontends/jssrc2cpg/src/main/scala/io/joern/jssrc2cpg/astcreation/AstForTypesCreator.scala
Outdated
Show resolved
Hide resolved
...i/frontends/jssrc2cpg/src/main/scala/io/joern/jssrc2cpg/astcreation/AstForTypesCreator.scala
Outdated
Show resolved
Hide resolved
| )) | ||
| } | ||
|
|
||
| private def staticClassInitMethodAst( |
There was a problem hiding this comment.
This still has some problems.
E.g. for static x = 1 the x is an identifier instead of a field access <constructor>.x = 1;. There's also an argument to be made about pulling this out into the defining scope (since semantically, this is what happens).
I'd leave these changes for another PR though.
There was a problem hiding this comment.
Yeah, a rework for the static init methods should be done separately.
...jssrc2cpg/src/test/scala/io/joern/jssrc2cpg/passes/ast/TsDecoratorAstCreationPassTests.scala
Outdated
Show resolved
Hide resolved
max-leuthaeuser
changed the title
This PR puts expressions from TS decorators into synthetic
__decoratecalls.