Comment 1 Ed Willink 2007-03-05 15:04:54 EST

I fixed the wrong side of the reduce-reduce conflicts.

x.not() is a perverse but valid spelling of not x.

The grammar error was to declare not and minus as operation call prefixes, which they cannot be. The correct fix, now in the UMLX CVS, is to only declare not and - as FeatureCallExps.

Comment 2 Ed Willink 2007-04-12 17:02:45 EDT

The extensible version of the grammar matching the M6 release and fixing 182201 may be found at /cvsroot/technology org.eclipse.gmt/umlx/plugins/org.eclipse.gmt.umlx.cst.
Note the change of plug-in to decouple from a particular QVT plug-in.

The extensible grammar is now extended without modification by two QVT grammars (core and relation) and the abstract parser and error handlers by a non-OCL grammar (KM3). All OCL specifics have therefore been migrated down from AbstractParser to AbstractOCLParser.

Comment 3 Ed Willink 2007-09-15 14:10:23 EDT

Created attachment 78499 [details]
Patch to support grammar extension and customisable problem handling

I assume you'll have to sort out all the copyrights etc. No problem assigning all as your standard IBM starter and me as a secondary contributor even for new classes.

Passes 365/365 Ecore and 364/364 UML, modifying only one Ecore test to add a context argument.

Comment 4 Christian Damus 2007-09-17 09:45:26 EDT

Fabulous!  Thanks, Ed.  I'll have a look right away.  Adolfo has expressed interest in reviewing this, too.

Comment 5 Christian Damus 2007-09-17 13:26:37 EDT

Hi, Ed,

Some comments from my initial review:

  - I like these changes!  It will significantly improve the error reporting which (not being a
    client of grammar extensibility) is of particular interest to me; I've wanted something
    like this for a long time

  - the refactoring doesn't cover the org.eclipse.emf.ocl compatibility plug-in (and tests).
    I will attach a patch that updates these (it was easy to do) and results in 100% JUnit pass rate
    
  - ProblemHandler API uses IProgressMonitor from Eclipse run-time.  Using
    org.eclipse.emf.common.util.Monitor from EMF would be better, as we need to continue to
    support stand-alone deployment (EMF provides adapters that delegate to IProgressMonitors)
    
  - LookupException:  adding a new checked exception to the Environment[Factory] API for
    looking up packages and classifiers is an incompatible API change for users and implementers
    of the Environment API; narrowing the exception type of lookupState(...) is an incompatible API
    change for implementers.  I see that in the first two cases, the environment implementations
    don't actually throw the exception.
    
  - extending BasicEnvironment is an incompatible API change for implementers of Environment.
    Can it be an optional mix-in interface, instead?  The default environment implementations
    should implement it, but the parser could create a delegate with appropriate error handling for
    environments that don't implement this interface
    
  - OCLStandardLibraryUtil/TypeUtil:  adding problemObject parameters to existing methods is
    an incompatible API change.  Can this easily be accommodated by deprecating the old
    signatures and delegating to the new implementations with a null problemObject?

Comment 6 Christian Damus 2007-09-17 13:29:17 EDT

Created attachment 78575 [details]
Patch for OCL 1.0 compatibility plug-in

Attached a patch for the OCL 1.0 compatibility plug-in (org.eclipse.emf.ocl) and its tests.  Includes a change to the new LookupException to supply the (optional) list of ambiguous matches, needed to support the way the old API worked.

100% JUnit pass with these changes in all three test suites.

Comment 7 Ed Willink 2007-09-17 15:21:35 EDT

IProgressMonitor: org.eclipse.emf.common.util.Monitor is fine, probably better. Though I thought I checked on the usage of org.eclipse.core.runtime and found that it was already in use.

Narrower Exceptions. For my 'development' I deliberately used new Exceptions to demonstrate that I was eliminating the dual use of ParserException/SemanticException - as a problem detection escape in inner routines, and as an outer reporting API. I eliminated the inner exceptions, introducing TerminateException when instant abort is required by the CompatibilityProblemHandler. TerminateException is caught and converted to the existing exceptions. These exceptions could all be broadened to match prior APIs, it's just the wider exception does not prohibit exception passing as problem handling.

Wider exceptions. Lookup errors are very rare, since the basic protocol is return something good, or null if nothing good. Exceptions only occur in the case of semantic impossibility; typically an inability to decide which of two good things to return. Philosophically this is true of all lookup methods. For the prior OCL code this only happened for state, and so this is the only place you see it happen, and so it looks to be narrower (but could be broadened). For QVT, classifiers can be looked up with missing package qualification, so an ambiguous lookupClassifier return is possible, hence the new exception. I think it is very desirable to use precisely LookupException for all lookups.

BasicEnvironment was a late addition. Its content was originally added to Environment. Splitting it off drastically reduced the delta to AbstractEnvironment, gave much tidier layering and allowed me to inherit from AbstractBasicEnvironment rather than AbstractEnvironment for a non-OCL LPG-based parser/unparser (the KM3 Ecore syntax). Since it is a fattened base I don't think this causes any problems to existing code that inherits AbstractEnvironment. I'm sure it could be a mix-in, but is it worth it? BasicEnvironment has 15 methods, 8 of which are just convenience methods. set/getParser are essential to support navigation from the environment via token indexes to the source text. set/getProblemHandler are fundamental to having a problem handling capability. getFormatter is necessary to allow message contributions to support customisable formatting. initASTMapping/getASTMapping can be dummies, but moving them down the hierarchy makes access from AST to source text difficult. I think that AbstractBasicEnvironment provides an adequate default for code to copy/implement. All clients should really be reviewing their problem handling behaviour because the CompatibilityProblemHandler is quite dreadful, and most of the OCL/Helper methods that rely on it are somewhat of a hindrance if you want to do something more sensible.  

OCLStandardLibraryUtil/TypeUtil extra/deprecated methods. Yeah no problem. I suspect that these and a few other areas may merit further revision when you consider them in the light of the new ProblemHander accumulates problem rather than aborting philosophy.

[If there's any plugin that can tolerate the Eclipse-specificness, I have a MarkerProblemHandler that supports just what you might expect.]

Comment 8 Christian Damus 2007-09-17 16:09:59 EDT

(In reply to comment #7)
> IProgressMonitor: org.eclipse.emf.common.util.Monitor is fine, probably better.
> Though I thought I checked on the usage of org.eclipse.core.runtime and found
> that it was already in use.

That's quite possible.  I should go back and check more widely for these dependencies  :-)


> Narrower Exceptions. For my 'development' I deliberately used new Exceptions to
> demonstrate that I was eliminating the dual use of
> ParserException/SemanticException - as a problem detection escape in inner
> routines, and as an outer reporting API. I eliminated the inner exceptions,
> introducing TerminateException when instant abort is required by the
> CompatibilityProblemHandler. TerminateException is caught and converted to the
> existing exceptions. These exceptions could all be broadened to match prior
> APIs, it's just the wider exception does not prohibit exception passing as
> problem handling.
> 
> Wider exceptions. Lookup errors are very rare, since the basic protocol is
> return something good, or null if nothing good. Exceptions only occur in the
> case of semantic impossibility; typically an inability to decide which of two
> good things to return. Philosophically this is true of all lookup methods. For
> the prior OCL code this only happened for state, and so this is the only place
> you see it happen, and so it looks to be narrower (but could be broadened). For
> QVT, classifiers can be looked up with missing package qualification, so an
> ambiguous lookupClassifier return is possible, hence the new exception. I think
> it is very desirable to use precisely LookupException for all lookups.

Agreed, yes.  These exceptions arguably should have been there from the start.  The problem is that they weren't, and it isn't easy to add them now that clients are already implementing these interfaces.

> BasicEnvironment was a late addition. Its content was originally added to
> Environment. Splitting it off drastically reduced the delta to
> AbstractEnvironment, gave much tidier layering and allowed me to inherit from
> AbstractBasicEnvironment rather than AbstractEnvironment for a non-OCL
> LPG-based parser/unparser (the KM3 Ecore syntax). Since it is a fattened base I
> don't think this causes any problems to existing code that inherits
> AbstractEnvironment. I'm sure it could be a mix-in, but is it worth it?

I like the BasicEnvironment, certainly.  The separation of concerns is tidy, even more so as a mix-in interface or a pluggable helper object.  It is worth it to mix it in because we cannot afford API-breaking changes.  Extending the AbstractEnvironment is a recommendation, but we have to assume that clients implement the Environment interface "de nouveau" for their own reasons.  Eclipse has a number of ways in which this kind of API evolution is generally handled.  I can come up with a revision that maintains compatibility, then perhaps you can have a look at that?  I have an idea for a generic EnvironmentAdapter that can handle this and future requirements by adding "facets" to the environment (actually, not my idea but an established pattern).

------8<------

> All clients should really be reviewing their problem handling
> behaviour because the CompatibilityProblemHandler is quite dreadful, and most
> of the OCL/Helper methods that rely on it are somewhat of a hindrance if you
> want to do something more sensible.

Yes, your improved problem reporting is definitely a plus.  The OCL and OCLHelper APIs are easier to expand because the rules set out for them are different  :-)

> OCLStandardLibraryUtil/TypeUtil extra/deprecated methods. Yeah no problem. I
> suspect that these and a few other areas may merit further revision when you
> consider them in the light of the new ProblemHander accumulates problem rather
> than aborting philosophy.

Yes, this is easy.  I just mentioned it for completeness, I guess (it stood out in the JUnit test delta).

> [If there's any plugin that can tolerate the Eclipse-specificness, I have a
> MarkerProblemHandler that supports just what you might expect.]

There's plenty of Eclipse-specific code in these plug-ins already.  The question is just whether the API requires it, or whether it is an adjunct that makes the API richer or easier to use.  The MarkerProblemHandler is something that a client will plug in to the parser in an Eclipse environment, but won't attempt to use otherwise.  That's not a problem.

Comment 9 Adolfo Sanchez-Barbudo Herrera 2007-09-18 10:05:25 EDT

Uaauuu !!!
What a lovely patch !!!!.

Having the EssentialOCL grammar in a separated file is a fantastic idea !!!. In my grammars i will not have to undefine rules with it !!!

Well, I have been dedicating some hours, examining the grammars and i have some comments to do:

OCLKWLexer.g
------------------------------------------------------------------------
$Define section:
	- $action_class macro is not used => should be deleted


LexerTemplateD.g
------------------------------------------------------------------------

$lex_stream_class could be a better name instead of $super_stream_class, so in lexer and parser grammar files we would have:
	- $lex_stream_class => AbstractLexer
	- $prs_stream_class => AbstractParser/AbstractOCLParser
Anyway $super_stream_class doesnt dislike me :P

Since the own template has a constructor with an environment, the initial comment of the template should suggest subclassing AbstractLexer
-- Macros that may be redefined in an instance of this template
--
--     $super/lex:)_stream_class -- subclass org.ocl.lpg.AbstractLexer

A constructor use the macro $ast_class => Change it to implicit $ast_type macro, in order to remove it from the macro definitions of the OCLLexer.g file

OCLLexer.g
------------------------------------------------------------------------

$Define section
	- remove $ast_class macro definition

EssentialOCL.g
------------------------------------------------------------------------
I dont know the reason of having DtParserTemplate removed, and have the generated code as a definition ($parseCode). The grammar file force to use a deterministicParser, when a backtracking cuould be a useful alternative. So, if you need to make changes because the code generated needs it, customize the template (as you do with the LexerTemplate). It Would be helpful, if we have the 2 versions, the deterministic template, and the backtracking template, so, it is a decision of the client which kind of parser (deterministic/backtracking) he want to use. (for example, i prefer the backtracking parser, for error recovery).

Advantages of using the template:
	- When extending a grammar, the options are not taken into account, so, all the options defined in the template doesnt need to be redefined. Besides, while EssentialOCL could be using a deterministParser (via dtParserTemplate) my new lenguage which extends EssentialOCL grammar, could use a backtrackingParser (vi btParserTemplate).
	- We have a cleaner grammar file: the related generated code, is in template file, not in the grammar file.

Some more notes:

$Define section
	- $prs_stream macro is not used.
	- $action_class macro is duplicated. Besides, it does need to be defined. The implicitt $action_type macro should be used instead.
	- about the macros $result,$getSym,$getToken, etc:
		1. They are dependant of a deterministic parser -> :\
		2. This macros should be in the each template (deterministic and backtracking). We will take a look in the determiniscTemplate:
			2 ways of defining this macros:
				1- Using the method: DetministicParser getParser() {return dtParser;} => $getSym /.getParser().getSym./
				2- Using the specific methods for kind of macro: (f.e.) Object getRhsSym(int i) { return dtParser.getSym(i);} => $getSym /.getRhsSym./
			The first way uses a not needed extra function call (getParser()). 
			The lpg original template use the first way, so in my grammars im redefining the macros in the second way, as follows
				$setResult /.setResult./
				$getSym /.getRhsSym./
				$getToken /.getRhsTokenIndex./
				$getIToken /.getRhsIToken./

$Header and $Trailer Sections
	with the correct use of the template both sections will dissapear.

$Rules section
	- When passing a IToken in the offset functions, getIToken($getToken(index)) would be better simplified as $getIToken macro.
	- When using the templates some fixes in some rules need to be done:
  	1. dotArrowExpCS ::= NUMERIC_OPERATION '(' argumentsCSopt ')'
		String text = getTokenText(dtParser.getToken(1)); => getTokenText($getToken(1));
	2. pathNameCS ::= pathNameCS '::' simpleNameCS
		result = extendPathNameCS(result, getTokenText($getToken(3)));
	3. operationCallExpCS ::= oclIsInState isMarkedPreCS '(' pathNameCSOpt ')'
		PathNameCS pathNameCS = (PathNameCS) dtParser.getSym(4); => PathNameCS pathNameCS = (PathNameCS) $getSym(4);
		(IsMarkedPreCS)dtParser.getSym(2) => (IsMarkedPreCS)$getSym(2)
							stateExpCS

I'll start to analyze the inclusion of an environment in the lexer, because without do it, i dont understand very much the necessity of having an environment when we do a sintactic analysis, Its really needed ?

In my implementation of QVT Operational Mapping parser, i have used the version 2 of the LPG library. In it, an interface IMessageHandler is included and the lexer(and parser) is able to handle the errors with a concret implementation of it.

Later, when i start with the semantic analysis or AST construction (when i think that the environment is really needed), the ASTBuilder (in this case the AbstractOCLAnalyzer) can easily get the IMessageHandler in order to continueing the process of the errors (semantic errors), so the environment doesnt need to manage the errors.  Anyway i will read again this bug activity because i remember that you talked about the advantages of managing the error inside the environments. I will give you a better conclusions about this theme, soon, when a i investigate a little more;).

I have some themes to talk about, for example:
    1. Getting error recovery (im mixing sintactic with semantic errors, in the editor). Having a good a AST, error recovery, error handler, etc, will be needed to support the probabale new Eclipse Project OCL-Tools (or someone which extends MDT-OCL project ;) )
    2. I not very pleased having AST related information inside the metamodel. I  have implemented a separated metamodel for the AST, as a decorator of the language metamodel. Some reasons for do it:
           - I have a cleaner language, without information related to nodes, visitors, etc.
           - I can have some concepts as nodes in the AST, that in the another way we can`t (without changing the language metamodel), for example because they  are EStructuralFeature in the language.
           - If i need to extend an metamodel (for example Ecore as i need in my language), i cant modify it to include this related information (offsets, visitable, etc). You are including some utilities for represent EClassifiers as TypedAstNode, but i think that it would be better, or at least smarter, having the concepts of the language and concepts of an AST in separated metamodels.     
    The problem is that i have to mantain 2 metamodels (QVT metamodel and QVT_AST metamodel) although very similar between them. Of course, it is easier to mantatin one mixed, but im not sure that it is the way to procces.
    What do you think, about this ?¿


I think that i have been writing too much ¬¬!!, let's go to work !!!!!

Cheers,
Adolfo

    

Comment 10 Christian Damus 2007-09-18 10:55:59 EDT

(In reply to comment #9)

You seem to be more adept with LPG than I am, that's for sure!  I can respond to some of your comments, and will let most of the grammar discussion percolate for a while.

> EssentialOCL.g
> ------------------------------------------------------------------------
> I dont know the reason of having DtParserTemplate removed, and have the
> generated code as a definition ($parseCode). The grammar file force to use a
> deterministicParser, when a backtracking cuould be a useful alternative. So, 

Aggreed.  Would be nice not to hard-code the dtParser, here.  This will require, as you mentioned also, cleanup of other usages of dtParser in the rules.


> I'll start to analyze the inclusion of an environment in the lexer, because
> without do it, i dont understand very much the necessity of having an
> environment when we do a sintactic analysis, Its really needed ?

The BasicEnvironment isn't really an Environment in the OCL sense, as it doesn't provide the function of relating OCL expressions to their model context.  Perhaps it should just be renamed to something more descriptive of what its purpose is:  ParseContext or some such? (I know, "context" is a little overloaded)


> In my implementation of QVT Operational Mapping parser, i have used the version
> 2 of the LPG library. In it, an interface IMessageHandler is included and the
> lexer(and parser) is able to handle the errors with a concret implementation of
> it.

That alone doesn't seem like a compelling reason to trudge through the IP process and other work to adopt another version of LPG.  Perhaps if other Modeling projects are already stepping up to version 2 and contributing it to Orbit ...

 
> Later, when i start with the semantic analysis or AST construction (when i
> think that the environment is really needed), the ASTBuilder (in this case the
> AbstractOCLAnalyzer) can easily get the IMessageHandler in order to continueing
> the process of the errors (semantic errors), so the environment doesnt need to
> manage the errors.  Anyway i will read again this bug activity because i
> remember that you talked about the advantages of managing the error inside the
> environments. I will give you a better conclusions about this theme, soon, when
> a i investigate a little more;).

One of the advantages of having error reporting in the environment is that certain utilities (e.g., TypeUtil) have only the environment as context, and need to report problems through it.  Using LPG 2, the environment would still have to provide an IMessageHandler, but the ProblemHandler seems to suffice.


> I have some themes to talk about, for example:
>     1. Getting error recovery (im mixing sintactic with semantic errors, in the
> editor). Having a good a AST, error recovery, error handler, etc, will be
> needed to support the probabale new Eclipse Project OCL-Tools (or someone which
> extends MDT-OCL project ;) )

Not probable.  OCL Tools is real!


>     2. I not very pleased having AST related information inside the metamodel.
> I  have implemented a separated metamodel for the AST, as a decorator of the
> language metamodel. Some reasons for do it:
>            - I have a cleaner language, without information related to nodes,
> visitors, etc.
>            - I can have some concepts as nodes in the AST, that in the another
> way we can`t (without changing the language metamodel), for example because
> they  are EStructuralFeature in the language.
>            - If i need to extend an metamodel (for example Ecore as i need in
> my language), i cant modify it to include this related information (offsets,
> visitable, etc). You are including some utilities for represent EClassifiers as
> TypedAstNode, but i think that it would be better, or at least smarter, having
> the concepts of the language and concepts of an AST in separated metamodels.    
>     The problem is that i have to mantain 2 metamodels (QVT metamodel and
> QVT_AST metamodel) although very similar between them. Of course, it is easier
> to mantatin one mixed, but im not sure that it is the way to procces.
>     What do you think, about this ?¿

The AST API is more concerned with being useful than with being a literal representation of the OCL metamodel.  The extras such as Visitable and ASTNode have no impact on the serialization and don't interfere with an implementation of standard XMI serialization for EssentialOCL or CompleteOCL.  It would be very difficult to change this now without severe API breakage.

I'm not sure that I understand the issue with including EStructuralFeatures and EClassifiers in the AST.  As far as I know, there would be no need to implement the ASTNode or Visitable interfaces on these ...

Comment 11 Adolfo Sanchez-Barbudo Herrera 2007-09-18 12:36:13 EDT

>You seem to be more adept with LPG than I am, that's for sure! 

Too much hours spent to be able to know how this f%%$&45$$ parser generator works ¬¬!!


> Not probable.  OCL Tools is real!

Bomb!!!!!

> 
> The AST API is more concerned with being useful than with being a literal
> representation of the OCL metamodel.  The extras such as Visitable and ASTNode
> have no impact on the serialization and don't interfere with an implementation
> of standard XMI serialization for EssentialOCL or CompleteOCL.  It would be
> very difficult to change this now without severe API breakage.

I supossed it. Anyway, i wanted to express you my point of view about having this "mixed" metamodel.

> I'm not sure that I understand the issue with including EStructuralFeatures and
> EClassifiers in the AST.  As far as I know, there would be no need to implement
> the ASTNode or Visitable interfaces on these ...

Well i' try to give you a more specific example. 

1. In my QVT Metamodel, i have an EClass called VarParamater, which simply is Variable and a parameter which have a DirectionKind (in/out/inout). This direction kind is an EDataType and can belong of VarParameter (as an EAtrribute of it). Well i could not have information about the offsets of a directionKind without modifying the language metamodel. What i could do is having an AST Metamodel in which i can include the concept of ASTDirectionKind, which will have information about offsets, and will have a reference to the DirectionKind EDataType.

Im not very sure, that this is the best way of implementation, but what i didnt want to do, is changing the language metamodel (Abstract Syntax Especification).

2. Look at this example. I can define metamodels in a transformation file. But what about ASTNode information related to definition of a Package, Class, etc... The instances of the output model, should be isntances of Ecore Metamodel, and of course im not going to change the metamodel to include this information. So what i have is a ASTPackage, ASTClass nodes in my ASTMetamodel. 

As i mentioned, i keep an ASTMetamodel which decorates the language Metamodel. All the facilities of a tool (Editor, viewers, etc), will keep attention over the AST model, while the output model will conform with a clean language metamodel.

Comment 12 Adolfo Sanchez-Barbudo Herrera 2007-09-18 12:41:43 EDT

Ouch!!!!
A lot of comments cut !!!! Ctrl + Z is our friend :D


> I can respond
>to some of your comments, and will let most of the grammar discussion percolate
>for a while.

No problem ;)

> Aggreed.  Would be nice not to hard-code the dtParser, here.  This will
> require, as you mentioned also, cleanup of other usages of dtParser in the
> rules.

Right. And i would be needed to have the posibility of working with the backtracking version ;P

> The BasicEnvironment isn't really an Environment in the OCL sense, as it
> doesn't provide the function of relating OCL expressions to their model
> context.  Perhaps it should just be renamed to something more descriptive of
> what its purpose is:  ParseContext or some such? (I know, "context" is a little
> overloaded)

Well, i need to take a look into the code, to know which is the objetive of that BasicEnvironment, and how is managed into the lexer/parser. Perhaps tomorrow could contribute with a better seeing ;)
 
> That alone doesn't seem like a compelling reason to trudge through the IP
> process and other work to adopt another version of LPG.  Perhaps if other
> Modeling projects are already stepping up to version 2 and contributing it to
> Orbit ...

Well, there are is not the lonely feature, I try to remember that it had a better design, with property interfaces, a new class for recovery (RecoveryParser as addition of DiagnoseParser), etc... Anyway what i wanted to explain, is how the error management was done in that version. As i mentioned before, i need to know how is treated in the patch.

> One of the advantages of having error reporting in the environment is that
> certain utilities (e.g., TypeUtil) have only the environment as context, and
> need to report problems through it.  Using LPG 2, the environment would still
> have to provide an IMessageHandler, but the ProblemHandler seems to suffice.

Ok, i will take into account this.

Comment 13 Ed Willink 2007-09-18 13:37:55 EDT

Christian: I'm happy to play ping-pong with edits. I figure it's up to you to make a significant improvement to support EnvironmentAdapters. Sounds like a good idea.

'Basic'Environment. I just wanted something fundamental like Abstract. EMOF uses Basic so I copied it. If you manage to create per-concern adapters, I'm sure this will vanish. Until BasicEnvironment is an adapted concern it is the implementation environment that is needed to support the OMG compatible OCL Environment, so BasicEnvironment seems a reasonable name.

Adolpho: I have a full QVT model, or rather many of them. I seem to be the only person who's fixed the Rose to Ecore importer so that it can accurately convert the QVT Rose file, so it is likely that my coherent MDL/Ecore/EMOF will be the final QVT courtesy release. From these I can genmodel a pure OMG compatible model. I prefer to use Ecore for tooling, so I have an independently edited but moderately well auto-correlated EQVT model in which Ecore, MDT-OCL replace EMOF, EssentialOCL. I have adapters for the EQVT model that allow loading and saving of OMG-compliant EMOF extensions. I have LPG based parsers/unparsers/editors for QVTCore and QVTRelation that motivated the OCL refactoring. You are welcome to have all these. (The current CVS content of org.eclipse.gmt.umlx plugins is slightly dated because I don't want to update stuff that depends on missing OCL stuff, and I don't want to create a temporary rival OCL.) See http://dev.eclipse.org/viewcvs/indextech.cgi/gmt-home/subprojects/UMLX/doc/MDD-TIF07/MDD-TIF07-QVTEditors.pdf.

I go from text to keywords to tokens to QVTCST model to QVT model. The QVT serves as an AST model since it is structurally identical. The missing 'AST' context is obtainable from the AST to typically CST map in the BasicEnvironment. (I put in a fair amount of AST population in the OCL Analyzer, but there is probably some nodes that are missing or too general. I tried to ensure that AST to CST mapping could not be overlooked, but it required unacceptable API change, so it's a manual obligation just the same as filling in the token contexts in the parser.

For my QVT parsers the 'ASTNode' Visitable capability of MDT-OCL is not used. The unparsers could be structured as visitors, so I started using EMF Switches but found that the hierarchical traversal and use of children was so regular but also bespoke that it was easier to hand code the traversal. I suspect that there is a distinction between OCL where nearly everything is polymorphically an OclExpression, and QVT where there are far more irregular structuring elements.

I have seen LPG papers claiming that CST parsing should be 100% automatic. Having hand coded too much of the QVT CST code, I can only agree. If I didn't have far too many other things to do I might automate this myself. If/when LPG advances from CST support to AST support, I would recommend a further restructuring of the MDT OCL parsers to remove gratuitously manual code. Till then the new structure separating Analyzer and Parser with a coherent ProblemHandler is a big improvement.

Details such as $action_class and DeterministicParser are I'm sure worth sorting out, but let's get this significant refactoring into CVS, then sort out the comparative trivia. I endeavoured to change very little of the LPG structure, partly because I didn't understand it or know who might want to use any of the other 4 bloated constructors. It would be good for an LPG expert to perform a prune of stuff that is really not appropriate. I recommend holding back on intra-file improvements until the inter-file names and APIs stabilise.

Comment 14 Christian Damus 2007-09-18 14:09:48 EDT

(In reply to comment #13)
> Christian: I'm happy to play ping-pong with edits. I figure it's up to you to
> make a significant improvement to support EnvironmentAdapters. Sounds like a
> good idea.

Agreed.  So, I can run your patch through the Eclipse IP process, then commit it with a few changes for API compatibility.  After that, we can continue with incremental patches; otherwise, it's very difficult to patch a patch.  Only once we're reasonably satisfied that it is stable would we release these changes into a 1.2 stream build.

Comment 15 Adolfo Sanchez-Barbudo Herrera 2007-09-19 08:13:10 EDT

> Adolpho: I have a full QVT model, or rather many of them. I seem to be the only
> person who's fixed the Rose to Ecore importer so that it can accurately convert
> the QVT Rose file, so it is likely that my coherent MDL/Ecore/EMOF will be the
> final QVT courtesy release. From these I can genmodel a pure OMG compatible
> model. I prefer to use Ecore for tooling, so I have an independently edited but
> moderately well auto-correlated EQVT model in which Ecore, MDT-OCL replace
> EMOF, EssentialOCL. I have adapters for the EQVT model that allow loading and
> saving of OMG-compliant EMOF extensions.

Great, i have an implementation of a QVT metamodel based on Ecore and MDT-OCL too. An it is updated with the last especification changes released 2 months ago, At least the epackages related to Operational Mapping and Relations (which the former depends on). The only modification that i needed in order to take advantage of the MDT-OCL implementation, is that some concepts (ModelType, OperationalTransformation, etc) needs to extend OCL PredefinedType. Relations or Core doesnt specify any predefined type or standard library so you should not have had this kind of problems.

> I have LPG based
> parsers/unparsers/editors for QVTCore and QVTRelation that motivated the OCL
> refactoring. You are welcome to have all these. (The current CVS content of
> org.eclipse.gmt.umlx plugins is slightly dated because I don't want to update
> stuff that depends on missing OCL stuff, and I don't want to create a temporary
> rival OCL.) See
> http://dev.eclipse.org/viewcvs/indextech.cgi/gmt-home/subprojects/UMLX/doc/MDD-TIF07/MDD-TIF07-QVTEditors.pdf.

 I had taken a quick look to your implementation at the beginning of the year. I will take a look a again, i'll give you some comments if you are pleased.

And i have my own MDT-OCL project modifications so im very interested in this kind of patches in order to get completely aligned with the project. I have some more pendant patches with Christian, related to the standard library for example, but i'll probably create a new patch when this fantastic patch is finally applied (Sorry Christian for my initial contribution patch for the OCL Standard Library, but as you may see im a complete noob in this kind of tasks, i'll send you a new one, that i'll configure when the ed's work is set in the CVS).

I'll take a look to that PDF ;).

> I go from text to keywords to tokens to QVTCST model to QVT model. The QVT
> serves as an AST model since it is structurally identical. The missing 'AST'
> context is obtainable from the AST to typically CST map in the
> BasicEnvironment. (I put in a fair amount of AST population in the OCL
> Analyzer, but there is probably some nodes that are missing or too general. I
> tried to ensure that AST to CST mapping could not be overlooked, but it
> required unacceptable API change, so it's a manual obligation just the same as
> filling in the token contexts in the parser.

Well im agreed with you when you say that an AST model is structurally identical with the Abstract Syntax of a language. But in my point of view, this has not to be "a must". Besides, independently of the discussion of how an AST must be, i think that is not a good idea mixing the semantic of a AST with the semantic of the language. Look at this piece of valid code of a operational mapping transformation file:

metamodel BOOK {
    class Book {title: String; composes chapters: Chapter [*];}
    class Chapter {title : String; nbPages : Integer;}
}

They are all concepts of emof, and in the output model shoud be taken as Ecore (with my implementation based in EMF) instances. How should the text-cst-ast process be? 

¿metamodel BOOK{....} => CSTPackage => EPackage ? No thanks. I need the concept of ASTPackage node, which will decorate the EPackage language concept.

Another point of view. Have you tried to create  manually a model instance of the OCLEcore.ecore metamodel ?. Why should i know about offsets or anything that is not concerned of the OCL abstract syntax?.

Anyway, although the discussion it would be very interesting, it is not important since it would be a too API-breaking (¿?) ;P. As far as i can (ImperativeOCL needs to have "mixed" metamodel), my parser distinct between AST (Abstract Syntax Tree) and Language AS (Abstract Syntax), so the chain is text => cst => ast => as (AST model and AS model is computed in the CST visit at the same time, as consequence that the former is a decorator of the later).

> For my QVT parsers the 'ASTNode' Visitable capability of MDT-OCL is not used.
> The unparsers could be structured as visitors, so I started using EMF Switches
> but found that the hierarchical traversal and use of children was so regular
> but also bespoke that it was easier to hand code the traversal. I suspect that
> there is a distinction between OCL where nearly everything is polymorphically
> an OclExpression, and QVT where there are far more irregular structuring
> elements.
> 
> I have seen LPG papers claiming that CST parsing should be 100% automatic.
> Having hand coded too much of the QVT CST code, I can only agree. If I didn't
> have far too many other things to do I might automate this myself. If/when LPG
> advances from CST support to AST support, I would recommend a further
> restructuring of the MDT OCL parsers to remove gratuitously manual code. Till
> then the new structure separating Analyzer and Parser with a coherent
> ProblemHandler is a big improvement.

Agree. I have a pendant researching line, envolving this kind of tasks (generate CST metamodel,  rules to build it for a especific parser generator, etc )

> Details such as $action_class and DeterministicParser are I'm sure worth
> sorting out, but let's get this significant refactoring into CVS, then sort out
> the comparative trivia. I endeavoured to change very little of the LPG
> structure, partly because I didn't understand it or know who might want to use
> any of the other 4 bloated constructors. It would be good for an LPG expert to
> perform a prune of stuff that is really not appropriate. I recommend holding
> back on intra-file improvements until the inter-file names and APIs stabilise.

Im agree that some changes of the grammar files are not very important, but the  posibility of using the backtracking parser is important to me. Have you resolved the problem when you have a sintatic error in the file. The diagnose parser is invoked in order to find more sintatic error, and it is a fantastic diagnose, but the CST is not build :\. This is not util for me. A good IDE for transformation definitions needs always a syntax tree, the parser needs a good error recovery strategy, etc.

Have you solved this with LPG's deterministicParser and a diagnoseParser ?¿ I would be very pleased to know how :).

About the patch, im very very happy with it. I will start to adopt it, and i ill comment any problem i encounter or note that i should add. I'll have to change my actual error handling, and back to LPGv1.1 but i think the internal refactoring is worth it!!! ;P

About the grammar files changes that i have commented (use of templates, files cleaning). I could make them in a later patch, once this one is applied ;)

Cheers !!!!!!
Adolfo 

Comment 16 Adolfo Sanchez-Barbudo Herrera 2007-09-19 11:38:03 EDT

A little detail of decoupling:

I was very happy with the EssentialOCL grammar :).
Why dont you do the same distinction with the AbstractParser and Analyzer. We dont need to deal with methods such as createPackageDeclaration() if we are only extending the EssentialOCL language ;P.

Am i going too far if i suggest making the same distinction in the metamodel?¿ :\. Sure i am ¬¬!!

Greetings!!!!

Adolfo.

Comment 17 Ed Willink 2007-09-20 03:49:53 EDT

I don't think that it is sensible for OCL to provide an AST representation that is bloated by every possible requirement of every possible language tool.

Text->Keywords->Token->CST->Minimal AST works well as a parsing process.

The Minimal AST is the XMI interchange standardised by OMG. For some tools this is an input and so the necessary starting point.

Therefore a tool wanting to work with a Rich AST must be able to create it from a Minimal AST. The AST mapping capability of AbstractBasicEnvironment provides an opportunity for the CST->Minimal AST to be elaborated to preserve richness from the CST.

EMF provides adapters, so you might consider adding your richness as adapters on the Minimal AST objects. I found that stateless adapters provided a magic solution to providing an OMG EMOF compatible view of an Ecore-based AST. It is not necessarily the most efficient solution, but it modularises concerns, and avoids maintaining an additional model, where the richer model is structurally very similar to the minimal model.

A set of adapters for cached compiler analysis would seem like a tidy solution to how do you add information to an Ecore model.

From a different perspective, I think you will find it very hard to polymorphise the AbstractOCLAnalyzer to support generation of an arbitrary AST model that does not re-implement the existing model interfaces. A rewrite is in order - at least you can keep the parser, and the CST is no longer internal.

The proposed refactoring does not change the CST or 'AST' models. A proposal for change of these should probably be a separate Bugzilla.  

----

Partitioning AbstractOCLAnalyzer into AbstractOCLAnalyzer and AbstractEssentialOCLAnalyzer would be tidier. With only a few State and Message methods to extract, I'm not sure that the bloat is too serious. Partitioning the grammar was much more important since bloated grammar impacts the generated parser, though even there most of the bloat was discarded as unused rules - it just gave excessive diagnostics. 

Christian persuaded me to call EssentialOCLParser.g just EssentialOCL.g. I think that this was a mistake, since OCLLexer.g should really have an EssentialOCLLexer.g split off so that CARETs and the like are not imposed on EssentialOCL re-users. Similarly an EssentialOCLKWLexer.g could avoid an EssentialOCL re-use having to repair a few spurious keyword creations.

----

If you can create a CST after a syntactic failure that would be good. I find it a bit confusing to edit a file and think there are only a couple of errors, fix the syntax and suddenly then have numerous semantic errors. But this is definitely an orthogonal improvement worth discussing in a separate Bugzilla. Let's get the refactoring sorted, which presents some API challenges, then improve some performance corners that could be simple local upgrades.

Comment 18 Adolfo Sanchez-Barbudo Herrera 2007-09-20 04:40:29 EDT

(In reply to comment #17)
> 
> EMF provides adapters, so you might consider adding your richness as adapters
> on the Minimal AST objects. I found that stateless adapters provided a magic
> solution to providing an OMG EMOF compatible view of an Ecore-based AST. It is
> not necessarily the most efficient solution, but it modularises concerns, and
> avoids maintaining an additional model, where the richer model is structurally
> very similar to the minimal model.
> 
> A set of adapters for cached compiler analysis would seem like a tidy solution
> to how do you add information to an Ecore model.

I'll study what you are talking about. It could be a good and sufficient solution for me.

As you may imagine i dislike having to mantain two metamodels. But i dislike more, having to add information about trees (or whatever) to a language metamodel.

> From a different perspective, I think you will find it very hard to
> polymorphise the AbstractOCLAnalyzer to support generation of an arbitrary AST
> model that does not re-implement the existing model interfaces. A rewrite is in
> order - at least you can keep the parser, and the CST is no longer internal.
> 
> The proposed refactoring does not change the CST or 'AST' models. A proposal
> for change of these should probably be a separate Bugzilla.  

Since the MDT-OCL metamodel include information about ASTs, i dont need to create MetaClasses about it in my AST Metamodel. So the behavior when my parser deal with the "ocl part" is the usual.

> ----
> 
> Partitioning AbstractOCLAnalyzer into AbstractOCLAnalyzer and
> AbstractEssentialOCLAnalyzer would be tidier. With only a few State and Message
> methods to extract, I'm not sure that the bloat is too serious. Partitioning
> the grammar was much more important since bloated grammar impacts the generated
> parser, though even there most of the bloat was discarded as unused rules - it
> just gave excessive diagnostics. 
> 
> Christian persuaded me to call EssentialOCLParser.g just EssentialOCL.g. I
> think that this was a mistake, since OCLLexer.g should really have an
> EssentialOCLLexer.g split off so that CARETs and the like are not imposed on
> EssentialOCL re-users. Similarly an EssentialOCLKWLexer.g could avoid an
> EssentialOCL re-use having to repair a few spurious keyword creations.

Well, as you say it is not a need. That methods in AbstractOCLParser and AbstractOCLAnalyzer doesnt bother me, but i think that they are easily decoupled. And im agreed with you, that the EssentialOCL should have their own Lexer and KWLexer files (and metamodel ¬¬!! )

Its not a crazy idea that OCLParser should be an extension of EssentialOCLParser, as same as QVTRelation or QVTOperationalMappings are extensions of it.

> ----
> 
> If you can create a CST after a syntactic failure that would be good. I find it
> a bit confusing to edit a file and think there are only a couple of errors, fix
> the syntax and suddenly then have numerous semantic errors. But this is
> definitely an orthogonal improvement worth discussing in a separate Bugzilla.
> Let's get the refactoring sorted, which presents some API challenges, then
> improve some performance corners that could be simple local upgrades.
> 

Agreed, we could talk about this theme in the future.

I'll continue aligning my code with the patch.

Greetings

Comment 19 Christian Damus 2007-09-20 10:05:50 EDT

We should be careful to keep the focus of this bugzilla to its original purpose, which is refactoring the grammar to support the implementation of QVT as an extension of MDT OCL, in particular, targeting the QVT implementations in development at Eclipse.

Other API changes, as good as they may be, will have to be analyzed separately and considering the mandate of this component which (as I understand it) is primarily to provide an OCL Parser/Interpreter for Ecore and UML models.  Especially for this release, let's not assume too much.

Comment 20 Adolfo Sanchez-Barbudo Herrera 2007-09-21 11:41:40 EDT

Some comments about adopting the patch :

(firstly, I must mention that i hve not run my parser, because i have to reimplement my own changes made to the OCLStandardLibrary infrastructure)

I have easily adopted the error handling management. While my QVToErrorHandler implemented IMessageHandler (LPG v2), now it will implement ProblemHandler and it will extend the abstract implementation, and i suppose that it should work as it used to (not run yet).

I have found some minor issues trying to solve syntatic sugar (shortcuts). Example of syntatic sugar:

mapping aContext::aMappingName () {

   anAttribute := newVaule;
}

corresponds to

mapping aContext::aMappingName () {

   population {
      object self : aContext {
         self.anAttribute := newValue;
   }
}

My processing in this kind of resolution: while building the AST i created the implicit CSTNodes  that a shortcut corresponds to. But now, in the astBuilder (analyzer) i hv not access to the protected methods of the cstBuilder (parser). 2 alternatives:
1. In my QVTOperationalParser, i could create public methods in order to access to the protected ones (for example, myCreateSimpleNameCS(...) ).
2. Trying to build the AST without creating implicit CSTNodes.

Anyway, the patch has been easily adopted. As soon as i finish to correct the errors i have (when i add my ocl-project modifications), i'll comment you the results.

Regards,
Adolfo.

Comment 21 Christian Damus 2007-09-21 11:58:24 EDT

Adolfo, your results are encouraging!  This is exactly the kind of validation that I need to be confident that this contribution is good for extensibility generally.

I would recommend against creating "fake" CST nodes, because your concrete syntax actually doesn't have them.  It could only confuse clients of the CST-AST mapping such as refactoring participants that will think, in looking at the CST, that there is text to be updated that doesn't actually exist or is in a different location.

Comment 22 Adolfo Sanchez-Barbudo Herrera 2007-09-21 14:07:48 EDT

(In reply to comment #21)
> Adolfo, your results are encouraging!  This is exactly the kind of validation
> that I need to be confident that this contribution is good for extensibility
> generally.

I prefer to be cautious. I need to know if my editor continue marking errors and having the correct QVTOperational output model, or not. Anyway, i continue thinking that this patch is a BIG step for extension of the OCL language :).

As soon as this patch is applied, i'll contribute with my OCL-Project modifications left, in order to be completely aligned with the project (all related to OCLStandardLibrary and the TypeResolver pendant enhencement). When it is applied, i'll give you "decent" attachment with the changes :), you know ;P.

> I would recommend against creating "fake" CST nodes, because your concrete
> syntax actually doesn't have them.  It could only confuse clients of the
> CST-AST mapping such as refactoring participants that will think, in looking at
> the CST, that there is text to be updated that doesn't actually exist or is in
> a different location.

well, firstly this "fake" nodes are not fake at all, because they are a reality in the CST Metamodel, i mean, they are part of concret syntax.

Secondly, this "fake" nodes are not created in the CST construction, they are temporally created while building the AST in order to resolve the typical syntatic sugar problems, and avoid having several AST creation methods related to the obtainment of the same ASTNode. So this "fake" nodes, dont belong to anything, they are merely temporal artifacts.

I have a unique cst2ast mapping (where i have just needed it) in a operationalTransformation environment, in order to resolve the imperativeOperations declaration and defitinition processing problem.

So, actually, i dont have a complete mapping between the CST and AST instances because i dont have needed it yet (perhaps in a future with an unparser as Ed has ?¿?¿?¿).

I expect to finish the adoption (run successfully the parser :) ) on next monday.

Cheers, and have a good weekend ;),
Adolfo.

Comment 23 Ed Willink 2007-09-22 03:39:48 EDT

Re #20, #21. Indeed, the CST is a tree of the Concrete Syntax. If the AST needs to be richer then that is part of the AST build. The whole reason for going from text to AST via a CST at all is to partition the problems. Text to CST deals,  potentially automatically, with textual difficulties. CST to AST deals with the language translation: how loose text finally becomes semantically meaningful structure.

Comment 24 Adolfo Sanchez-Barbudo Herrera 2007-09-26 10:16:35 EDT

Well, i could say that i have finally adopted the patch (not with some problems as you may imagine ;P).

I only need to investigate a little more about the error handler, because the errors are not propertly marked (wrong locations) in the editor. I dont understand what is the reason, since the mechanism only need a CSTNode to get the offsets, ant it's what im passing to the ERROR methods... Anyway it should be some own failure.

Some notes:

Why is the reason of having a constructor in lexer,parser where they only receive a basicEnvironment?

I was creating my lexer/parser where the constructors receives a string with the full path of file so:
    - I needed to create a constructor method in the abstractLexer:

public AbstractLexer(BasicEnvironment environment, String filename) throws java.io.IOException {
		super(filename);
		this.environment = environment;
		
}
    - I needed to make some changes in the LexerTamplateD.g (uncomment and modify a commented method).

public QVTOperationalLexer(Environment<?,?,?,?,?,?,?,?,?,?,?,?> environment, String filename) throws java.io.IOException {
         super (environment, filename);
         this.kwLexer = new QVTOperationalKWLexer(getInputChars(), TK_IDENTIFIER);
     }

As i commented some commentes above, in my QVToperationalParser im overwriting some methods of AbstractOCLParser, with the only intention of they are available by the QVTOperationalAnalyzer.

A issue related to a old discussion about TypeExp that perhaps Christian could remember. I dont want to deal with OclType as the type of a TypeExp, so i prefer to deal with the metaclass of the referredType (to compatibilize with some operations over types defined in the Operational Mappings Standard Library). I need to overwrite the OCLAnalyzer - TypeExp<C> typeCS method, which actually im able to overwrite, but there is a problem in the OCLExpression<C> simpleNameCS method, which has a hand-coded creation of this TypeExp.

So in the line 2059 (AbstractOCLAnalyzer):
Would be better placed the next code:

TypeExp<C> texp = typeCS(simpleNameCS, env, classifier);
astNode = texp;

When patch is applied, i'll work in the modifications needed to have the possibility of using the LPG backtrackingParser.

P.D: I have been had some problem with consecutive run of the parser, but i used some reinitialization code provided in LPG v2, so i'll have to investigate to make some changes and make it work propertly.

Comment 25 Ed Willink 2007-09-27 03:42:59 EDT

The construction policy is historic.

The original (default LPG) constructors provide about 5 different simple options that seem of limited use, and I pruned what I dared.

The old merged AST/CST OCL parser constructor performed two of the four conversions during construction; perverse either do all or none.

The new constructors with just environment allows an environment to be constructed and then used in a controlled fashion to progress parsing.

For my usage I construct an environment then supply its input from perhaps a Stream via a Reader that returns the char[] that the lexer uses. The input is not necessarily associated with a file.

I found the numerous LexStream constructors hard to understand and so tried to hide them. If your sole requirement is to pass the filename, then why not just use setFileName() and/or setTab()? Since the filename is solely for diagnostic purposes and diagnostics are handled by a ProblemHandler surely the filename is redundant?

 

Comment 26 Adolfo Sanchez-Barbudo Herrera 2007-09-27 05:16:57 EDT

Lexer's constructor with the input file, will propertly initialize the lexer: creates an inputStreamReader and obtain the char[] buffer, and initializes the lexer with it (setInputChars, setStreamLength, setInputFile, computeLineOffsets).

As you say, there is several ways of creating the lexer, you can create and pass the inputChars, i prefer passing the location of the file and delegate this labour to the Lpg classes (LexStream class, if im more specific).

A little modification in order to use the locally defined ECLIPSE_TAB_VALUE instead of the LPG Default One:

public AbstractLexer(BasicEnvironment environment, String filename) throws java.io.IOException {
	this(environment, filename, ECLIPSE_TAB_VALUE);		
}

P.D: AbstractLexer(BasicEnvironment environment, String filename, int tab) is already defined ;P.

Another comment: Im not sure that overwriting setInputChars is the best practice, since this method has its own purpose inside LPG LexStream. LPG has defined methods called initialize, which should be used to initialize (and reinitialize) the lexer. Perhaps you could add a new one which only receives a char[] inputChars, and do the labours what you are doing overwriting setInputChars. So i'd remove the setInputChars method, i'd add the following one

public void initialize(char[] inputChars) {
    setInputChars(inputChars);
    setStreamLength(inputChars.length);
    computeLineOffsets();
}

And dont forget to change the setInputReader method

public void setInputReader(Reader reader) throws IOException {
    	char[] buffer = getInputChars(reader);
    	initialize(buffer);
}

Of course I have my initialize(String filename) defined in my QVTOperationalLexer.g header section for my usage. Perhaps it could be included as a initializing (and reinitializing ;) ) method in the AbstractLexer. 
As additional note, the last version of lpg published one week ago, this method (initialized(String filename)  )has beeen included as part of the set of initialize methods of the LexStream class.

Regards,
Adolfo

Comment 27 Ed Willink 2007-09-28 02:47:23 EDT

Yes, the override is horrible. setInputChars is a poke method; initialize is a tool method.

As you wrote AbstractLexer.setInputChars should be reimplemented as initialize
to avoid the override

	public void initialize(char[] inputChars) {
	  setInputChars(inputChars);
          setStreamLength(inputChars.length);
          computeLineOffsets();        
	}

AbstractLexer.setInputReader should be renamed initialize
and invoke initialize(char[]).

AbstractAnalyzer.setInputChars should be renamed initialize
and invoke AbstractLexer.initialize(char[]).

AbstractAnalyzer.setInputReader should be renamed initialize
and invoke AbstractLexer.initialize(Reader).

Not so sure about the last two names; perhaps initializeInput.

A further AbstractLexer.initialize(String filename) would
presumably emulate the I/O constructor.

    public void initialize(String fileName) throws IOException
    {
        try
        {
            File f = new File(fileName);
            InputStreamReader in = new InputStreamReader(new FileInputStream(f));
            char[] buffer = new char[(int) f.length()];
            in.read(buffer, 0, buffer.length);
            initialize(buffer);
            setFileName(fileName);
        }
        catch (Exception e)
        {
            IOException io = new IOException();
            System.err.println(e.getMessage());
            e.printStackTrace();
            throw(io);
        }
    }

Comment 28 Adolfo Sanchez-Barbudo Herrera 2007-09-28 05:15:36 EDT

(In reply to comment #27)
> Yes, the override is horrible. setInputChars is a poke method; initialize is a
> tool method.
> Not so sure about the last two names; perhaps initializeInput.

Yes, perhaps in order to follow the name convention taken by LPG, would be better to use the "initialize" word in order to do this tasks, in lexer, parser, etc.

> A further AbstractLexer.initialize(String filename) would
> presumably emulate the I/O constructor.
> 
>     public void initialize(String fileName) throws IOException
>     {
>         try
>         {
>             File f = new File(fileName);
>             InputStreamReader in = new InputStreamReader(new
> FileInputStream(f));
>             char[] buffer = new char[(int) f.length()];
>             in.read(buffer, 0, buffer.length);
>             initialize(buffer);
>             setFileName(fileName);
>         }
>         catch (Exception e)
>         {
>             IOException io = new IOException();
>             System.err.println(e.getMessage());
>             e.printStackTrace();
>             throw(io);
>         }
>     }
> 

Sure it will be ;). With the only detail that i use the initialize(char[] buffer, String filename), but its the same ;P.

Comment 29 Christian Damus 2007-09-29 12:51:53 EDT

Created attachment 79441 [details]
Updated patch for API compatibility and other changes

Attached a ZIPped up variant of the original patch, with the following changes:

API Compatibility
-------------

Restored old signatures (without the problemObject argument and/or throwing SemanticException) of public utility methods in TypeUtil and OCLStandardLibraryUtil.  Old methods are deprecated.

OCL Environment interface no longer extends BasicEnvironment (though the AbstractEnvironment impl still does).  Introduced Adaptable interface that provides Environment adaptation to/from BasicEnvironment.  New OCLUtil::getAdapter(...) methods adapt an environment to optional adapter interfaces.

Removed new thrown exceptions from lookupXyz() methods on Environment and EnvironmentFactory interface.  Refactored these throwing signatures as optional "Lookup" adapter interfaces (OCLUtil provides a default adapter if the environment doesn't provide one).


Problem Handling
--------------

Replaced the IProgressMonitor used by ProblemHandler interface with emf.common.util.Monitor, which provides integration with Eclipse IProgressMonitor.

Replaced the CompatibilityProblemHandler and TerminateException class with an OCLProblemHandler which gathers problems in a Diagnostic instead of throwing on the first problem.  OCL and OCLHelper parsing and validation operations now use an OCLProblemHandler by default and check for diagnostics, throwing ParserException on error severity or worse.

ParserException now includes a Diagnostic carrying one or more problems.

LookupException optionally includes a list of matches in case of ambiguous match.

Replaced non-localized string constants in ProblemHandler with interface with Severity and Phase enums, having localized names for inclusion in error messages.


Other Changes
------------

LPG is now part of the public OCL API.  Re-exported it in the OSGi manifest.

Added Option interface and parsing options API to BasicEnvironment, for clients to specify options for custom parsing behaviour.  Implemented options in OCL parser to let clients choose severity of problems reported when expressions use non-standard constructs such as the closure iterator.  Added JUnit tests for these options.

Updated "MDT OCL Tests - Stand-Alone Mode" launch configuration in the org.eclipse.ocl.standalone.tests plug-in, which is an Ant-based JUnit test suite covering all of the other test projects that runs in an Eclipse-free classpath.  All tests pass.  :-)

I will add, later, compatibility tests to verify that environments that do not not adapt to BasicEnvironment still function with the default adapters provided by OCLUtil.

Comment 30 Christian Damus 2007-09-29 12:57:12 EDT

I almost forgot -- the updated patch (comment #29) also addresses Adolfo's concerns about TypeExp and lexer initialization, following the recommendations in comment #24, comment #26, and comment #27.

Comment 31 Ed Willink 2007-09-30 05:23:12 EDT

New patch is considerably better.

Minor installation problems:

needed to allow 2.3.0 on org.eclipse.emf.ecore import to org.eclipse.ocl.ecore 
and 2.1.0 on org.eclipse.uml2.uml import to org.eclipse.ocl.uml.

Trivial issues:

ProblemHandler.Phase/Severity classes give much better signatures than my Strings but
an enum is it not extensible. I guess they need to be true classes.

ProblemHandler.Phase.VALIDATION: all other names are tools rather than processeses, so perhaps VALIDATOR better.

AbstractAnalyzer.getEnvironment() returns a BasicEnvironment which is not an Environment.
AbstractAnalyzer.getOCLEnvironment() returns the Environment. Change AbstractAnalyzer.getEnvironment()
to AbstractAnalyzer.getBasicEnvironment().  

A few (~15) dead imports.

Give lookupException a simple arrayof constructor. I search for matches and get a list/array. There is
no particular benefit in trimming off one as a special argument.

Significant issue:

getAdapter() works nicely for decoupling ProblemHandler. It doesn't really help for LookupException. The support incurs non-trivial complexity and an additional non-adapting adapter object for all clients. It successfully supports throwing an 'unsupported' exception, but provides no assistance to catching it. LookupException might as well be declared a RuntimeException, since all practical type checking has been lost; existing signatures would then not be a problem.

It is not possible to create a derived environment that implements both Environment and Environment.Lookup, since the derived implementation with throw is incompatible with the also-derived implementation without.

Given the need to preserve the existing signatures, there seems no alternative to introducing new methods with throws. In my derived environment I just added tryLookupClassifier and tryLookupPackage, with adapters to exploit them and a final @Deprecated on the old methods. The caller now gets to see the LookupException if using the new methods or a Deprecated if not. My tests pass. I didn't need to edit the patch. I just had to create silly adapters.

Therefore either:

Restore Lookup as an integral facility, by eliminating the Lookup interafces, adding at least tryLookupClassifier, tryLookupPackage and tryLookupState to Environment, and tryCreatePackageContext to EnvironmentFactory, then delegating directly to these methods from the traditional signatures with a backstop catch. Perhaps mark at least lookupState as deprecated.

or:

Make Lookup a fully extensible facility and put the full set of tryLookupXXX, tryCreateXXX in the Lookup interface, and delegate directly to these methods possibly via instanceof for mix-in and/or via adaption.

Comment 32 Ed Willink 2007-09-30 06:47:42 EDT

Oops. Forgot to rerun LPG.

EssentialOCL.g should not import or throw ParserException.

Many of your unit tests could probably lose ParserESception too.

Comment 33 Christian Damus 2007-10-01 09:19:56 EDT

(In reply to comment #31)

Thanks, Ed, for taking a look at this so soon.  Some replies in-line, below.

> New patch is considerably better.

Great!  I'm glad that it looks like a good direction.

> Minor installation problems:
> 
> needed to allow 2.3.0 on org.eclipse.emf.ecore import to org.eclipse.ocl.ecore 
> and 2.1.0 on org.eclipse.uml2.uml import to org.eclipse.ocl.uml.

This is picking up recent changes on the CVS HEAD (OCL 1.2).  Since EMF and UML2 on the Ganymede release have already updated their metamodel plug-ins to the next minor version, OCL has to adapt its narrow tolerance.  The problem for OCL is that we are extending the "internal" (formally such in the UML2 case) implementations of the Ecore and OCL metamodels, so we have to keep our version numbers in locked step and regenerate concurrently with our dependencies.


> Trivial issues:
> 
> ProblemHandler.Phase/Severity classes give much better signatures than my
> Strings but
> an enum is it not extensible. I guess they need to be true classes.

Do they need to be extensible?  Severity maps to the platform-supported concepts that aren't extensible.  Do specializations anticipate adding more phases?  If so, changing these to classes is trivial; they don't really need to be enumerations.


> ProblemHandler.Phase.VALIDATION: all other names are tools rather than
> processeses, so perhaps VALIDATOR better.

Sounds good.

> AbstractAnalyzer.getEnvironment() returns a BasicEnvironment which is not an
> Environment.
> AbstractAnalyzer.getOCLEnvironment() returns the Environment. Change
> AbstractAnalyzer.getEnvironment()
> to AbstractAnalyzer.getBasicEnvironment().  

Sounds good.  I was just trying to avoid changing the name that you were already using, but if you don't mind, then this is better  :-)

> A few (~15) dead imports.

Yeah ... I was planning to clean those up on commit.


> Give lookupException a simple arrayof constructor. I search for matches and get
> a list/array. There is
> no particular benefit in trimming off one as a special argument.

Yes, I guess an impl would normally collect these in some sort of collection anyway.  Makes sense.

> Significant issue:
> 
> getAdapter() works nicely for decoupling ProblemHandler. It doesn't really help
> for LookupException. The support incurs non-trivial complexity and an
> additional non-adapting adapter object for all clients. It successfully
> supports throwing an 'unsupported' exception, but provides no assistance to
> catching it. LookupException might as well be declared a RuntimeException,
> since all practical type checking has been lost; existing signatures would then
> not be a problem.

I'm not sure I follow.  It was principally for the abstract parser's benefit that these look-up methods are defined.  Being optional, implementations such as OCL or QVT that are concerned with ambiguity can benefit from providing the adapter.

> It is not possible to create a derived environment that implements both
> Environment and Environment.Lookup, since the derived implementation with throw
> is incompatible with the also-derived implementation without.
> 
> Given the need to preserve the existing signatures, there seems no alternative
> to introducing new methods with throws. In my derived environment I just added
> tryLookupClassifier and tryLookupPackage, with adapters to exploit them and a
> final @Deprecated on the old methods. The caller now gets to see the
> LookupException if using the new methods or a Deprecated if not. My tests pass.
> I didn't need to edit the patch. I just had to create silly adapters.
> 
> Therefore either:
> 
> Restore Lookup as an integral facility, by eliminating the Lookup interafces,
> adding at least tryLookupClassifier, tryLookupPackage and tryLookupState to
> Environment, and tryCreatePackageContext to EnvironmentFactory, then delegating
> directly to these methods from the traditional signatures with a backstop
> catch. Perhaps mark at least lookupState as deprecated.

Well, adding to the existing interfaces isn't really an option, but I can definitely change the Lookup operations to tryXyz() to simplify the environment implementation (yes, the additional adapter object isn't nice).  I hesitate to deprecate methods in an interface that clients implement because this imposes warnings in code that they are contractually obliged to implement.  :-)


> or:
> 
> Make Lookup a fully extensible facility and put the full set of tryLookupXXX,
> tryCreateXXX in the Lookup interface, and delegate directly to these methods
> possibly via instanceof for mix-in and/or via adaption.

Yes, I'll prefer this.  And I was actually intending to "try-ify" the other look-ups, too.  The plan item for supporting navigation of unnamed association ends will need to be able to report ambiguities that weren't really interesting, before when we only supported named ends that are necessarily unique in a valid model.  Now, with unnamed ends and multiple associations between the same two classifiers, ambiguity becomes more of a normal case.

Comment 34 Christian Damus 2007-10-01 09:23:37 EDT

(In reply to comment #32)
> Oops. Forgot to rerun LPG.

Well, didn't really forget.  Can't run it on Mac platform at home.  That will have to wait until I commit (still waiting for EMO's approval of my CQ).

> EssentialOCL.g should not import or throw ParserException.

Right, that follows from the improved problem handling.

> Many of your unit tests could probably lose ParserESception too.

Heh heh ... that would be a lot of work, and they do at least test the legacy behaviour.  I think I probably need to add more tests for the flexible problem handling, but will have to delegate job that to community contributions.

Comment 35 Ed Willink 2007-10-01 14:59:15 EDT

Severity doesn't need to be extensible.

Phase does. If it isn't, a composer/optimiser/executor will have to introduce yet another problem handling protocol for its phases.

A consumer of LookupException does three things, trigger throw, propagate and catch. In order to trigger a throw there must be a lookup method with a declared throw. If I support this in MyLookupAdapter.tryLookupPackage, the consumer must do env.getAdapter(...).tryLookupPackage(..) and catch the ambiguity exception, a bit slow and tedious. In order to propagate, further adapters must be available and used. This propagation is such a fundamental Environment behaviour that I suspect all consumers will mix-in rather than adapt.

My current implementation adapts because I cannot mix-in and stepping through revealed how massive the Adapter costs are. I'm a bit sensitive to such cost because everything I use on Eclipse 3.3 seems really slow compared to 3.1. I would recommend supporting the Lookup interface solely as a mix-in. Actually, I think it is necessary for AbstractEnvironment to implement this interface anyway, since all invocations of lookupXXX must be changed to tryLookupXXX to ensure that the LookupException is handled as such rather than as a compatibility backstop. If AbstractBasicEnvironment must implement it, the interface might as well be LookupEnvironment extending Environment rather than Lookup augmenting Environment.

Comment 36 Adolfo Sanchez-Barbudo Herrera 2007-10-02 05:24:56 EDT

Uau !!!!

What a wonderful lesson of design and pattern's use !!!

Im learning a lot guys !!!!

Well, i prefer to wait to apply my ocl-project dependant modifications to check the correct run of my parser. When this changes about the lookup interface are established i'll engage my modifications again ;).

A trivial issue that i hv detected:

@deprecated tag was forgotten in the old TypeUtil.CheckMutuallyComparable(env, type1, type2, opcode) method.

Best Regards !!!

Comment 37 Ed Willink 2007-10-10 14:10:51 EDT

I'm finding the need for AbstractEnvironment's parent to be more than an interface very restricting. (I am unable to use generic parmeters in derived environments to parameterize with respect to an interface since the parent type must inherit a specific concrete class which defeats the versatility of an interface.)

The only reason for it to be concrete seems to be to allow addProperty and addOperation to be protected.

AbstractBasicEnvironment parent usage can easily be changed to BasicEnvironment.

AbstractEnvironment parent usage can easily be changed to e.g AbstractedEnvironment that merges Environment and BasicEnvironment interfaces
and declares the addProperty and addOperation methods.

I've changed the code as above without problems apart from:

This then highlights a limitation in BasicEnvironment.getOptions(); the return type should be Map<Option<?>, Object> not Map<Option<?>, ?>.

Comment 38 Christian Damus 2007-10-11 19:13:39 EDT

The accumulation of patches is committed (with additional JUnit tests for compatibility scenarios) to CVS HEAD (1.2 branch).

Comment 39 Ed Willink 2007-10-12 02:33:18 EDT

Created attachment 80200 [details]
Trivial patch to make ProblemHandler.Phase sortable

Great. My tests work with very little catching up.

Strings and enums were sortable. The improved class isn't. With the attached patch it is.

Should I raise a separate bugzilla for comment #37?

Comment 40 Christian Damus 2007-10-12 09:59:08 EDT

Comparibility patch is committed.  Sorting by name accounts nicely for fitting subclasses' instances into the sort order.  I added hashCode() and equals() to be consistent with the zero comparison and made all comparison-oriented methods final to ensure enum-like consistency across the Phase hierarchy.

On the subject of comment #37, yes, a separate bug would be a good idea.  I haven't had a chance to think about the change you are proposing (and it looks like something is missing in the comment).  In any case, it looks like maintaining API compatibility might be an issue, so it will require more analysis.

Comment 41 Ed Willink 2007-10-12 13:17:00 EDT

Created attachment 80250 [details]
Patch to add LookupException(String, List<?>) constructor

You were going to make LookupException more friendly with a

LookupException(String, List)

constructor.

The

LookupException(String, Object, Object...)

is weird. It doesn't enforce two objects, just one. It constructs a BasicEList<Object> internally. With an Object type BasicEList has needless baggage compared to ArrayList.

I'm not sure why you shut down all possibility of extension. It is really hard to add the missing constructor by derivation. At least the getAmbiguousMatches() could be non-final so that a derivation could re-implement state.

Comment 42 Christian Damus 2007-10-12 14:22:59 EDT

Sorry, forgot about that LookupException.  Committed the patch.
About the final modifier:  for such a simple object as an exception, I guess that being able to override the getAmbiguousMatches() didn't seem interesting, because the exception could just be constructed with the list and wouldn't ever change.  But, if a client has a need to override, that's fine.

Comment 43 Christian Damus 2007-10-17 16:13:55 EDT

Forgot to assign to contributor

Comment 44 Christian Damus 2007-10-17 16:15:08 EDT

Fixed in the MDT OCL 1.2.0 I200710171533 build.

Comment 45 Kenn Hussey 2007-12-07 11:22:08 EST

Christian, any chance you could provide a link to the CQ for this bug... or is there another convention for linking the two records?

Comment 46 Christian Damus 2007-12-19 11:29:22 EST

(In reply to comment #45)
> Christian, any chance you could provide a link to the CQ for this bug... or is
> there another convention for linking the two records?

There was a CQ

  https://dev.eclipse.org/ipzilla/show_bug.cgi?id=1764

which the EMO resolved as "worksforme" because it wasn't necessary, Ed being a committer in the Modeling Project.  So, I didn't follow it up any further.

Comment 47 Kenn Hussey 2007-12-19 11:44:54 EST

Thanks!

Comment 48 Nick Boldt 2008-01-28 16:39:25 EST

Move to verified as per bug 206558.

Comment 49 Ed Willink 2011-05-27 02:38:45 EDT

Closing after over a year in verified state.

Comment 50 Ed Willink 2011-05-27 02:41:12 EDT

Closing after over a year in verified state.