Detect name conflicts within a reactor in

[cmnrd]

The following code is legal LF

reactor Foo(bar:int=2) {
    input bar:int
    state bar: int
    method bar(): int {==}
    reaction bar(bar)
}

but it causes compile errors in the C++ target and likely also other targets. The problem is, that we don't enforce uniqueness of names in the reactor scope. Currently, there is only a Xtext generated mechanism that detects if triggers have the same name, but it does not handle other reactor elements.

[lhstrh]

I have a fix for this ( #2085), but it is incompatible with test/Rust/src/CtorParamMixed.lf:

target Rust

reactor Print(value: i32 = 42, name: String = {= "xxx".into() =}, other: bool = false) {
  state value = value
  state name = name
  state other = other

  reaction(startup) {=
    assert_eq!(42, self.value);
    assert_eq!("x2hr", self.name.as_str());
    assert_eq!(true, self.other);
    println!("success");
  =}
}

main reactor CtorParamMixed {
  p = new Print(other=true, name = {= "x2hr".into() =})
}

To fix this test, the constructor parameters would have to be renamed... Do we want this?

[cmnrd]

Interesting. I am curious how the rust generator handles this. Is 'other' essentially handled like a mutable parameter?

I think the safe thing to do for now is enforce uniqueness of names, also between state variables and parameters in all targets. If we think we need "mutable parameters", then we can think about how to express them nicely in LF and properly introduce them in all targets. But I would like to hear @oowekyala's opinion also.

[lhstrh]

@oowekyala?

[oowekyala]

Interesting. I am curious how the rust generator handles this. Is 'other' essentially handled like a mutable parameter?

Parameters in the Rust target are treated as constructor parameters, they are not implicitly stored as state variables, so they are not a mutable parameter here. Parameters and state variables are orthogonal in the rust target. Tbh I think this should be the default behavior in all targets. It is more important that that be the case in Rust, because then you can pass references as parameters, which you couldn't do if they were implicitly stored in a field. However I think the distinction is useful in all targets, because that allows one to express that we need a value at construction time, but not later, so we don't need to store it. Defining parameters as 'constant' state variables (as now in most targets) is also not so useful IMHO, as some of our target languages cannot enforce this.

In my view name conflicts between state variables and parameters are not harmful and can even help convey the 'relatedness' between a parameter and a state var. Eg the idiom of writing a state foo = foo where the second foo is a parameter is useful. Alternatively we should introduce the shorthand syntax that declares a state var and a parameter at the same time, which I wanted to do in rust for a while. For instance:

reactor R(state foo: i32) {}
// is the same as
reactor R(foo: i32) {
  state foo: i32 = foo
}

(Ref #1492)

This idiom would not fix the use case where you want to wrap your parameter in some other structure, eg

reactor R(something: i32) {
  state something: Wrapper<i32> = {= wrap(something) =}
}

IMHO unless there is a technical reason to forbid conflicts we shouldn't forbid them. This kind of naming constraint doesn't really improve your code, but can be frustrating to the programmer, especially when they feel arbitrary. Just my 2 cents anyway

[lhstrh]

IMHO unless there is a technical reason to forbid conflicts we shouldn't forbid them. This kind of naming constraint doesn't really improve your code, but can be frustrating to the programmer, especially when they feel arbitrary. Just my 2 cents anyway

I agree. I noticed this when I tried to "fix" the test that started failing and had to rename the parameters...

I think the only way to address this in the C target would be to organize things in the self struct differently, e.g.: self->state->foo and self->parm->foo to distinguish between the state variable and the parameter. @edwardalee: WDYT?

[edwardalee]

I think it's bad enough in the C target that we have to refer to a state variable (and parameter) as self->name. To have to do self->state->name would be even worse.

Some time ago, I tried to change the C target so you could just use name. I failed. But that would make the name conflicts even worse.

[lhstrh]

It sounds like we should make the check target-specific then...

[cmnrd]

What @oowekyala is saying though, is that in the Rust target parameters do not become part of the self struct. They are only available during construction (passed as arguments to the constructor). If they are to be stored on the self struct, then they need to be explicitly assigned to a state variable (which can be constant or mutable). Since parameters are not stored on the self struct, they also don't need to be checked for name conflicts.

This view makes a lot of sense to me, but bringing the other targets in line with this view on parameters would be a lot of work...

[edwardalee]

Does this mean that in Rust you cannot access parameter values in a reaction body? That would be a rather severe limitation.

[oowekyala]

Yes that's exactly it. I don't think it is limiting as the programmer can just save the parameter into a field if they want. It's actually giving more freedom to the lf programmer.

[edwardalee]

I think we have quite a few issues that are more important than this. I think what we should do is that if any target has a problem with name collisions, then we should forbid them in the validator for all targets and call it done. Later, if someone has the inclination and the time, they can reverse this restriction in a backward-compatible way.

[lhstrh]

So what is your take on #1973 (comment) then? Adopt that solution as-is and change the tests?

[lhstrh]

If it's not important, we can also just remove the issue from the milestone...

[edwardalee]

So what is your take on #1973 (comment) then? Adopt that solution as-is and change the tests?

I think it's very odd that you can't access reactor parameters in reaction bodies in the Rust target. Why are we calling them reactor parameters then? I would change the test and rename the parameter.

[lhstrh]

So what is your take on #1973 (comment) then? Adopt that solution as-is and change the tests?

I think it's very odd that you can't access reactor parameters in reaction bodies in the Rust target. Why are we calling them reactor parameters then? I would change the test and rename the parameter.

I don't think that's the case. @oowekyala wrote:

Parameters in the Rust target are treated as constructor parameters, they are not implicitly stored as state variables, so they are not a mutable parameter here.

In Java, a constructor argument also isn't treated as a class variable. I think that would be odd.

[edwardalee]

Hmm... Is a reactor declaration a constructor declaration?

[lhstrh]

If it's not, what is it? In all targets with languages that have OOP features, parameters are mapped to constructor arguments. What other reasonable way is there?