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...