search

eclipse-ocl / org.eclipse.ocl

Eclipse Public License 2.0
0 stars 0 forks source link

[parser] OCLLPGParser.g grammer incorrectly defines 'if' expression #170

Closed eclipse-ocl-bot closed 1 month ago

eclipse-ocl-bot commented 1 month ago

| --- | --- | | Bugzilla Link | 184048 | | Status | CLOSED FIXED | | Importance | P3 normal | | Reported | Apr 25, 2007 12:18 EDT | | Modified | May 27, 2011 02:47 EDT | | Version | 1.1.0 | | Blocks | 191819 | | Reporter | Sergey Boyko |

Description

According to OCL concrete syntax:

OclExpressionCS definition contains\ [F] OclExpressionCS ::= IfExpCS\ OperationCallExpCS definition contains\ [C] OperationCallExpCS ::= OclExpressionCS ‘.’ simpleNameCS ‘(‘ argumentsCS? ‘)’

So that the following expression should be parsed:\ if 1>0 then a else b endif . someOperationCall()

Actually this is parsed with error states that another input (instead of '.') is expected. Seems the reason is in the following rule \ ifExpCSPrec -> ifExpCS\ from OCLLPGParser.g. This rule is defined 'if' expression too early in rule's sequence tree. \ It should be removed and rule\ oclExpCS -> ifExpCS\ should be added instead.

eclipse-ocl-bot commented 1 month ago

By Christian Damus on Apr 25, 2007 12:50

I'm not sure about that. Section 9.3.2 defines the operator precedence, in which if-then-else-endif is explicitly set to a lower precedence than '.' and '->' (and the arithmetic operators, too).

So, I think it is necessary to enclose the if-then-else-endif in parentheses in order to apply '.' to its value. You can follow the 'endif' with 'and' or another boolean operator, though, because these are at a lower precedence.

eclipse-ocl-bot commented 1 month ago

By Sergey Boyko on Apr 26, 2007 06:22

Since in OCL all parts of 'if-then-else-endif' is mandatory I don't see any example when precedence of the operators results in syntax error. \ 'if-then-else-endif' can be considered as atom so '(a)' is equivalent to 'a'.

eclipse-ocl-bot commented 1 month ago

By Sergey Boyko on Apr 26, 2007 09:41

Btw, same problem with 'if-then-else-endif' was encountered with the following expression:\ 'text' + if a.oclIsUndefined() then 'a' else 'b' endif

Erro is an unexpected input after '+'.

eclipse-ocl-bot commented 1 month ago

By Christian Damus on Apr 26, 2007 09:45

Yes, but that's again the precedence defined by OCL: + has higher precedence, so you need parentheses on the if-then-else-endif. I don't think the parser should allow expressions that will not compile in other spec-compliant implementations.

eclipse-ocl-bot commented 1 month ago

By Christian Damus on May 31, 2007 10:27

Investigate possible grammar changes for 1.2 release.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Aug 28, 2009 17:25

The OCL 2.0 and 2.1 grammar precedences are specified in a very misleading and piecemeal fashion. The CS-AS mapping has no mention of precedence. The precedence has no mention of CS.

if-then-else-endif is a unary term without any vague edges. I agree with with #2, that it is an atom. It should appear at the top. Clearly even

^^ if ... endif @pre

means

^^ ((if ... endif) @pre)

The grammar also looks suspect for let-in, which has a vague right hand edge that should be correctly resolved by an accurate grammar. The grammar currently has numerous duplicate ...let terms.

messageExpCS also looks suspect.

OCL 2.1 requires prioritising xor, or, and.

These problems can all be handled at once.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Aug 30, 2009 02:35

letExpCS is coorect for the OCL 2.1 clarification.

messageExpCS is not incorrect wrt OCL 2.0. It needs changing for 2.1.

Changes to support 2.1 are in Bug 288040.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Sep 11, 2009 15:11

Created attachment 146988 (attachment deleted)\ Resolution of if and many names issues

In addition to fixing this if precedence anomally, thios attached fix has significant cleanup\ of names and parsing to support bug 288575.

Fix some @since 3.0's that don't regenerate.

Eliminate StateExpCS

CollectionTypeCS is a SimpleNameCS

PrimitiveTypeCS is now a SimpleNameCS allowing usage within a pathNameCS and eliminating\ an unnecessary 'the order of these rules is important!' comment.

Add OperationCallExpCS.pathNameCS to support qualified names.

Eliminate the following keywords\ attr\ oper\ oclIsKindOf\ oclIsTypeOf\ oclAsType\ oclIsNew\ oclIsUndefined\ oclIsInvalid\ oclIsInState\ allInstances

Migrate the folowing keyword defs from EssentialOCL to OCLParser\ def\ endpackage\ inv\ post\ pre\ static

Introduce and use the clear\ notIteratorNorReservedSimpleNameCS\ notLiteralNorReservedSimpleNameCS\ notReservedSimpleNameCS\ reservedSimpleNameCS\ operationOrNotReservedSimpleNameCS\ simpleNameCS\ productions to continue to parse what was parsed before and more.

Flatten all the dot, arrow and message productions to eliminate the shift reduce conflicts\ that prevented parsing of awkward names. Parsing now matches the OCL 2.0 overenthusiasm.\ create methods revise to match the flatten crteation context.\ The resulting productions much more closely resemble the OCL 2.1 Concrete Syntax.\ Rename variableExpCS as associationClassCallExpCS which is what is implemented.

Rename isMarkedPreCS as isMarkedPreCSopt.

AbstractOCLParser.operationCallExpCS is refactored to peel off\ arrowOperationCallExcpCS\ staticOperationCallExpCS\ oclIsInStateOperationCallExpCS\ making the functionality much clearer.

(There is still a little bit more cleaning up worth doing, why is one name a VariableExpCS, two names a PathNameCS and three an EnumLiteralExpCS?,\ however this is enough cleaning up for one commit, with two others awiting +1 too.)

eclipse-ocl-bot commented 1 month ago

By Adolfo Sanchez-Barbudo Herrera on Oct 02, 2009 07:02

Hi Ed,

Is this patch completed? Does it depend on a different one ?

After applying several compilation errors appear,\ After regenerating from grammars other errors appear,\ After investigating I see that, for instance, a new notReservedSimpleNameCS rule appears in OCLParser.g, but it's no defined anywhere.

I guess that at least EssentialOCL.g is missing, isn't it ?

BTW, from the bugzilla's thread I assume that precendence operator's is not correct in OCL 2.1. I also agree with Comment 2. Did you send an issue to OMG ?

Cheers,\ Adolfo.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Oct 04, 2009 07:48

Yes EssentialOCL.g is missing; there were a lot of important ambiguity cleanups. I naively thought that the patch was my backup. It was over a week ago so it's not in my Eclipse history. I really am developing this twice!

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Oct 07, 2009 14:34

Created attachment 149024 (attachment deleted)\ Changed files for bug fix

Perhaps bug 291635 explains why the patch was incomplete.

Anyway after much pain, I hope the attached represents the changed files. I am afraid that the bug prevents me submitting as a patch.

In addition to the changes StateExpCS and StateExpCSImpl can be deleted.

eclipse-ocl-bot commented 1 month ago

By Adolfo Sanchez-Barbudo Herrera on Oct 08, 2009 08:51

Hi Ed,

I have successfully created the patch. I have uploaded with it some trivial changes:

About the patch.\ Ouo, I think that the part needed to solve the bug is a 0.0000...01 part of it >.<.... This is refactoring patch rather a resolution of the if precedence.

In general I like the changes and the test passes: Good ;).... Some comments:

I need more time to make some check VariableCS/VariableCS" and OperationCallExpCS, and play a little more with this. I hope to +1 tomorrow.

BTW, I haven't found any issue in OMG-OCL's page related to this 'if' precedence changes, have you ?. This must be submitted to OMG.

Cheers,\ Adolfo.

eclipse-ocl-bot commented 1 month ago

By Adolfo Sanchez-Barbudo Herrera on Oct 08, 2009 08:52

Created attachment 149105 Fixing patch

Sorry, forgot to attach the patch.

:notepad_spiral: ocl184048.patch

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Oct 08, 2009 16:26

I can't see any changes other than blank lines some in comments and two imports.

The change of asserts in escapeSequence is perhaps an accidental incorporation of patch 285120 which was approved two months ago but has not been committed. Maybe I'll have to do it. Need it out of the way to progress with Issue 14537.

Yes. I suspect SimpleTypeEnum is a waste of space. I think 'KEYWORD' is correct but there is no definition of what 'KEYWORD' means and the OMG spec is vague too.

I'm trying to make the grammar much more rteadable so that I can just strip the actions and be close to the official grammar that Issues 10439 and 13076 call for.

I find many small Issues very difficult to handle because there is no coherent revised text to show where we are now, so I want a single Issue to merge all the others and resolve

reserved words\ precedence\ associativity\ grammar

issues all at once, based on an implementation that really works.

true and null and self not being reserved words is quite ridiculous, though the grammar demonstrates that it is possible; however if they're not reserved what syntax forces resolution in favour of the OCL special semantic rather than a user name; oh! they must be reserved then.

Bag etc not being reserved is also pretty cretinous by the same argument. Resolution of Issue 12953 makes reserving them essential.

Having arranged for the grammar to manage to not reserve them, the change to reserved is really easy.

Please review the nice trivial bug 288601 so that I can commit both together.

eclipse-ocl-bot commented 1 month ago

By Adolfo Sanchez-Barbudo Herrera on Oct 09, 2009 07:13

