Skip to content
New issue

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

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

Already on GitHub? # to your account

More natural syntax for reaction declarations #1853

Merged
merged 19 commits into from
Jun 20, 2023
Merged

More natural syntax for reaction declarations #1853

merged 19 commits into from
Jun 20, 2023

Conversation

lhstrh
Copy link
Member

@lhstrh lhstrh commented Jun 17, 2023

This PR changes the syntax for reaction declarations (which no inline implementation) from reaction(t) s -> e named foo to reaction foo(t) s -> e. Moreover, these changes allow reactions definitions (which do have an inline implementation) to also specify a name. This can be useful for documentation purposes and for making the generated code more readable. Also see #1856.

Note the semicolon at the end, which is required to avoid ambiguity with connection statements. We can make the semicolon optional if we pay attention to newlines, which we currently simply filter out. There is no mandatory semicolon for concrete reactions (ones that feature a body).

A trailing semicolon is only required in situations where not having one leads to ambiguity. A validator check makes sure that we handle this situation.

  • Implement validator check
  • Make error message more specific
  • Ensure that formatter does not introduce newline between triggers and sources (@petervdonovan)
  • Add a validator check to avoid NPE in targets other than C
  • Add unit tests

Note: this PR also fixes a problem where duplicate sources were detected inaccurately (62113a4)

@lhstrh lhstrh marked this pull request as draft June 17, 2023 05:58
@lhstrh lhstrh changed the title Required semicolon for bodiless reactions Required semicolon for abstract reactions Jun 17, 2023
Copy link
Collaborator

@cmnrd cmnrd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also allows reactions with bodies to be named, right? I think generally allowing named reactions would be a good thing, but we should adjust generators and the Diagramm synthesis to handle the names similiarly to the current labels.

@lhstrh
Copy link
Member Author

lhstrh commented Jun 17, 2023

This also allows reactions with bodies to be named, right? I think generally allowing named reactions would be a gold thing, we should only adjust generators and the Diagramm synthesis to handle the names similiarly to the current labels.

That is true. Prohibiting it would require an additional validator check, but I don't really see a reason for that.

@lhstrh
Copy link
Member Author

lhstrh commented Jun 17, 2023

This also allows reactions with bodies to be named, right? I think generally allowing named reactions would be a good thing, we should only adjust generators and the Diagramm synthesis to handle the names similiarly to the current labels.

That is true. Prohibiting it would require an additional validator check, but I don't really see a reason for that.

@edwardalee and @a-sr, what do you think of this?

@edwardalee
Copy link
Collaborator

I find the occasional need for semicolons to be an ugly defect in the grammar. I would prefer to require semicolons everywhere over requiring them sometimes.

@lhstrh
Copy link
Member Author

lhstrh commented Jun 17, 2023

I find the occasional need for semicolons to be an ugly defect in the grammar. I would prefer to require semicolons everywhere over requiring them sometimes.

I agree, but I think that most languages that have optional semicolons have some corner cases where a semicolon is required. JavaScript is one of them. The reason why I find the proposed solution better than the one that's currently in master is twofold:

  • consistency with the way we declare other named syntactic elements in the language; and
  • the prospect of removing the remaining oddity of the required semicolon by using a line break instead of a semicolon to disambiguate.

Note that the semicolon is only required when you do not inline the reaction body, so this will not require any changes to any LF code you've written, as opposed to requiring semicolons, which will affect every piece of LF code out there. As you can see, this PR is very small.

What I was hoping you'd comment on, was not so much the semicolons, but the ability to name any reaction (also those that have inlined implementations) and use the name in the generated code and diagrams. I think this would be helpful and worth implementing, but if for some reason we want to stick with numbers and forbid the naming of reactions unless they are abstract, then a simple validator check can enforce this. WDYT? I can of course implement the check and leave the dealings with names for later... The idea would be that this would subsumes @a-sr's @name attribute.

@lhstrh lhstrh changed the title Required semicolon for abstract reactions More natural syntax for abstract reactions Jun 17, 2023
@edwardalee
Copy link
Collaborator

I agree that the ability to name reactions can help with documentation and code clarity. I suspect we will get people who will want to manually invoke a reaction, but I guess we can point them to methods.

@erlingrj
Copy link
Collaborator

I think this looks alot better than reaction(a) named foo. But also agree that it would be optimal if we could get rid of the semi-colon and use the newline as the delimiter. Parsing and grammars is however not my area of expertise so cant contribute with any ideas on how to do it.

Copy link
Member Author

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can now remove the semicolons in all the tests (because none of them pose ambiguity).

test/C/src/no_inlining/Count.lf Outdated Show resolved Hide resolved
test/C/src/no_inlining/CountHierarchy.lf Outdated Show resolved Hide resolved
test/C/src/no_inlining/CountHierarchy.lf Outdated Show resolved Hide resolved
test/C/src/no_inlining/IntPrint.lf Outdated Show resolved Hide resolved
test/C/src/no_inlining/IntPrint.lf Outdated Show resolved Hide resolved
test/C/src/no_inlining/MultiportToReactionNoInlining.lf Outdated Show resolved Hide resolved
@lhstrh lhstrh marked this pull request as ready for review June 19, 2023 01:11
@lhstrh lhstrh requested review from edwardalee and cmnrd June 19, 2023 03:24
@cmnrd
Copy link
Collaborator

cmnrd commented Jun 19, 2023

Great! I have a concern with terminology though. "Abstract" is commonly used for fully virtual methods (i.e. methods that are virtual and don't have a default implementation). Our reactions, however, are not virtual. So the term "abstract" is quite misleading in my opinion.

We could use the C/C++ terminology and call a bodiless reaction "reaction declaration", or we simply use bodiless reaction.

@lhstrh

This comment was marked as resolved.

@cmnrd

This comment was marked as resolved.

@lhstrh lhstrh changed the title More natural syntax for abstract reactions More natural syntax for reaction declarations Jun 19, 2023
@lhstrh
Copy link
Member Author

lhstrh commented Jun 19, 2023

Great! I have a concern with terminology though. "Abstract" is commonly used for fully virtual methods (i.e. methods that are virtual and don't have a default implementation). Our reactions, however, are not virtual. So the term "abstract" is quite misleading in my opinion.

We could use the C/C++ terminology and call a bodiless reaction "reaction declaration", or we simply use bodiless reaction.

OK, I renamed "abstract reactions" to "reaction declarations".

@lhstrh lhstrh enabled auto-merge June 19, 2023 07:14
Copy link
Collaborator

@cmnrd cmnrd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a validator check is a great idea. This means we will only need to use a semicolon for reaction declarations (without a body) that have sources, right?

There should also be a validator check that ensures that the target code generator has support for bodyless reactions if they are used. Currently, the C++ generator fails with a nullpointer exception and I expect that also other target generators will fail ungracefully if the reaction body is missing.

@cmnrd
Copy link
Collaborator

cmnrd commented Jun 19, 2023

One more comment: Currently the error message produced by the validator is "Missing semicolon at the end of reaction declaration." Perhaps we can add a little more context as to why the semicolon is needed and how to fix it. I am not sure if we condense this into a short message, but if not, we could add a page on the website and point to it.

@lhstrh lhstrh disabled auto-merge June 19, 2023 15:49
@lhstrh
Copy link
Member Author

lhstrh commented Jun 19, 2023

Adding a validator check is a great idea. This means we will only need to use a semicolon for reaction declarations (without a body) that have sources, right?

There should also be a validator check that ensures that the target code generator has support for bodyless reactions if they are used. Currently, the C++ generator fails with a nullpointer exception and I expect that also other target generators will fail ungracefully if the reaction body is missing.

Fixed in 8b920a8.

@lhstrh lhstrh added this pull request to the merge queue Jun 20, 2023
Merged via the queue into master with commit 1db4b0c Jun 20, 2023
@lhstrh lhstrh deleted the reaction-syntax branch June 20, 2023 06:15
@lhstrh lhstrh added the enhancement Enhancement of existing feature label Aug 28, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement Enhancement of existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants