Bug in OperationCallExpOperations.checkArgumentsConform restriction #284
Comments
|
By Adolfo Sanchez-Barbudo Herrera on May 14, 2008 05:34 (In reply to comment #0)
Sorry, that statement is wrong. It should use environment just for instropection, or It shoud not use the environment for lookups. Cheers!! |
|
By Christian Damus on May 14, 2008 07:02 Hi, Adolfo. Is this a major severity? On the newsgroup, you suggested that it was a severe problem for QVT's model of operations. |
|
By Adolfo Sanchez-Barbudo Herrera on May 14, 2008 07:14 (In reply to comment #2)
Yes it is. All imperative call exp, throws an "Illegal operation" error due to the referred operation is not found by env.lookupOperation. I have decided not checking this restriction by now (losing the argument conformance :( ). Sorry I'm not used to changing severity levels. I thought it's labour for the committers. Cheers. |
|
By Christian Damus on May 14, 2008 07:21 No problem. FYI, the Severity field is intended for the bug reporter to indicate the impact of the bug on his client code. The Priority field is used by the component team to indicate how urgent we consider the need to fix the problem in comparison to everything else on the plan. |
|
By Christian Damus on May 14, 2008 20:50 Created Attached a patch to implement the proposed change, doing a straight-forward comparison of operation-call arguments against parameters. Added Ecore and UML JUnit tests to check the handling of generic parameters (the "T" of collection operations). |
|
By Christian Damus on May 14, 2008 20:52 Adolfo, can you try the patch that I attached, above, to check that it works for you? I would like independent confirmation before I commit this to the RC1 build on 19 May. |
|
By Adolfo Sanchez-Barbudo Herrera on May 15, 2008 07:19 It's very nice ;) Just one thing: the following error is thrown when the arguments count is different from the parameters one; Validator-ERROR in org.eclipse.ocl.expressions; MappingCallExp.qvto:37 : ? I would suggest, no evaluating args conformance, since the further args count will fail. if (parms.size() != args.size()) { Another possibility, would be thowing two errors. However, the message needs to be configured: if (parms.size() != args.size()) { P.S: Uou List<?>!!!! It's clear that I'm not used to working with generics :). I won't forget the wildcard anymore. Cheers, |
|
By Christian Damus on May 15, 2008 09:20 Thanks, Adolfo, for reviewing the patch. Yes, I agree that the conformance shouldn't be tested for the wrong number of arguments, because that is the other constraint's job. I'll update that, and the tests (which actually check for this bad behaviour). |
|
By Christian Damus on May 15, 2008 20:46 Created Hi, Adolfo. I have attached an amended patch that ignores operation calls bearing the wrong argument count, as suggested. If you like this, I shall commit it for RC1 and that'll be a wrap on the validation for this release. |
|
By Adolfo Sanchez-Barbudo Herrera on May 16, 2008 07:25 Perfect !!!! Regards. |
|
By Christian Damus on May 16, 2008 20:54 Committed the second patch to CVS HEAD (1.2 branch). |
|
By Adolfo Sanchez-Barbudo Herrera on May 28, 2008 12:58 Created I'm afraid that I have to reopen the bug. The provided test cases demonstrate that the validator find a problem when checking the argument against a parameter whose type is a generic collection (occurs with several standard operations of the oclstdlib: includesAll, excludesAll, union, etc). Studying the issue, the problem relays on the fact that TypeUtil.compatibleTypeMatch doesn't take into account the generic type (T and T2). TypeUtil.matchArgs seems to be more powerful , since it receives the type that owns the operation so that it can resolve the generic type to the specific, and check it with the argument. Maybe in 1.3, a public checkArg could be used by checkArgumentConform. So the type of the argument and the (generic) parameter is strictly checked. The generic type is considered in checkArgumentsConform, but the generic collection type isn't. So an action is needed since the actual EValidator throws An "emergency solution" could be avoiding the checking if the param type is any of the generic collections. Later in 1.3 we could include a better argMatch. The provided patch fixes the problem. Another possibility, is going back to the old version (which, indeed, use matchArgs via lookingOperation). At least, It should work for pure OCL Validators. That's all the info I can provide... Cheers, |
|
By Christian Damus on Jun 05, 2008 09:52 Actually re-opening the bug, as Adolfo indicates should be done. I'll look at doing this in the 1.2.1 maintenance release (it's too late for 1.2.0 because there is a work-around for QVT, which extends the model). |
|
By Adolfo Sanchez-Barbudo Herrera on Jun 05, 2008 10:17 Hi Christian, Just remarking (I suppose that you have taken it into acount) that the applied patch (the second one) in RC1, partially solves QVTo models validation, but breaks OCL models validation one. Anyway, It seems that nobody is using EValidator for validating OCL models, so the bug resolution can wait :). Cheers, |
|
By Christian Damus on Jun 05, 2008 11:29 (In reply to comment #14)
My thoughts, exactly. This feature really did target QVT in this release (perhaps it should have been marked as "provisional"). In any case, OCL itself doesn't use it, but I hope to change that in the next release. |
|
By Ed Willink on Jun 06, 2008 02:04 The bug should not have been reopened. The new bug is different and a bad regression failure between M7 and RC1. I now get 98 validation failures each for one of my QVT Relation regression tests. If the bug cannot be fixed for Ganymede the validation must be backed out. No validation is better than bad validation. The problem is that OperationCallExpOperations.checkArgumentsConform special cases with parmType != stdlib.getT(), parmType != stdlib.getT2() to workaround the inability of TypeUtil.getRelationship to handle T. Consequently when, with for instance intersection(), Set(Variable) is compared against Set(T) the inadequacies of TypeUtil.getRelationship show up. The above seems like an OCL not QVT problem. |
|
By Christian Damus on Jun 08, 2008 14:55 I'd like to revert this to the M7 implementation. Let's see what the Modeling PMC thinks. |
|
By Christian Damus on Jun 08, 2008 15:00 Created attachment 104111 Attaching a patch to revert to the M7 implementation. This still passes all of the OCL JUnit tests, including the new ones added when attempting to fix the QVT problem of operation look-up in RC1. This constraint now functions exactly as the ValidationVisitor that is used by the OCL parser, and has been working OK for the last three releases. :notepad_spiral: Eclipse232028patch |
|
By Christian Damus on Jun 08, 2008 15:06 Kenn and Ed W., can you please review the attached patch that reverts this constraint to the M7 implementation to resolve the regression indicated by Ed in comment #16? OCL has no other committers but me, so I need some surrogates to meet the Modeling ramp-down policy. Kenn and Ed M., do I have PMC approval to revert this regression to the algorithm that worked in M7? |
|
By Ed Willink on Jun 08, 2008 16:51 Great. This gives me the same test results as M7. +1 (if this is what your surrogate should say). |
|
By Christian Damus on Jun 09, 2008 10:28 Thanks, all, for your support on this problem. I have committed the patch for today's RC4 build. |
|
By Christian Damus on Jun 09, 2008 20:23 Fix available in HEAD: 1.2.0RC4 (S200806091714). |
|
By Ed Willink on May 27, 2011 02:40 Closing after over a year in verified state. |
|
By Ed Willink on May 27, 2011 02:41 Closing after over a year in verified state. |
|
By Ed Willink on Mar 25, 2024 16:36 Oops 16 years later I find a request. |
| --- | --- |
| Bugzilla Link | 232028 |
| Status | CLOSED FIXED |
| Importance | P2 critical |
| Reported | May 14, 2008 04:55 EDT |
| Modified | Mar 25, 2024 16:36 EDT |
| Version | 1.2.0 |
| Blocks | 236247 |
| Reporter | Adolfo Sanchez-Barbudo Herrera |
Description
OperationCallExpOperations.checkArgumentsConform restriction should not use the Environment for introspection.
Checking all the arguments of the OperationCallExp with all the parameters of the referredOperation should be done instead.
I have tried to workaround the patch, but I was pwned when I tried to get the parameters of the referred operation, the generic PM is not in the API for the method. I suppose that adding it freely is not the way (API change).... ¿ more ideas ?
Cheers,
Adolfo.
The text was updated successfully, but these errors were encountered: