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

TGG editor: Insufficient validation for node types for refined rules #73

Open
patrickrobrecht opened this issue Mar 26, 2018 · 14 comments
Labels

Comments

@patrickrobrecht
Copy link
Contributor

Example:
Super rules: Declared node p: Person and p: Female
Refined rules: p: Male does not show an error in the editor (but transformations fail which is written on the console).

@anthonyanjorin
Copy link
Contributor

Any progress? Have you been able to reproduce with a concrete example?

@patrickrobrecht
Copy link
Contributor Author

For the GT part a check for this has been implemented:

@LegionaryCohort
Copy link
Contributor

I was unable to reproduce the problem. I've tested this using the BenchmarxFamiliesToPersons TGG test project:
The abstract rule FamilyMember2Person contains a node definition ++ p : Person, the refined rule FatherToMale overwrites that to ++ p : Male. Changing the definition in the abstract rule to ++ p : Female causes no errors when running MODELGEN. Src, trg and corr model instances are produced containing the affected nodes. It appears that the node definition in the refined rule completely overwrites the definition in the abstract rule.

As far as I understand it, this refinement shouldn't be valid, but it appears to work fine regardless. Am I misunderstanding the issue?

@LegionaryCohort
Copy link
Contributor

LegionaryCohort commented Dec 12, 2018

I should note that I had to add some max-rule-counts to the MODELGENStopCriterion to ensure that it doesn't simply create large heaps of empty families. This shouldn't have any effect on this issue though, should it..?

@anthonyanjorin
Copy link
Contributor

I should note that I had to add some max-rule-counts to the MODELGENStopCriterion to ensure that it doesn't simply create large heaps of empty families. This shouldn't have any effect on this issue though, should it..?

No not at all.

@anthonyanjorin
Copy link
Contributor

I was unable to reproduce the problem. I've tested this using the BenchmarxFamiliesToPersons TGG test project:
The abstract rule FamilyMember2Person contains a node definition ++ p : Person, the refined rule FatherToMale overwrites that to ++ p : Male. Changing the definition in the abstract rule to ++ p : Female causes no errors when running MODELGEN. Src, trg and corr model instances are produced containing the affected nodes. It appears that the node definition in the refined rule completely overwrites the definition in the abstract rule.

As far as I understand it, this refinement shouldn't be valid, but it appears to work fine regardless. Am I misunderstanding the issue?

Hmm... this is a difficult question/decision. I think the easiest answer is that Male should not be able to override Female in a refinement, as it is not a subclass. The argument for this is that arbitrary overriding would require checking all possible links attached to the node. Overriding only with subclasses is automatically safe in this regard. Allowing arbitrary overriding would be flexible, but also a bit "dirty": for example, it would work for some time, but suddenly stop working as soon as a forbidden link is added in the rule.

So I would vote for forbidding this (even if it limits specification freedom a bit).

What do you think?

@LegionaryCohort
Copy link
Contributor

I agree that only subclasses should be allowed, especially since this conforms to the intuitive understanding of inheritance/refinement.

This aside, the description of this issue mentions that transformations fail with a corresponding error output to the console. This was not the case for me, which leaves me somewhat uncertain as to whether I'm understanding correctly what the issue exactly is.

@anthonyanjorin
Copy link
Contributor

Apparently some recent changes have made things more robust... so all that is needed here is a validation rule in the xtext editor with a useful error message. I’m sure enough that we understand the issue correctly.

@LegionaryCohort
Copy link
Contributor

If I'm seeing this right, it should be sufficient to check each rule's source and target patterns against the patterns of the rule's supertypes (recursively, not just against the direct supertype).
Is there any other place where the type-check should be done?
The correspondence types defined in the schema cannot be overwritten, right?

@anthonyanjorin
Copy link
Contributor

@LegionaryCohort Have you seen the tips provided by @patrickrobrecht above? You might be able to just adapt the code to the case of TGGs...

@anthonyanjorin
Copy link
Contributor

Please also check if this issue is not very much related to: eMoflon/emoflon-ibex#369

@LegionaryCohort
Copy link
Contributor

I've already looked at the code in the GTValidator. I'm expecting the code for TGGs to end up looking very similar, but the GT code uses some utility methods that don't exist for TGGs as far as I can tell.

As for the other issue, as far as I can tell they are certainly related, but still seperate issues. If I'm understanding the other Issue correctly, it's dealing with a diamond problem caused by being able to refine multiple rules. This Issue on the other hand is a much more basic type-checking problem.

@anthonyanjorin
Copy link
Contributor

As far as I can tell the issues are one and the same - our refinement flattening Code first combines all super rules with the current rule, and then picks out a clear subtype from all candidates (with the same name). If this candidate is not unique, we should complain and list all conflicting candidates. I think the other issue would be solved by this as well.

But just start somewhere and yell if you can’t make any progress.

@LegionaryCohort
Copy link
Contributor

All right, given the additional knowledge about how the refinements are flattened, you've convinced me that they are indeed pretty much the same.

As discussed in Slack, I'll put this issue on hold for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants