Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for generics in TypeScript target #1595

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

Support for generics in TypeScript target #1595

wants to merge 6 commits into from

Conversation

lhstrh
Copy link
Member

@lhstrh lhstrh commented Feb 17, 2023

The test in test/TypeScript/src/target/failing/GenericDelay.lf should work but does not. There are two issues:

  • a generic reactor in reactor-ts needs a type constraint extends Present which is a result of the way we currently deal with present/absent
  • when referring to the port of a generic reactor, the concrete type must be used, not the abstract one (e.g., function (this, __d_inp: ReadWrite<T>) { should be function (this, __d_inp: ReadWrite<number>) { in the generated code

@lhstrh
Copy link
Member Author

lhstrh commented Jun 17, 2023

Present is defined as the union of all types, except undefined. As such, ? extends Present precludes the use of undefined, which in TS is both a type and a value. I think that removing this constraint may introduce the capability of "unsetting" a port that after it has already been set. This, however, is only a concern for ports typed Present, which one would use for a port that relays pure events. This leads to the question: what should be the type of a port that relays pure events? Obvious choices would be unknown or any. With the latter, the "unsetting" behavior would be allowed; with the former, it would not be allowed, but it would also not be legal to set a port using null, which would be a sensible choice for a pure event. The best next thing would probably would be to set it using an empty string or something like that, which unknown would allow for.

@axmmisaka
Copy link
Collaborator

I think this deserves a second look after merging lf-lang/reactor-ts#168 and #1857.

The first issue appears to be here and can be addressed rather quickly:

if (!reactor.typeParms.isEmpty()) {
reactorName +=
reactor.typeParms.joinToString(", ", "<", ">") { it.toText() }
}

The source of the second issue appears to be here:

private fun generateReactionSignatureElementForPortEffect(effect: VarRef, isMutation: Boolean): String {
val outputPort = effect.variable as Port
val portClassType = if (outputPort.isMultiport) {
(if (isMutation) "__WritableMultiPort" else "MultiReadWrite") + "<${(effect.variable as Port).tsPortType}>"
} else {
(if (isMutation) "__WritablePort" else "ReadWrite") + "<${(effect.variable as Port).tsPortType}>"
}
return if (effect.container != null && effect.container.isBank) {
"Array<$portClassType>"
} else {
portClassType
}
}

Defined here:

val Port.tsPortType: String
get() = type?.let { TSTypes.getInstance().getTargetType(it) } ?: "Present"

Types were inferred here:

public String getTargetType(StateVar s) {
var type = TargetTypes.super.getTargetType(s);
if (!ASTUtils.isInitialized(s)) {
return "%s | undefined".formatted(type);
} else {
return type;
}
}

Which ultimately leads me here:
https://github.com/lf-lang/lingua-franca/blob/master/core/src/main/java/org/lflang/generator/TargetTypes.java

I might need to read the code more times to better understand what's happening; yet, if other target languages' generic type support doesn't suffer this problem, then it would appear a bit weird to me, as it appears to me that the issue is that the type for the port, d in this example, is not inferred from the previous declaration correctly.

@lhstrh
Copy link
Member Author

lhstrh commented Jul 6, 2023

Which ultimately leads me here: https://github.com/lf-lang/lingua-franca/blob/master/core/src/main/java/org/lflang/generator/TargetTypes.java

I might need to read the code more times to better understand what's happening; yet, if other target languages' generic type support doesn't suffer this problem, then it would appear a bit weird to me, as it appears to me that the issue is that the type for the port, d in this example, is not inferred from the previous declaration correctly.

You're right; this is where the problem lies. I'm also unsure how to fix it. I've asked @oowekyala if he has any hints.

Still todo for other things like actions
@oowekyala
Copy link
Collaborator

So apparently the generated code looks like

// there is a field
d: Delay<number> 

// and in the ctor    
        this.addReaction(
            [this.startup],
            [this.writable(this.d.inp)],
            function (this, __d_inp: ReadWrite<T>) {
                // =============== START react prologue
                const util = this.util;
                let d = {inp: __d_inp.get(),}
                // =============== END react prologue
                try {
                    d.inp = 42;
                } finally {
                    // =============== START react epilogue
                    if (d.inp !== undefined) {
                        __d_inp.set(d.inp)
                    }
                    // =============== END react epilogue
                }
            }
        );

It is possible to replace the type T with number at codegen time, by mapping type parameters of the reactor to their argument. This is what the Rust generator does (and how I fixed this for TS). But it's ugly, as in general this means we have to parse target code blocks. For instance a type parameter might be declared {= T: Eq =} to bound it. Similarly, the type of the port might be something like {= Vec<T> =}.

Ideally we would generate the code in a form that avoids specifying this type explicitly. For instance if we could avoid putting the ports in the parameters of the reaction lambda, we could use TS's type inference to resolve the type of d.inp:

        this.addReaction(
            [this.startup],
            [this.writable(this.d.inp)],
            function (this, __d: Delay<number>) { // we don't need to do string replacement here, this is the declared type of d
                // =============== START react prologue
                const util = this.util;
                let d = {inp: __d.inp.get(),} // the type of d is inferred to `{inp: number}` (I suppose)
                // =============== END react prologue
                try {
                    d.inp = 42;
                } finally {
                    // =============== START react epilogue
                    if (d.inp !== undefined) {
                        __d.inp.set(d.inp)
                    }
                    // =============== END react epilogue
                }
            }
        );

But I don't know enough about the TS target to know if that's possible easily, or if that has other problems. For instance I don't know what this is in the body of the lambda.

@axmmisaka
Copy link
Collaborator

axmmisaka commented Jul 11, 2023

Ideally we would generate the code in a form that avoids specifying this type explicitly

function (this, __d_inp: ReadWrite<number>) {
This is okay. It has to be ReadWrite iiuic, but based on my experiment with this, __d.inp.get() will infer correct type.

For instance I don't know what this is in the body of the lambda.

I think it's a rather cursed design; in JS you could rebind this; here, instead of passing in a mutationsandbox, we define explicit this and rebind it when calling it:

https://github.com/lf-lang/reactor-ts/blob/c1b12b4945a704563d59dd293f66f596d3bf8836/src/core/reaction.ts#L151-L156

I don't know the rationale of this design, but changing it require some more efforts......

(Why doesn't gh show the code?!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants