search

eclipse-ocl / org.eclipse.ocl

Eclipse Public License 2.0
0 stars 0 forks source link

[parser] Exploit LPGv2 runtime. Reintroduce official LPG templates. #498

Closed eclipse-ocl-bot closed 1 hour ago

eclipse-ocl-bot commented 1 hour ago

| --- | --- | | Bugzilla Link | 299396 | | Status | CLOSED FIXED | | Importance | P3 normal | | Reported | Jan 12, 2010 12:03 EDT | | Modified | May 22, 2013 14:14 EDT | | Version | 1.3.0 | | Reporter | Adolfo Sanchez-Barbudo Herrera |

Description

MDT-OCL grammars are LPGv2 compliant. However, they still keep some hard-coded functionality which originally resided in the official LPG templates. Ideally, LPG grammars should use the lastest version of said templates, which have been designed to exploit LPGv2 runtime. So we need to reintroduce the official recommended LPG templates which should be exploited by the Essential and CompleteOCL grammars. The main goals of this change is the following.

eclipse-ocl-bot commented 1 hour ago

By Ed Willink on Jan 12, 2010 12:29

Welcome back; hope you had a good break.

Go for it.

It would be good to get this and the Stream decoupling in comfortably before M5.

eclipse-ocl-bot commented 1 hour ago

By Adolfo Sanchez-Barbudo Herrera on Jan 12, 2010 12:36

Created attachment 155886 (attachment deleted)\ First approach to fix

Hi all,

The ~~attachment ~~ (attachment deleted)provides a first approach to solve this. I have tried to keep the templates as similar as possible from the original one. Obviously, due to the evolution of OCL grammars, several modifications must be accomplished. A resume of the work is the following

The new versions of grammars/templates/inclusion are based on:

That said, I'll explain the relevant changes or notes I have taken:

Essential/OCLKWLexer related Grammars/Templates/Includes

KeywordTemplateD.g to KeywordTemplateF.gi:

  1. The new template has been adopted, excepting the removal of the unnecessary %Globals section.

KWLexerMap.gi to KWLexerMapF.gi:

  1. The new template has been adopted.

Essential/OCLKWLexer.gi:

  1. The import and used template have accordantly been changed. And I have accurately changed the file prefix for the EssentialOCLKWLexer.

OCLKWLexer.gi:

  1. The used template has accordantly been changed.

Essential/OCLLexer related Grammars/Templates/Includes

In LPG v2 the runtime API has been refactored so that a generated Lexer doesn't extend LexStream. Instead, it will contain an instance of said class, so that the lexer's API is much simpler. Therefore, we will deal with an AbstractLexer class (which will maintain the environment) and a DerivedLexerStream (which will contain the logic related to ILexStream). Unfortunately since the LexStream reports the errors, and we have delegated to our BasicEnviroment's problem handler, the DerivedLexerStream must contain the BasicEnviroment as well.

NB. BasicEnviroment/ProblemHandler design could be re-analyzed.\ NB. Some DerivedLexerStream methods should be analyzed to see if we can remove them. They are kept for compatibility reasons.

LexerTemplateD.g to LexerTemplateF.gi

  1. The generated OCLexer will extend the AbstractLexer, which won't be the LexStream anymore. Therefore, the macro names must be appropriately changed

    • A new macro $super_lexer_class appears which corresponds to the AbstractLexer, the super class which be extended by the generated lexer. This macro may be redefined by extenders.
    • A new macro $lex_stream_class corresponds to the type of an instance of the ILexStream used by the template. It's defined in the template and the include file to use the default LPG class (LpgLexStream). This macro may be redefined by grammars so that the EssentialOCLLexer.gi uses the new DerivedLexStream class (which will contain the customized logic of ILexStream).

  2. Some functionality which resided in the old LexerTemplateD now resides in the new LexerBasicMapF template.

  3. Several constructors based on some reset methods are included in the new template. Actually they are the contructors methods which resided in the AbstractLexer. Since they are decouple from the generated parser they are now are coded in the template (as the official LPG template does):

    • These reset methods are the KEY, so re-parsing will be made via this methods.
    • The constructor based on no-input (only an environment as parameter) has been removed. A parser/lexer infrastructure will be always constructed on a given input. Actually no uses of this way of creating parser/lexer have been found along MDT-OCL implementation.

  4. Some methods related to LexStream API have been been removed, such as setInputChars.

  5. Some methods related to AbstractLexer API have been kept, due to compatibility backwards, such as lexToTokens. However this method has been moved to AbstractLexer and reworked to invoke the proper lexer template's method.

LexerBasicMap.g to LexerBasicMapF.gi

  1. The original LexerBasicMapF.gi defines an extension of LpgLexStream. Since we have a new DerivedLexStream with customized logic a new $lex_stream_class macro is needed. Note that is the same macro used and defined by the template file (look at point 5).
  2. Since now we have an internal LpgLexStream class, some constructors methods are added to this include file.

EssentialOCLLexer.gi

  1. It seems that the $action_class macro is not needed. Although it could be removed, I have only added a deprecated comment just for compatibilty reasons.
  2. the $prs_stream_class and $lex_stream_class are redefined to use our DerivedPrsStream and DerivedLexStream classes
  3. Some additional methods needed by OCL Lexer in order to work with an input Reader, have been included in the lexer file (Header section) instead of creating them in the template file.
  4. The import and used template have accordantly been changed. And I have accurately changed the file prefix for the EssentialOCLLexer.

OCLParser related Grammars/Templates/Includes

As occurred with the Lexer, the AbstractParser doesn't extend PrsStream anymore. Instead, it will contain an instance of said class, so that the parser's API is much simpler. Same approach, explained for the lexer, has been taken for the parser.\ On the other hand, the previous EssentialOCL.gi grammar file includes the original parserTemplate code defined by LPG into the own file. The concept of parserTemplate has been rescued and the last version provided by LPG has been adopted. Actually the refactoring of parser infrastructure comes from the template itself.

dtParserTemplateF.gi

  1. Most of the %define section defined in Essential.OCL.gi was the original LPG dtParserTemplate.g . So, the section has been removed from there and it has been merged into the new dtParserTemplate.F
  2. Only one constructor, which receives an AbstractLexer instance is maintained.
  3. The OCL's default_repair_count approach has been maintained, although it's has been adopted via macro mechanisms
  4. Some macro definitions (such as $parserCore) and some methods (parseTokensToCST), which were initially defined in Essential.gi have been revised and removed/reworked since they are now considered in somehow in the template and/or AbstractParser.
  5. All the mechanisms introduced in Essential.gi in order to allow backtracking parser using are removed, since the btParserTemplateF.gi will also be reintroduced.
  6. The confusing $lex_stream_class macro has been renamed so that it's defined as $super_lexer_class.

btParserTemplate.gi

  1. No additional comments are needed. Just the idea of error_repair_count < 0 => parse and error_repair_count > -1 => fuzzyParse is kept in the template.

EssentialOCL.gi

  1. Most of the the define section has been removed (look at point 13).
  2. %EOF and %Error section has been removed since it is defined in the template.
  3. %Headers section doesn't call $parserCore anymore. The code now resides in the template (look at point 16). Besides, now includes couple of methods which avoid changing grammar action-blocks.\ NB. The action-block code could be reworked in the future so that this methods can be removed. Another point is the problem that may cause extenders if we remove this methods.
  4. In section %Rules, The macro $BeginActions doesn't need to be called.
  5. %Trailers section is removed since it is defined in the template.

Changes to OCLParser.g

  1. The main change is that now, a LPG template will be used, so that a new option will be required by the .g grammar file: %option template=dtParserTemplateF.gi
  2. Some options will be removed since they are included by the template => remove table, error_maps, scopes, margin, programing_language, action-block options.
  3. the confusing $lex_stream_class has been

Concerning backtracking grammars there is no need to add extra information. Most of the specific macros (which let us work with BacktrackingParser) are removed and we only have to use the new btParserTemplateF.gi).

OCL API impact \ The main impact of the current lexer/parsing infraestructure is the following:

  1. Now we are not allowed to create a analyzer/parser/lexer instances without firstly providing the input (reader, array of chars, filename\ or whatever).
  2. Secondly some used Parse/LexerStream functionality is not available as it has been so far, since the AbstractParser and AbstractLexer have been drastically simplified. One easy solution is calling getIPrsStream or getILexStream from AbstractParser and AbstractLexer respectively as I have firstly done in this patch. Another solution is invoking the current API provided by the template (such us getRhsIToken, getRhsFirstToken, etc), or even adding and using some helpful specific functionality to the AbstractParser and AbstractLexer.

I think this is a first approach to start working around the needed refactoring of API for the MDT parsing infrastructure. I guess we will have some interesting discussion about it. I remember that Ed didn't like the new contract to invoke the lexer (OCLLexer.lexer()) and the parser (OCLParser.parser()). These names come from the LPG templates and I haven't wanted to change them because it is not necessary. In any case, this convention (and anything else) my be discussed and changed.

Regards,\ Adolfo.

eclipse-ocl-bot commented 1 hour ago

By Adolfo Sanchez-Barbudo Herrera on Jan 12, 2010 12:49

Assigning and adding interested committers

(In reply to comment #1)

Welcome back; hope you had a good break.

No good at all. I didn't work three days (from Wed to Fri) last week because of an illness :(

I have been working Tue(last week), yesterday and today in the attached patch.

Go for it.

It would be good to get this and the Stream decoupling in comfortably before M5.

It depends on how QVTo and QVTd accepts the proposal.

Ed, Sergey, if you feel that I should better or quicker face on the QVTd or QVTo counterparts, just let me know it. I think I could invest tomorrow to provide a patch for them.

BTW, sorry for the so long post with the details >.<

Cheers,\ Adolfo.

eclipse-ocl-bot commented 1 hour ago

By Ed Willink on Jan 14, 2010 12:58

The (trivial) patch for GenericParserBacktrackingTest is missing.

It seems relatively easy to fix the QVTd utility code to accommodate the extra object indirection.

A commented out action line such as:

// IdentifierCS identifierCS = createUniqueIdentifierCS($getToken(1));

generates code when the $getToken is substituted with a new-line. Changing the getToken deprecation macro to a single line fixes the problem:

$getToken /. /* FIXME macro getToken is deprecated. Use function getRhsIToken */ prsStream.getToken./

QVTc then generates with only one error the single environment argument conbstructor. The comment needs the FIXME to attract attention.

I thought the purpose of this change was to simplify the API. OCLBacktrackingLexer now has seven rather than three constructors and does not have the constructor used by QVTd's OCL editor; a constructor with a single environment argument allowing the source text to be specified later. See org.eclipse.qvt.declarative.parser.ocl.OCLFileAnalyzer.

The public API is much too large. It is probably necessary to have all the getRhsIToken etc methods to avoid hitting the 65535 byte class size limit, but I think all such methods should be protected. Any access to the IPrsStream should use the single getIPrsStream() method. Make the public API as small as possible. makeComment etc should have protected rather than package access. setResult should have protected rather than private access.

Migrating closer to standard templates is a good idea.

I'm not at all clear whether the aesthetic benefits of separating Parser and PrsStream are worthwhile. Removing the fat inherited API is good, but does anyone really care? Requiring every token access within the grammar rules to have an extra indirection seems to be a modest but undesirable cost that outweighs the modest but unnecessary modularity improvement. Is there a stronger use case for partitioning the lexer and parser objects?

eclipse-ocl-bot commented 1 hour ago

By Adolfo Sanchez-Barbudo Herrera on Jan 14, 2010 13:58

Ed,

On the one hand, considerable amount of code of the grammars should be changed in order to use the proper macros, or just invoking methods, as the own template's macro suggests

For instance, $getIToken($getToken(N)) should be replaced by the getRhsIToken function call.

On the other hand, I have done minor API changes (breaks), for instance removing an AbstractParser constructors in order not to increase the template. I have tried to be aligned to the template as closer as possible. However, I would agree on introducing some changes in it in order to simplify more the OCLParser/Lexer APIs, for instance removing constructor or resets methods if they are not interesting or used along OCL/QVTd/QVTo.

In resume, this is a first approach to reintroduce the LPG templates, which I consider very useful to gain maintenability of the parser and adopt the design principles of LPGv2. I also agree that we can still do more improvements of the API and the use of it, but it's really difficult to track all-in-one when we have the messy we have (an old-fashioned template coded inside the grammars).

About a use case (which is not currently coded in this patch) to show the benefits of this division: Reparsing (for instance in an editor) should be done via calling the resets method instead of creating new instances of OCLAnalyzers/Parsers/Lexers, which should pretentiously improve performance.

I believe we should go for this division as regards of the recommended LPG templates, in order to establish decoupled artifacts (LexStream from Lexer, PrsStream from Parser). Then we could do some custom (documented) tuning of the template, in order to simplify the API (M6 is our last checkpoint to do that). Finally, we could take advantage of this refactored API, which may even be done in future releases since the API is established now.

Sergey, any chance to align QVTo ? any comments ?.

Cheers,\ Adolfo.

eclipse-ocl-bot commented 1 hour ago

By Sergey Boyko on Jan 14, 2010 14:56

(In reply to comment #5)\ Hi Adolfo,

Excuse me, I'm quite busy during these days.\ I'll look at QVTo grammar aligning tomorrow.

Sergey, any chance to align QVTo ? any comments ?.

Cheers, Adolfo.

eclipse-ocl-bot commented 1 hour ago

By Sergey Boyko on Jan 19, 2010 16:51

(In reply to comment #6)\ Hi Adolfo,

I've checked QVTo against patch you provided. Basically there are no problems, just a few test fails. \ Appropriate patch for QVTo is ready.

Regards,\ Sergey

Sergey, any chance to align QVTo ? any comments ?.

Cheers, Adolfo.

eclipse-ocl-bot commented 1 hour ago

By Adolfo Sanchez-Barbudo Herrera on Jan 20, 2010 05:08

Ok,

So I guess this is a +1 from QVTo counterpart. I'll complete the migration guide during today.

Ed, I would need your +1. It seems to me that you are note completely happy with the change, but not sure about completely reluctant. I could do some improvements in the grammars and API when this is committed.

Next steps:

Cheers,\ Adolfo.

eclipse-ocl-bot commented 1 hour ago

By Ed Willink on Jan 20, 2010 05:57

Sorry -1. Thought I'd already replied.

I cannot give a +1 while the Environment-only constructor is missing.

This constructor was specifically introduced to support the re-use that you think requires the partitioning.

The editors:\ a) create an Analyzer etc with a FileEnvironment and no associated input\ b) the current input is esablished and parsed by a RootEnvironment etc

b) can be repeated many times on the same Analyzer.

Please put back the missing constructor and consider removing all the new ones.

You're earlier comments suggested API refinement in progress. This needs to be substantially complete before a commit to HEAD. HEAD is not for work in progress, certainly not post M4.

eclipse-ocl-bot commented 1 hour ago

By Adolfo Sanchez-Barbudo Herrera on Jan 20, 2010 10:01

Created attachment 156652 (attachment deleted)\ Proposed patch (try2)

Description of the modifications introduced by this patch (regarding the previous one).

Modifications to LexerTemplateF.gi template which introduce some deviations from the official LPG one:

Modification to EssentialKexer.gi file:

Note that the following QVTr code:

Reader fileReader = new FileReader(file);\ QVTrLexer lexer = new QVTrLexer(env.getFileEnvironment());\ QVTrParser parser = new QVTrParser(lexer);\ lexer.setFileName(file.toString());\ lexer.initialize(fileReader);

Could be simplified as follows:

QVTrParser = new QVTrParser(new QVTrLexer(env.getFileEnvironment(), fileReader, file.toString());

API discussion. I have decided to deprecate API (the less as possible) rather than remove it. In spite of the fact we are in major increment release, I'm not sure about providing API cleaning, since it won't probably be welcomed (compatibility backwards and API freeze approach reasons). However, if you agree, we could do the API cleaning, by deprecating methods which could be removed in the next major increment release. I could study it, and elaborate a list of method candidates, which you could simple + (agree on deprecating the method) or - (leave the method as part of the API).

If you don't like the idea, we could also leave the API as this patch provides.

In any case, if you feel that the patch is OK now, I'm inclined to commit the patch and make the API cleaning as part of a different bugzilla.

I would clarify the next steps as follows: \ a) Complete the migration guide.\ b) Lexer/Parser/Analyzer API cleaning (via deprecating methods).\ c) Exploit new Lexer/Parser/Analyzer API (via using lexer/parser reset methods in favour of creating multple lexer/parser instances)\ d) Grammars changes. (replacing deprecated macros by new parser functions, for instance replace $getIToken($getToken(1)) by getRhsIToken(1), or $setResult by setResult), when it's done we could remove (or deprecate) the IToken getIToken(int i) function defined in EssentialOCL.gi herader's section. Another alternative: we could override the deprecated macros, in the MDT-OCL grammar file, to keep the current MDT-OCL macro definition, and not to introduce the said unnecessary indirections included by the deprecrated macros definition from LPG templates.

If you really consider that we need to include any of this next steps as part of this bug, just let me know (I'll try to work on it asap). As said, I prefer to adopt this templates-based grammars approach now that seems to be fine, so that this bug doesn't conflict with other bugs related to the parser.

Cheers,\ Adolfo.

eclipse-ocl-bot commented 1 hour ago

By Ed Willink on Jan 21, 2010 15:03

Created attachment 156855 Fixes

Attached:

Fixes an NPE when the Parser constructor uses an input-less lexer.

Provides the missing test warning fix.

Narrows the Parser API by making most support protected.

Fixes the macros to (not) work within comments.

Eliminates deprecated @since 3.0 methods.

This then works with only very modest changes for QVTc and QVTr.

The 'new' methods in AbstractParser should have JavaDoc comments.

I can +1 the new attached, so if you're happy too go for it.

I'm happy to +1, in advance, a further commit replacing the deprecated macros.

:notepad_spiral: Bug299396.patch

eclipse-ocl-bot commented 1 hour ago

By Adolfo Sanchez-Barbudo Herrera on Jan 22, 2010 13:38

Ed,

I'm OK with the patch.

I'll commit it with the following adjustments:

P.S: I'll create new bugs for the next steps:

Cheers,\ Adolfo.

eclipse-ocl-bot commented 1 hour ago

By Adolfo Sanchez-Barbudo Herrera on Jan 22, 2010 13:46

Sergey, Ed,

Committed to HEAD. Resolving as FIXED.

eclipse-ocl-bot commented 1 hour ago

By Sergey Boyko on Jan 22, 2010 15:53

(In reply to comment #13)\ Adolfo,

thank you for notifying. I'll commit QVTo changes on Monday.

Sergey, Ed,

Committed to HEAD. Resolving as FIXED.

eclipse-ocl-bot commented 1 hour ago

By Sergey Boyko on Jan 24, 2010 16:26

Hi,

Please provide new OCL I-build.

Thanks in advance,\ Sergey

eclipse-ocl-bot commented 1 hour ago

By Alexander Igdalov on Jan 25, 2010 10:14

(In reply to comment #15)

Hi,

Please provide new OCL I-build.

Thanks in advance, Sergey

Hi Sergey,

The I-build is available at our downloads page.

Cheers,

eclipse-ocl-bot commented 1 hour ago

By Ed Willink on May 27, 2011 02:54

Closing