Bugzilla – Full Text Bug Listing
| Summary: | [parser] OCL 2.1 grammar precedence rule changes | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Modeling] OCL | Reporter: | Ed Willink <ed> | ||||
| Component: | Core | Assignee: | OCL Inbox <mdt-ocl-inbox> | ||||
| Status: | CLOSED FIXED | QA Contact: | |||||
| Severity: | normal | ||||||
| Priority: | P3 | CC: | alexander.igdalov | ||||
| Version: | 1.3.0 | Keywords: | plan | ||||
| Target Milestone: | 3.0.0 | ||||||
| Hardware: | PC | ||||||
| OS: | Windows XP | ||||||
| Whiteboard: | Compliance | ||||||
| Attachments: |
|
||||||
|
Description
Ed Willink
xor/or/and ---------- revising the grammar gives silent changes in behaviour. I've therefore implemented a suppressable warning for 2.1 behaviour that is different to 2.0. message ------- revising the grammar breaks 6 JUnit tests where Christian did Apple.allInstances()->any(true)^^ripen(Color::yellow)->notEmpty() The LHS is too complicated so m ust now be (Apple.allInstances()->any(true))^^ripen(Color::yellow)->notEmpty() This is a safe but slightly irritating change. Who uses messages? who uses message on more than a name as LHS? Since ^ and ^^ are not diadic, this change seems to be misguided. Christian's realisation with ^,^^,.,-> all equal seems much better. Do we want this change? Should we reject it with as OMG issue? The resolution for Issue 6544 has no associated rationale; it was an afterthough for xor/or/and. An issue is probably required to specify left recursion for e.g. name1.name2.name3 A decent resolution of this generic issue should specify a grammar with precedence (just like Java). Created attachment 146263 [details]
Fix for Xor, Or, And precedence
Attached (which excludes the auto-generated Java parsers) gives xor, or, and distinct precedence as per the imminent OCL 2.1.
If an xor/or/and expression has a different CST under OCL 2.0 a warning is generated. This may be suppressed via a ParsingOption.
I have left message precedence unchanged. I propose to raise a substantive OMG Issue revising the OCL grammar to provide a combined definition of syntax, precedence and associativity in the same style as the C++ or Java grammars. This should clarify message, if, let.
Hi Ed, I have a lot conflicts while trying to apply the patch, due to modifications in CVS, which come from other bug resolutions. I have tried to do the merge in a sensible way, but It's not easy when dealing with CST XMI text, specially when I don't know what you are trying to do in it. Is there any chance to do the modifications yourself, and uploading it again ?. As it has been manifested, I (we) shouldn't delay too much patches's revision, since we make the assignee waste time. I hope to respond earlier in future bugs. Another concern is that as far as I have understood from your comments, extra stuff has been included to "warn" previous clients of changes. I have exposed my opinion (see my last comment in Bug 285633) about this some times, which is in conflict with yours. I guess that we need Alex or even PMC's help to solve this. Cheers, Adolfo. (In reply to comment #3) > Another concern is that as far as I have understood from your comments, extra > stuff has been included to "warn" previous clients of changes. I have exposed > my opinion (see my last comment in Bug 285633) about this some times, which is > in conflict with yours. I guess that we need Alex or even PMC's help to solve > this. IMO, we shouldn't concentrate too much on warnings about OCL 2.1/2.0 incompatibility. But in case like this, when we can help users avoid hidden problems without little effort I give +1 for the option. However, I would unset it by default since we shouldn't warn users when they intentionally use OCL 2.1 precedence order in MDT OCL 3.0.0. (In reply to comment #1) > Apple.allInstances()->any(true)^^ripen(Color::yellow)->notEmpty() > > The LHS is too complicated so m ust now be > > (Apple.allInstances()->any(true))^^ripen(Color::yellow)->notEmpty() > > This is a safe but slightly irritating change. Who uses messages? who uses > message on more than a name as LHS? > > Since ^ and ^^ are not diadic, this change seems to be misguided. Christian's > realisation with ^,^^,.,-> all equal seems much better. > > Do we want this change? Should we reject it with as OMG issue? The resolution > for Issue 6544 has no associated rationale; it was an afterthough for > xor/or/and. +1 for rejection with an OMG issue. (In reply to comment #2) > Created an attachment (id=146263) > Fix for Xor, Or, And precedence Looks good. A few observations: 1. Although you introduce "isParenthesized" attribute to OperationCallExpCS, you seem to never check it. I suppose you intended to check it in AbstractOCLAnalyzer.checkForXorOrAndPrecedenceHazard(). Otherwise, you will report a warning in cases such as a xor (b and c) A JUnit test checking it would be helpful. 2. As I've mentioned I would set the ParsingOptions.WARN_OF_XOR_OR_AND_PRECEDENCE_CHANGE to `false` by default. 3. I would omit the ParsingOption.WARN_OF_MESSAGE_PRECEDENCE_CHANGE since we do not change anything in messages. There is also a typo in its javadoc comment - should be "MDT-OCL 1.x release", not "DT-OCL 1.x release" 4. As you have proposed we should eliminate extra copyrights. This should be applied to this patch as well;-) (In reply to comment #4) > But in case like this, when we can help users avoid hidden > problems without little effort I give +1 for the option. Oops, I meant "avoid hidden problems with little effort" =)) Committed to HEAD. #4 Very observant. isParenthesized was put in when I thought I needed to parse the OCL 2.0 way and fix up at analyze-time. Doing it just the 2.1 way with a warning doesn't need this so its gone. Junit test checks with/without parentheses. WARN_OF_MESSAGE_PRECEDENCE_CHANGE was similarly put in when I thought things were changing. They didn't. Option gone. Copyrights trimmed on all associated files. WARN_OF_XOR_OR_AND_PRECEDENCE_CHANGE default polarity is very debatable. If everyone likes false no problem. My view is that the OCL 2.0 has significant usage, so the OCL 2.1 makes this syntax fundamentally unsafe, so it needs a warning till OCL 3.0. cf certain Java usages that must be rewritten of @suppressWarning'd. Here the warning goes if you put in parentheses. ---- Awaiting OMG Issue to: Define message precedence and associativity. (In reply to comment #6) > Committed to HEAD. > > #4 Very observant. > > isParenthesized was put in when I thought I needed to parse the OCL 2.0 way and > fix up at analyze-time. Doing it just the 2.1 way with a warning doesn't need > this so its gone. Junit test checks with/without parentheses. > I thought isParenthesized was introduced to selectively warn users only in cases when and/or/xor precedence is changed. Now you report a warning in the case shown below: (true or (false and false)) = true Why? Here the precedence is the same both in OCL 2.0 and OCL 2.1. We shouldn't warn users in the following case as well: (true or false.and(false)) = true But now the warning is reported. IMO the revised isParenthesized (i.e. isParenthesizedOrNonInfix) concept would be ideal to solve the revealed problems. > > WARN_OF_XOR_OR_AND_PRECEDENCE_CHANGE default polarity is very debatable. If > everyone likes false no problem. My view is that the OCL 2.0 has significant > usage, so the OCL 2.1 makes this syntax fundamentally unsafe, so it needs a > warning till OCL 3.0. cf certain Java usages that must be rewritten of > @suppressWarning'd. What I see in CVS now is the WARN_OF_XOR_OR_AND_PRECEDENCE_CHANGE option set to true by default. From your comment above I conclude that it is a mistake. Am I correct? IMO we are implementing OCL 2.1 in MDT OCL 3.0.0 and we shouldn't warn users by default in case they correctly use OCL 2.1. We've got this right because we are changing the major segment in the version. We can help users avoid problems if they migrate from OCL 2.0 - but IMO it shouldn't be the default option. Observant again. isAtomic introduced as per the earlier isParenthesised to cover explicot dot calls too. JUnit test now covers these cases. After reviewing xor precedence in a variety of languages, use of xor without parentheses is clearly unwise. OCL 2.1 is more sensible. OCL 2.0 without any left association specification had no predictable meaning; the MDT OCL 1.3.0 meaning was useful but not defined. The current MDT OCL 3.0 behaviour is hopefully more useful, and defined for these operators ... So since OCL 2.1 is sensible and OCL 2.0 crazy I am happy to warn only when people want to know about a crazy language. Thanks, Ed! Looks good. Closing as RESOLVED FIXED. Closing after over 18 months in resolved state. |