Validation for assignments with `=` instead of `+=`

eclipse-langium / langium

Next-gen language engineering / DSL framework

https://langium.org/

MIT License

665 stars 61 forks source link

Validation for assignments with `=` instead of `+=` #1412

Open JohannesMeierSE opened 3 months ago

JohannesMeierSE commented 3 months ago

This PR adds validations for assignments using the = operator, while they got multiple values assigned. This validation gives the user the hint to use += as assignment operator instead.

Among others, the following details are considered during the validation and complemented with test cases:

assignments in neighbored alternatives
array multiplicities of outer groups
assignments in fragments which are called multiple times
actions creating new objects, which changes the location of stored values (that counts for looped actions as well!)
the assignment of a rewriting action

Lotes commented 2 months ago

Thanks @JohannesMeierSE ! Nice refactoring. No it is going down only and we can concentrate better on the special cases. Especially the one about multiple actions. Is it really a bad thing? Same we actually have for binary operators. Of course you could add some concrete example with two explicit actions and see what happens.

Apart from that, just two additional comments :).

JohannesMeierSE commented 2 months ago

Thanks @JohannesMeierSE ! Nice refactoring. No it is going down only and we can concentrate better on the special cases. Especially the one about multiple actions. Is it really a bad thing? Same we actually have for binary operators. Of course you could add some concrete example with two explicit actions and see what happens.

Apart from that, just two additional comments :).

Thanks @Lotes for the second review!

Yes, I like the new structure as well! In the end, it was easier than I expected before 🙂

The handling of two or more explicit actions within the same parser rule was already covered, I just forgot to remove my internal hint. The case with a single action with a loop around it was already covered and tested.

Additionally, I fixed this issue in another test case in the Langium code base and rebased this branch.

JohannesMeierSE commented 2 months ago

It is funny, there is a second checkRuleCallMultiplicity... but it has a different scope, it is not entirely what you are checking. WDYT about it?

Thanks @Lotes, yes, I saw https://github.com/eclipse-langium/langium/pull/1401, and you are right with the bad names: I just renamed the functions for these two validations and added a comment.