(In reply to comment #14)

I can't see any changes other than blank lines some in comments and two imports.

As I said, just some @since and removal of imports (maybe, some blanks) were included from the original zipped modified files.

The change of asserts in escapeSequence is perhaps an accidental incorporation of patch 285120 which was approved two months ago but has not been committed. Maybe I'll have to do it. Need it out of the way to progress with Issue 14537.

Yes. I suspect SimpleTypeEnum is a waste of space. I think 'KEYWORD' is correct but there is no definition of what 'KEYWORD' means and the OMG spec is vague too.

I have only found in 7.4.9: "keywords cannot occur anywhere in an OCL expression as the\ name of a package, a type, or a property". In any case, the following still needs to be changed: "reservedKeywordCS/reservedPunctuationCS should be\ KEYWORD_LITERAL and iterateNameCS/iteratorNameCS should be IDENTIFIER_LITERAL". Change this before committing.

I'm trying to make the grammar much more rteadable so that I can just strip the actions and be close to the official grammar that Issues 10439 and 13076 call for.

I agree. On the other hand, I guess we will have some complains from extending languages implementors, specially QVTo who may need to modify OCL rules (I haven't had any time to check the impact to my QVto grammar). Anyway, since we are in a major increment, QVTo has time to react to this changes, we shouldn't have any problem with it.

I find many small Issues very difficult to handle because there is no coherent revised text to show where we are now, so I want a single Issue to merge all the others and resolve

reserved words precedence associativity grammar

issues all at once, based on an implementation that really works.

Ok.

true and null and self not being reserved words is quite ridiculous, though the grammar demonstrates that it is possible; however if they're not reserved what syntax forces resolution in favour of the OCL special semantic rather than a user name; oh! they must be reserved then.

Like in Java ;=P.

Bag etc not being reserved is also pretty cretinous by the same argument. Resolution of Issue 12953 makes reserving them essential.

Having arranged for the grammar to manage to not reserve them, the change to reserved is really easy.

Please review the nice trivial bug 288601 so that I can commit both together.

Done.

------------------------------------------\ More things:

I'll +1. I think there could be some noise around this. However the sooner the noise appears, the best for us, as we will have more time to fix any side-effect. I think this is a good contribution which we shouldn't block.

P.D: Don't forget to regenerate (CST and grammarS) and remove fix the warning (or still missed @since) in the generated files before committing.

Cheers,\ Adolfo.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Oct 10, 2009 03:16

Changes committed to HEAD; with considerable difficulty. Most mult0file commits timed out of got a connection resert by peer, so I had to proceed file by file. Please watch your CVS activities very carefully. I/we need to discover if I have a network problem, the CVS server has a problem, our CVS database is partially corrupt.

Re #12

BasicOCLTEst now only adds a test.

Re #15

In any case, the following still needs to be changed: "reservedKeywordCS/reservedPunctuationCS should be KEYWORD_LITERAL and iterateNameCS/iteratorNameCS should be IDENTIFIER_LITERAL".

Absolutely.

More things:

  • Shouldn't the CompleteOCL keywords of EssentialOCL.g%Keywords be moved to OCLParser.g ?

Yes. I moved them at one point, but then realised that until the OCL spec reserves them just for CompleteOCL they are reserved for EsentialOCL too and\ to support this we have to have an EssentialOCLKWLexer. Another Bugzilla needed.

  • I have to confess that this is dangerous patch:
    • I can't sinceresly assert that there won't be any side-effect.
    • I really preferred the old . You recall the need of removing shift-reduce conflicts. Let's accept then.

Did you miss out a word "old" what?

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Oct 10, 2009 03:37

Fixed

eclipse-ocl-bot commented 1 month ago

By Adolfo Sanchez-Barbudo Herrera on Oct 13, 2009 14:29

(In reply to comment #16)

Changes committed to HEAD; with considerable difficulty. Most mult0file commits timed out of got a connection resert by peer, so I had to proceed file by file. Please watch your CVS activities very carefully. I/we need to discover if I have a network problem, the CVS server has a problem, our CVS database is partially corrupt.

I haven't particularly experimented any problem with CVS. I have only made some updates and created some patches, though. No commits during the last weeks.

  • I have to confess that this is dangerous patch:
    • I can't sinceresly assert that there won't be any side-effect.
    • I really preferred the old . You recall the need of removing shift-reduce conflicts. Let's accept then.

Did you miss out a word "old" what?

I meant old callExp style:

callExp-> '->' featureCallExp\ callExp-> '.' featureCallExp\ etc.

Cheers,\ Adolfo.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Oct 14, 2009 02:19

RE #16\ I had no problems creating the patch for 292112, which was a big improvement. I still need to commit not more than a cou[ple of big files at a time. I suspect the major problem is in the 'Hebrew' JUnit test file which is causing a CVS server diff failure either through the UTF-8 or the large number of quoted backslashes.

The old callExp style is not what the spec does and was one of the practices that introduced spurious shift-reduce conflicts. Now that bug 292112 has a simple closely complinat grammar, it could be refactored to exploit such commonality, but would then diverge from the OMG spec.

eclipse-ocl-bot commented 1 month ago

By Adolfo Sanchez-Barbudo Herrera on Oct 14, 2009 05:09

The old callExp style is not what the spec does and was one of the practices that introduced spurious shift-reduce conflicts. Now that bug 292112 has a simple closely complinat grammar, it could be refactored to exploit such commonality, but would then diverge from the OMG spec.

Ed, you are very right. I didn't check the spec's grammar at this point. As u said I liked the practical old style, but this is much closer to the spec. I think this could stay as it is.

Cheers,\ Adolfo.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on May 27, 2011 02:47

Closing after over 18 months in resolved state.