Closed eclipse-ocl-bot closed 2 months ago
eclipse-ocl-bot
commented
2 months ago By Ed Willink on Mar 05, 2007 15:04
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.
eclipse-ocl-bot
commented
2 months ago By Ed Willink on Apr 12, 2007 17:02
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.
eclipse-ocl-bot
commented
2 months ago By Ed Willink on Sep 15, 2007 14:10
Created attachment 78499 (attachment deleted)\
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.
eclipse-ocl-bot
commented
2 months ago By Christian Damus on Sep 17, 2007 09:45
Fabulous! Thanks, Ed. I'll have a look right away. Adolfo has expressed interest in reviewing this, too.
eclipse-ocl-bot
commented
2 months ago By Christian Damus on Sep 17, 2007 13:26
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?
eclipse-ocl-bot
commented
2 months ago By Christian Damus on Sep 17, 2007 13:29
Created attachment 78575 (attachment deleted)\
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.
eclipse-ocl-bot
commented
2 months ago By Ed Willink on Sep 17, 2007 15:21
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.]
eclipse-ocl-bot
commented
2 months ago By Christian Damus on Sep 17, 2007 16:09
(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.
eclipse-ocl-bot
commented
2 months ago By Adolfo Sanchez-Barbudo Herrera on Sep 18, 2007 10:05
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:
$lex_stream_class could be a better name instead of $super_stream_class, so in lexer and parser grammar files we would have:
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
$Define section
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:
Some more notes:
$Define section
$Header and $Trailer Sections\ with the correct use of the template both sections will dissapear.
$Rules section
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:
I think that i have been writing too much ¬¬!!, let's go to work !!!!!
Cheers,\ Adolfo
eclipse-ocl-bot
commented
2 months ago By Christian Damus on Sep 18, 2007 10:55
(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:
- 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 ...
eclipse-ocl-bot
commented
2 months ago By Adolfo Sanchez-Barbudo Herrera on Sep 18, 2007 12:36
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.
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).
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.
eclipse-ocl-bot
commented
2 months ago By Adolfo Sanchez-Barbudo Herrera on Sep 18, 2007 12:41
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.
eclipse-ocl-bot
commented
2 months ago By Ed Willink on Sep 18, 2007 13:37
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.
eclipse-ocl-bot
commented
2 months ago By Christian Damus on Sep 18, 2007 14:09
(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.
eclipse-ocl-bot
commented
2 months ago By Adolfo Sanchez-Barbudo Herrera on Sep 19, 2007 08:13
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
eclipse-ocl-bot
commented
2 months ago By Adolfo Sanchez-Barbudo Herrera on Sep 19, 2007 11:38
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.
eclipse-ocl-bot
commented
2 months ago By Ed Willink on Sep 20, 2007 03:49
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.
eclipse-ocl-bot
commented
2 months ago By Adolfo Sanchez-Barbudo Herrera on Sep 20, 2007 04:40
(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
eclipse-ocl-bot
commented
2 months ago By Christian Damus on Sep 20, 2007 10:05
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.
eclipse-ocl-bot
commented
2 months ago By Adolfo Sanchez-Barbudo Herrera on Sep 21, 2007 11:41
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:
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.
eclipse-ocl-bot
commented
2 months ago By Christian Damus on Sep 21, 2007 11:58
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.
eclipse-ocl-bot
commented
2 months ago By Adolfo Sanchez-Barbudo Herrera on Sep 21, 2007 14:07
(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.
eclipse-ocl-bot
commented
2 months ago By Ed Willink on Sep 22, 2007 03:39
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.
eclipse-ocl-bot
commented
2 months ago By Adolfo Sanchez-Barbudo Herrera on Sep 26, 2007 10:16
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:
public AbstractLexer(BasicEnvironment environment, String filename) throws java.io.IOException {\ super(filename);\ this.environment = environment;\ \ }
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
So in the line 2059 (AbstractOCLAnalyzer):\ Would be better placed the next code:
TypeExp
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.
eclipse-ocl-bot
commented
2 months ago By Ed Willink on Sep 27, 2007 03:42
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?
eclipse-ocl-bot
commented
2 months ago By Adolfo Sanchez-Barbudo Herrera on Sep 27, 2007 05:16
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
eclipse-ocl-bot
commented
2 months ago By Ed Willink on Sep 28, 2007 02:47
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);\
}\
}By Adolfo Sanchez-Barbudo Herrera on Sep 28, 2007 05:15
(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(newFileInputStream(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.
eclipse-ocl-bot
commented
2 months ago By Christian Damus on Sep 29, 2007 12:51
Created attachment 79441 Updated patch for API compatibility and other changes
Attached a ZIPped up variant of the original patch, with the following changes:
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).
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.
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.
:notepad_spiral: ocl-parser-refactoring.patch.zip
eclipse-ocl-bot
commented
2 months ago By Christian Damus on Sep 29, 2007 12:57
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.
eclipse-ocl-bot
commented
2 months ago By Ed Willink on Sep 30, 2007 05:23
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.
eclipse-ocl-bot
commented
2 months ago By Ed Willink on Sep 30, 2007 06:47
Oops. Forgot to rerun LPG.
EssentialOCL.g should not import or throw ParserException.
Many of your unit tests could probably lose ParserESception too.
eclipse-ocl-bot
commented
2 months ago By Christian Damus on Oct 01, 2007 09:19
(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.
eclipse-ocl-bot
commented
2 months ago By Christian Damus on Oct 01, 2007 09:23
(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.
eclipse-ocl-bot
commented
2 months ago By Ed Willink on Oct 01, 2007 14:59
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.
eclipse-ocl-bot
commented
2 months ago By Adolfo Sanchez-Barbudo Herrera on Oct 02, 2007 05:24
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 !!!
eclipse-ocl-bot
commented
2 months ago By Ed Willink on Oct 10, 2007 14:10
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<?>, ?>.
eclipse-ocl-bot
commented
2 months ago By Christian Damus on Oct 11, 2007 19:13
The accumulation of patches is committed (with additional JUnit tests for compatibility scenarios) to CVS HEAD (1.2 branch).
eclipse-ocl-bot
commented
2 months ago By Ed Willink on Oct 12, 2007 02:33
Created attachment 80200 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?
:notepad_spiral: SortablePhases.patch
eclipse-ocl-bot
commented
2 months ago By Christian Damus on Oct 12, 2007 09:59
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.
eclipse-ocl-bot
commented
2 months ago By Ed Willink on Oct 12, 2007 13:17
Created attachment 80250 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
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.
:notepad_spiral: LookupException.patch
eclipse-ocl-bot
commented
2 months ago By Christian Damus on Oct 12, 2007 14:22
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.
eclipse-ocl-bot
commented
2 months ago By Christian Damus on Oct 17, 2007 16:13
Forgot to assign to contributor
eclipse-ocl-bot
commented
2 months ago By Christian Damus on Oct 17, 2007 16:15
Fixed in the MDT OCL 1.2.0 I200710171533 build.
eclipse-ocl-bot
commented
2 months ago By Kenn Hussey on Dec 07, 2007 11:22
Christian, any chance you could provide a link to the CQ for this bug... or is there another convention for linking the two records?
eclipse-ocl-bot
commented
2 months ago By Christian Damus on Dec 19, 2007 11:29
(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.
eclipse-ocl-bot
commented
2 months ago By Kenn Hussey on Dec 19, 2007 11:44
Thanks!
eclipse-ocl-bot
commented
2 months ago By Nick Boldt on Jan 28, 2008 16:39
Move to verified as per bug 206558.
eclipse-ocl-bot
commented
2 months ago By Ed Willink on May 27, 2011 02:38
Closing after over a year in verified state.
eclipse-ocl-bot
commented
2 months ago By Ed Willink on May 27, 2011 02:41
Closing after over a year in verified state.
| --- | --- | | Bugzilla Link | 176110 | | Status | CLOSED FIXED | | Importance | P3 enhancement | | Reported | Mar 01, 2007 17:15 EDT | | Modified | May 22, 2013 14:08 EDT | | Version | 1.1.0 | | Reporter | Ed Willink |
Description
I have created a variant of the 1.1M5 org.eclipse.ocl.internal.parser\ package to support extension for e.g. QVTrelation and to support user\ configurable error handling policies such as the use of IMarker. I have\ also fixed a few bugs in the grammar implemntation.
The revised OCL package may be found in
/cvsroot/technology/org.eclipse.gmt/umlx/plugins/org.eclipse.gmt.umlx.eqvtr.cst
The extending QVTr package may be found at
org.eclipse.gmt.umlx.eqvtr.cst.parser in the same plugin.
Brief summaries of the changes and their rationale follows.
The internal.parser refactoring\
No changes: Ascii,g, KWLexerMap.g\
Grammar changes - KeywordTemplateD.g\
Remove unnecessary import.
Grammar changes - OCLKWLexer.g\
Introduce $copyright_contributions, so that...
My QVTrKWLexer.g can import OCLKWLexer.g, define additional keywords and extend the copyright.
Grammar changes - LexerBasicMap.g\
Add an @Override.
Grammar changes - LexerTemplateD.g\
Use AbstractLexer.java base class.\ Add 4 @Overrides.\ Move single argument lexer to AbstractLexer baseclass.
Grammar changes - OCLLexer.g\
Bug fix: remove redundant $\ Bug fix: Remove redundant imports\ Bug fix: Remove duplicate EOF definitions.\ Move error reporting to AbstractLexer base class.\ Introduce setInputChars() to decouple construction and initialisation. Introduce $copyright_contributions, so that...
My QVTrLexer.g can import OCLLexer.g, define additional keywords and extend the copyright.
Grammar changes - OCLLPGParser.g\
Refactor:\ basic error reporting to AbstractLPGParser.java\ OCL creation utilities to AbstractOCLLPGParser.java\ the residual OCL grammar stays in OCLLPGParser.g\ \ My QVTrLPGParser.g just includes .
Bug fix: Correct 'AST'Node type\ Bug fix: Remove redundant imports\ Bug fix: Narrowed operationCallExpCS on unary (MINUS, not) to eliminate reduce/reduce conflicts\ Adjusted createMessageExpCS argument to make baseclass symbol file indepenedent \ Code evolution: Added generic arguments to all Lists.\ Introduce $copyright_contributions
[I put in all the missing generic signatures, but it just moves around the\ unchecked casts with only subjective aesthetic benefits, so I rewound to\ avoid unnecessary differences. Perhaps the next LPG release will support typed\ productions and solve the problem.]
Structure changes - OCLParser.java\
OCLParser used to inherit from OCLLPGParser making it very difficult to independently extend AST and CST layers. It now composes an OCLLPGParser so that there is a consistent structure for each of the four parsers, AST parser composes CST parser composes CST lexer composes KW lexer, and each is extensible.
Refactor:\ basic error reporting/environment to AbstractParser.java\ generic OCL CST to AST conversion to AbstractOCLParser.java\ goal related OCL CST to AST conversion to OCLParser.java
For QVTr, I replace OCLParser by QVTr specific layers.
[The Parser|LPGParser/PrsSteam|Lexer/LexStream|KWLexer naming is not that good. I renamed to ASTParser/CSTParser/CSTLexer/KWLexer which was better then reverted to minimise changes. I think the renaming was a good idea.]\ \ Error handling\
Difficult to configure. I want errors to appear as IMarker elements in a text editor, so introduce ErrorHandler, with ErrorStringHandler for the current behaviour or ... or MarkerErrorHandler for me.
I do not really understand the benefit of a declared SemanticException, it just causes every AST method to have a throws declaration:
Ideally an ErrorHandler would be maintained by Environment so that all errors go via the Environment to the ErrorHandler.
There are nearly five different errors methods for each of lexer, parser and semantic\ analyser. Examination suggests that only one method is used for each, so the\ LexerErrorHandler, ParserErrorHandler and SemanticErrorHandler interfaces have\ just one method each.
I didn't want to edit Environment, so I temporarily support setting ErrorHandler on each abstract parser layer. I'm sure the ErrorHandler interface can be further improved when moved to Environemnt.
Separate queries\
OCL.genmodel\
The Edit and Editor plugin clases are defined as ecore.provider.OCLEditPlugin and ecore.presentation.OCLEditPlugin. Surely an org.eclipse.emf.ocl prefix should be added?
org.eclipse.emf.ocl.edit and org.eclipse.emf.ocl.editor\
These plug-ins may be redundant for pure OCL usage, but they may be needed by OCL extensions.
OCLCST.ecore\
Why isn't composition used throughout? The CST is a simple tree so determination of composition/reference is not hard. Failure to define compositions mean that when the CST is saved to a file, many terms are trivialised to dummies.
internal\
All internal packages report a Java discouraged access.
It would be helpful to adopt the pre-Java 5 policy that 'internal' represents a verbal warning. Since the CST is so useful, the Java warning should be eliminated by exporting the internal packages.
NPEs\
CollectionTypeImpl.getName line 363: elementTypeName = elementType.getName() needs to just skip the elementTypeName if elementType null.
Trivia\
In TypeUtil.matchArgs following the resolution of a Variable arg:
is needed in case the source has user domain rather than OCL domain types.