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

Validation for assignments with = instead of += #1412

Merged

Conversation

JohannesMeierSE
Copy link
Contributor

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

@JohannesMeierSE JohannesMeierSE requested a review from Lotes April 10, 2024 15:27
@Lotes
Copy link
Contributor

Lotes commented Apr 11, 2024

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 JohannesMeierSE force-pushed the validation-assignments-cardinality branch from a6ee465 to 5dc9426 Compare April 15, 2024 08:35
@JohannesMeierSE
Copy link
Contributor Author

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.

Copy link
Contributor

@Lotes Lotes left a comment

Choose a reason for hiding this comment

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

Looks good to me :). Thanks.

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?

@JohannesMeierSE
Copy link
Contributor Author

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 #1401, and you are right with the bad names: I just renamed the functions for these two validations and added a comment.

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me. Just a few minor improvements, see below.

packages/langium/src/grammar/validation/validator.ts Outdated Show resolved Hide resolved
packages/langium/src/grammar/validation/validator.ts Outdated Show resolved Hide resolved
packages/langium/src/grammar/validation/validator.ts Outdated Show resolved Hide resolved
packages/langium/src/grammar/validation/validator.ts Outdated Show resolved Hide resolved
@JohannesMeierSE
Copy link
Contributor Author

Thanks @msujew for your review! In my eyes, this feature is ready to merge.

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me 👍

@msujew msujew merged commit 58e28b7 into eclipse-langium:main Oct 8, 2024
4 checks passed
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