Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: Add action templates functionality #4345

Open
wants to merge 9 commits into
base: dev
Choose a base branch
from

Conversation

DavidGregory084
Copy link

@DavidGregory084 DavidGregory084 commented Jul 3, 2023

This is an idea about how to resolve #4067 without developing an entirely new actions language.

This PR enables users to write cross-target grammars which make use of actions, by enabling users to provide action templates as StringTemplate .stg group files on the command line.

By providing different action templates for each target language, users can provide a different implementation of the action logic for each target.

Java.stg:

normalizerImports() ::= <<
import java.text.Normalizer;
import java.text.Normalizer.Form;
>>
normalize(s) ::= <<Normalizer.normalize(<s>, Form.NFKC)>>
getText() ::= <<getText()>>
setText(s) ::= <<setText(<s>);>>

Javascript.stg:

normalizerImports() ::= ""
normalize(s) ::= <<<s>.normalize("NFKC")>>
getText() ::= "this.text"
setText(s) ::= "this.text = <s>"

The example below is the motivating example for me - I have a grammar that I'd like to use in both a JVM-based compiler and a VS Code extension:

Example.g4:

lexer grammar Example;

@lexer::header {
<normalizerImports()>
}

ID : (ID_START ID_CONTINUE* | '_' ID_CONTINUE+) { <setText(normalize(getText()))> } ;
ID_START : [\p{XID_Start}] ;
ID_CONTINUE: [\p{XID_Continue}] ;
WS : (' '|'\n') -> skip ;

Thanks to my employer @opencastsoftware for sponsoring this work!

This enables users to write cross-target grammars which make use of
actions, by enabling users to provide action templates on the command
line.

By providing different action templates for each target language, users
can provide a different implementation of the action logic for each
target.

Signed-off-by: David Gregory <2992938+DavidGregory084@users.noreply.github.com>
Signed-off-by: David Gregory <2992938+DavidGregory084@users.noreply.github.com>
@ericvergnaud
Copy link
Contributor

Mmmm... this IS an "entirely new actions language". The fact that it leverages templates makes it look like not (and is elegant), but we'd still have to opine on the syntax and more importantly: the supported set of 'keywords', both characteristics of a language...
The latter could be a blocker because we precisely don't want to find ourselves in a situation where we'd have to expand that set for every language, because the existing set does not support the specific use case a user needs to support...
Using templates for testing works because the set is limited to what the engine requires, and the task of supporting additional templates lies with the developer needing it, but with your proposal that task would lie with target authors who don't actually need it, so not sure it would work well.
As you know, there are other options being discussed.

@DavidGregory084
Copy link
Author

Mmmm... this IS an "entirely new actions language". The fact that it leverages templates makes it look like not (and is elegant), but we'd still have to opine on the syntax and more importantly: the supported set of 'keywords', both characteristics of a language...

Hmm can you explain what you mean by that? The syntax is that of StringTemplate, and all of the template identifiers used are user-defined - they’re simply supplied as StringTemplate groups.

@DavidGregory084
Copy link
Author

Ah I think I understand - I’m not proposing that there is a centrally managed collection of StringTemplate groups shipped with ANTLR - this PR adds a command line option via which users can supply their own templates which they have written themselves.

Writing cross-target grammars is a rather advanced use case and I think it’s reasonable to expect users to learn StringTemplate syntax if they want to do that.

@DavidGregory084
Copy link
Author

Why StringTemplate?

  • It's used in the implementation of ANTLR itself and in the ANTLR code generation process, so it's familiar to ANTLR contributors
  • It's already on the classpath of the ANTLR tool, so there are no new dependencies needed
  • It's a stable and well-tested template language that will allow users to provide whatever kind of action logic they want

@kaby76
Copy link
Contributor

kaby76 commented Jul 4, 2023

I have a few observations and questions.

  • I've been writing target-agnostic Antlr grammars probably more than most, porting grammars-v4 to different targets, pretty much every day of the week for several years, and just got through yet another, python/python3. The main difficulty is that the runtime APIs across targets are inconsistent. Three bugs were discovered ([typescript] Runtime declares CommonToken(source, .....) wrong. #4318 [Dart] Fix for #4320--export additional types #4340 [Go] Fix for #4342--EmitToken(token) in Go target needs to be exported #4343). While annoying, dealing with actions per target is manageable using target agnostic format.
  • Importantly, why is this part of a parser generator? And, as with any parser generator, where is the boundary between parser generator and some other tool that uses it? For example, I am firmly against embedding a sub-par XPath engine (pre-version 1) in the Antlr runtime. It exists only for some targets. That is why I ported a XPath version 2 engine to C#, and added a DOM interface over the Antlr parse tree in a separate tool. Antlr trees change very rarely, which means that it can be released on its own schedule, not tied at all to a minor version number change in Antlr.
  • Note, I've been writing, already, a string processor for Antlr grammars, trgen, so I am familiar with ST--and all it's problems--and the idea of separating ST processing from the parser generator. (I've been thinking of a more general processor as well, and have always been struck by the fact that ST does not contain a general-purpose, command-line processor.)
  • I am not at all happy with the way the model-view is separated in ST. For example, you cannot write a simple string equality test: <if(target=="CSharp")>....<endif>.

@DavidGregory084
Copy link
Author

DavidGregory084 commented Jul 4, 2023

While annoying, dealing with actions per target is manageable using target agnostic format.

I think the Fortran90 grammar is actually a good example in itself of the need for some kind of solution to target-agnostic actions.

The target-agnostic format described there relies upon superClass, language keywords like this and property dereference via . in order to work, which means that it falls short for non-OO targets like Go and needs adjustments in order to produce valid code. It’s great that such a pattern exists and is well documented, but I think a more integrated solution would be preferable.

In many cases it would be really difficult for users to integrate such a script into their workflow (e.g. when the ANTLR generator is called by a build tool plugin or as part of some other complex workflow).

Importantly, why is this part of a parser generator? And, as with any parser generator, where is the boundary between parser generator and some other tool that uses it?

ST is already a dependency of the ANTLR tool, so I guess to some extent the question of whether to embed it into ANTLR has already been decided. The expansion of ST templates in this PR is completely optional and is only active if the user provides the -DactionTemplates command line flag.

I suppose the boundary here is the point of template expansion, which happens early in the ANTLR generation process just after parsing the grammar file itself. It's also where the most troubling interactions occur, in terms of showing users error messages relating to the template expansion process.

I am not at all happy with the way the model-view is separated in ST.

I think that is a moot point in this implementation as no model binding is performed - the user has to write all of the groups that they want to use in their .stg template, and they can use only those groups in their grammar. 😄

Signed-off-by: David Gregory <2992938+DavidGregory084@users.noreply.github.com>
@kaby76
Copy link
Contributor

kaby76 commented Jul 4, 2023

Usually, I would say that ST should be supported outside the tool, but after thinking about this a bit, I think you are right. The main reason why it should be supported in the tool is because people would say a grammar that requires further processing shouldn't be called an Antlr grammar. In addition, the grammar could be tagged as requiring a certain version of Antlr. I will try out some examples shortly.

@kaby76
Copy link
Contributor

kaby76 commented Jul 4, 2023

Semantic predicates are actions, too. It's not expanding template references there, but it is working elsewhere.

header() ::= <<
int count;
>>

action1() ::= <<
this.count = 0;
>>

pred() ::= <<
this.count \<= 0
>>

action2() ::= <<
this.count++;
>>
grammar Expr;

options { actionTemplates='Temp.stg'; }

@members {
<header()>
}

file : x (SEMI x)* SEMI? EOF;
x : { <action1()> } expression;
expression : expression POW expression
 | expression (TIMES | DIV) expression
 | expression (PLUS | MINUS) expression
 | LPAREN expression RPAREN
 | (PLUS | MINUS)* atom ;
atom : scientific | { <pred1()> }? variable { <action2()> } ;
scientific : SCIENTIFIC_NUMBER ;
variable : VARIABLE ;

VARIABLE : VALID_ID_START VALID_ID_CHAR* ;
SCIENTIFIC_NUMBER : NUMBER (E SIGN? UNSIGNED_INTEGER)? ;
LPAREN : '(' ;
RPAREN : ')' ;
PLUS : '+' ;
MINUS : '-' ;
TIMES : '*' ;
DIV : '/' ;
GT : '>' ;
LT : '<' ;
EQ : '=' ;
POINT : '.' ;
POW : '^' ;
SEMI : ';' ;
WS : [ \r\n\t] + -> channel(HIDDEN) ;

fragment VALID_ID_START : ('a' .. 'z') | ('A' .. 'Z') | '_' ;
fragment VALID_ID_CHAR : VALID_ID_START | ('0' .. '9') ;
fragment NUMBER : ('0' .. '9') + ('.' ('0' .. '9') +)? ;
fragment UNSIGNED_INTEGER : ('0' .. '9')+ ;
fragment E : 'E' | 'e' ;
fragment SIGN : ('+' | '-') ;

@DavidGregory084
Copy link
Author

Semantic predicates are actions, too. It's not expanding template references there, but it is working elsewhere.

Ah thanks for trying it out - I will take a look at semantic predicates tomorrow

@kaby76
Copy link
Contributor

kaby76 commented Jul 4, 2023

Looks like you need to test for SEMPRED in addition to ACTION in expandActionTemplates(GrammarRootAST root). Then, trim the extra '?' from the text of the predicate. Honestly, I don't understand why the text isn't just trimmed of '{', '}', and '?' when the parse tree is constructed. You might need to check the equivalent action blocks for a lexer grammar. I don't know if the parse tree is very different.

Otherwise, I think solution is better than the others we've thought of up to now. It fixes the "p" vs "l" problem, as well as the "this." vs "$this->" vs "self." vs "this->" problem. And it doesn't result in a proliferation of if-then-else code sprinkled through the grammar itself.

I don't think we have any more non-OO targets. The Go runtime is slammed into something that resembles an OO framework. It just wan't implemented well to begin with. But, ST actions could come in handy with a new target like C.

This doesn't take care of the target-specific code in local and return clauses. But, we don't use those features in grammars-v4.

You got my vote. @parrt Please consider this PR. It goes a long way to fixing the problems in writing grammars over different targets.

@ericvergnaud
Copy link
Contributor

ericvergnaud commented Jul 5, 2023 via email

@kaby76
Copy link
Contributor

kaby76 commented Jul 5, 2023

The problem with writing the grammar as STG-format file is that it is then not an Antlr grammar. The format could not be parsed using the grammars-v4 antlr4 grammar. Existing VSCode or VSIDE extensions would not work. People would not call it an Antlr grammar. The use of the template calls would not be restricted to just the actions. Not a good solution.

You could implement this as a separate command-line tool called "antlr4-plus", processing the .g4 as a ST formatted file where ST attributes are referenced only in the actions, the proposed format for the grammar. This tool could parse, then render a normal Antlr4 grammar. VSCode and VSIDE extensions would still work. But, people would not call the grammar an "Antlr4 grammar", but an "Antlr4-plus" grammar. It would be labeled a third-party tool, and any grammars in the format would probably get a new extension, like ".g4plus".

The template group file reference is directly listed as an option in the grammar. It's an explicitly listed dependency, which is a much better thing than hiding it. If the grammar option contains options { actionTemplates='Temp.stg'; }, and folks accidentally run the Antlr4 tool on it, the Antlr tool would give a warning because the tool warns on all options it does not know. If the build uses -Werror, the build breaks which is correct, but, people would say "What is this? Is this an Antlr4 grammar?". They're then going to have to redo all their build tooling, new rules in the NPM .json, pom.xml, .csproj, etc. teverett and KvanTTT won't like adding .g4plus files to grammars-v4 because they're not exactly Antlr4 grammars.

Folks are very hesitant to use third-party tools. They won't know how to install, screw up on install. Antlr4-tools is official, and directly referenced in antlr.org, nothing further to do. Nobody trusts my Trash toolkit--years in development, used in grammars-v4, extremely useful. But, it's not listed in antlr.org, won't ever get there and only modified by me. Several other tools that are Antlr based, and they're also not listed there, like XText (although that is Antlr3-based). I'm sure there are others.

@ericvergnaud
Copy link
Contributor

@kaby76 re the template not being a valid grammar, that's a fair point.
That said I think actionTemplates should be a command line option only. Its purpose is to provide a different implementation per target. Making it a grammar option ( using options { actionTemplates='Temp.stg'; } ) pushes the responsibility of locating that template to antlr, which is wrong.
@DavidGregory084 there is a big downside to this which relates to automation. Many developers use the maven plugin to generate code at build time (rather than at dev time - which I prefer). The plugin would now need to know where to find the action templates, on a per target basis. Does your PR cater for this ?

@@ -80,6 +80,7 @@ public class Grammar implements AttributeResolver {
parserOptions.add("TokenLabelType");
parserOptions.add("tokenVocab");
parserOptions.add("language");
parserOptions.add("actionTemplates");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure we want to support this as a grammar option, it should be a cmd line option only

@@ -1178,6 +1179,10 @@ public String getLanguage() {
return getOptionString("language");
}

public String getActionTemplates() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

@kaby76
Copy link
Contributor

kaby76 commented Jul 5, 2023

Making it a grammar option ( using options { actionTemplates='Temp.stg'; } ) pushes the responsibility of locating that template to antlr, which is wrong.

Well, I could solve the explicit dependency by applying a naming standard to the .g4's and .stg's, e.g., Python3Lexer.g4/Python3Lexer.stg and Python3Parser.g4/Python3Parser.stg. I already apply a coding standard for the location of target-specific files over in grammars-v4. It took years to straighten it all out, but all target-specific files are in sub-directories Cpp/, CSharp/, Dart/, Go/, Java/, JavaScript/, PHP/, Python3/, and TypeScript/. So, these .stg's would go into each of the sub-directories. That's fine because that's where the superClass files already exist. The base classes can be easily rewritten as .stg's. I'd know which grammar requires a command-line option -actionTemplates=foobar for trgen, and could then generate a build script in whatever build system (dotnet, maven, npm, etc) to apply the command-line option.

But, why is an equivalent actionTemplates "wrong"? The Antlr tool already has a superClass option in the grammar. Why even use options { superClass=Python3LexerBase; } when we can just do antlr4 -DsuperClass=Python3LexerBase Python3Lexer.g4 and assume so because we have a naming scheme?

@kaby76
Copy link
Contributor

kaby76 commented Jul 5, 2023

there is a big downside to this which relates to automation. Many developers use the maven plugin to generate code at build time (rather than at dev time - which I prefer). The plugin would now need to know where to find the action templates, on a per target basis. Does your PR cater for this ?

All the more that it should be explicitly listed in the grammar file, and not pushed to the build tool. You won't have to change the build tool, except that the grammar now requires a new version of the Antlr tool which people do anyways.

@ericvergnaud
Copy link
Contributor

@kaby76 Sorry to disagree, but that would have to be a new entry in the pom file. I am not supportive of an approach that requires the tool to resolve the stg file path in the first place, and differently for each target.

@kaby76
Copy link
Contributor

kaby76 commented Jul 5, 2023

@kaby76 Sorry to disagree, but that would have to be a new entry in the pom file. I am not supportive of an approach that requires the tool to resolve the stg file path in the first place, and differently for each target.

First, actionTemplates is not a directory, it's a top-level file name, including path. The path can encode the target, but it's best not to do that. And, I would not have the -Dlanguage option (or hardwired option in the grammar file) affect where to find the templates. That would not be referentially transparent.

At least in grammars-v4, nothing in the .g4 would need to be changed per target. Just place the .stg's in the same directory as the .g4's. The command line to generate the target-specific recognizers would still be the same: antlr4 -actionTemplates=Python3Lexer.stg -Dlanguage=CSharp Python3Lexer.g4; antlr4 -actionTemplates=Python3Lexer.stg -Dlanguage=CSharp Python3Lexer.g4, or antlr4 -actionTemplates=Python3Lexer.stg -Dlanguage=Java Python3Lexer.g4; antlr4 -actionTemplates=Python3Lexer.stg -Dlanguage=Java Python3Lexer.g4.

The target-specific nature of the templates is laid out in the coding standard--the directory structure--after years of arguing about it in grammars-v4. But, as far as I know, the Maven plugin does not work for other targets besides Java. But, for trgen, the target specific files are overlayed over the files where the .g4's reside.

If you really want to place the .g4's and .stg's in separate directories, the Antlr tool already has a way to do that with -lib on the command-line. I think the PR should be updated to reference that path. The target-specific nature would then tied with an explicit target mentioned in the -Dlanguage and the -lib option together.

Nothing in the pom.xml would need to change whatsoever other than now adding the -actionTemplates option--which I wouldn't do, because I would always write the grammar with the option in the grammar itself, just like the superClass option. You could then group the .stg files with the .g4's all in one directory. Trgen would still work because the target-specific files are copied to where the .g4 files are, overwriting the Java-specific files.

Signed-off-by: David Gregory <2992938+DavidGregory084@users.noreply.github.com>
@DavidGregory084
Copy link
Author

DavidGregory084 commented Jul 5, 2023

That said I think actionTemplates should be a command line option only. Its purpose is to provide a different implementation per target. Making it a grammar option ( using options { actionTemplates='Temp.stg'; } ) pushes the responsibility of locating that template to antlr, which is wrong.

I agree that a target-specific option does not make a lot of sense in the grammar file - the only reason that I have done it this way is that the "language" option is already a grammar-level option and "actionTemplates" seems to belong wherever "language" is defined - happy to change it.

I expect most folks would provide "actionTemplates" as a command line option.

EDIT: I see that @kaby76 has a slightly different pattern in mind but both usage patterns would work with a grammar-level option I think?

@DavidGregory084 there is a big downside to this which relates to automation. Many developers use the maven plugin to generate code at build time (rather than at dev time - which I prefer). The plugin would now need to know where to find the action templates, on a per target basis. Does your PR cater for this ?

Not at the moment - I think that would require changes to the Maven plugin and I haven't looked at that part of the project yet.

The Gradle plugin already requires manually manipulating the command line options for any advanced use-cases, e.g. here's the invocation from my current project:

tasks.generateGrammarSource {
    // See: https://github.com/antlr/antlr4/issues/2335
    val outputDir = file("build/generated-src/antlr/main/org/mina_lang/parser")
    // the directory must exist or ANTLR bails
    doFirst { outputDir.mkdirs() }
    arguments = arguments + listOf(
        "-visitor",
        "-no-listener",
        // the lexer tokens file can't be found by the parser without this
        "-lib", outputDir.absolutePath,
        "-package", "org.mina_lang.parser")
}

Having said that, I don't really know which use cases would require providing different arguments to the same build tool?

e.g. in my case, I will probably move my grammar to a top-level directory, and symlink it into my Gradle source directories and my npm source directories. Then each build tool will invoke the ANTLR tool with different command line arguments to generate the target that's relevant for that build tool - Java for my Gradle build, TypeScript for my npm build.

assertEquals(
"State: Generate; \n" +
"error(211): " + grammarFile + ":2:14: error compiling action template: 3:3: invalid character '¢'\n" +
"error(211): " + grammarFile + ":2:14: error compiling action template: 3:0: mismatched input ' ' expecting EOF\n",
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This message is a little weird - I'm not sure why changing the action template to be multiline triggers this error message from StringTemplate

@parrt
Copy link
Member

parrt commented Jul 29, 2023

Very interesting and well thought out proposal. Away from my computer, but I am positively inclined to approve something in this nature. So it sounds like the grammar itself would mention the name of a template file, and then the -lib would indicate a directory with a template file. But what is the convention for changing the name of the template according to the language? we definitely don’t want to name the target language inside the grammar file as that defeats the purpose. Seems to me that you might simply want to have a command line option that indicates the specific template file to use, but then it means we don’t need to specify the template file name inside the grammar. But that has the negative that users will be mystified when the code generator does not compile as you point out

@ericvergnaud
Copy link
Contributor

ericvergnaud commented Jul 29, 2023 via email

@kaby76
Copy link
Contributor

kaby76 commented Jul 30, 2023

I’m all for making this a cmd line only option. We don’t went to embed any target directory logic.

If you want to declare the .stg/.g4 pairing outside the grammar file--in a bash script, makefile, Maven file, etc--that's your choice. But, please don't force the coding style on me. I would prefer to declare the actionTemplates option in the grammar because it has to be specified somewhere.

In my example, there is no mention of target in either of the .g4's: options { actionTemplate='Python3Parser.stg';} is the same for all targets. There are no forked copies of the .g4's for each target (a maintenance nightmare). There is no target-specific logic in the .g4. The templated actions are the same for all targets in the .g4, e.g., @header {<header()>}. Really, there is zero target logic in the .g4 files in my example. The target-specific choice is in the generated build script. trgen overlays the grammar with target-specific files. Alternatively, it would be fine if the target-specific .stg's could be found using the -lib option.

If the pairing of top-level .stg with the .g4 is not specified in the grammar, I'll have to devise a way to know when to add the -DactionTemplates='...' option to the generated build script. Some grammars in grammars-v4 have multiple .g4's, e.g., csharp. If the .stg/.g4 pairing is not declared in the .g4, it would need to be declared in the desc.xml or computed by parsing the .g4's, .stg's. Driver code is not part of the grammar we share in grammars-v4.

@DavidGregory084
Copy link
Author

So it sounds like the grammar itself would mention the name of a template file, and then the -lib would indicate a directory with a template file.

Yes, that's right, actionTemplates is currently a grammar-level option, but note that grammar-level options can always be provided to the ANTLR tool via the command line, i.e. antlr4 -DactionTemplates=Actions.stg.

It seems that @ericvergnaud is strongly against keeping this as a grammar-level option and @kaby76 is strongly for it, so I hope you can be a tie breaker. 😅

I will note though that I don't see any harm in leaving this at the grammar level if it suits use cases whereby the relative folder structure or -lib argument is used to determine where a fixed template file name mentioned in the grammar file is picked up from.

@ericvergnaud
Copy link
Contributor

ericvergnaud commented Jul 31, 2023 via email

@kaby76
Copy link
Contributor

kaby76 commented Jul 31, 2023

Since -lib describes an input directory, I’m fine with the tool trying to locate the template file in that directory.But I will strongly oppose any logic where the template file name needs to be inferred from the target.

I agree!

We need a change for finding the .stg along the -lib path.

07/31-12:01:45 ~/spython3-copy
$ grep actionTemplates Python3Lexer.g4
    actionTemplates='Python3Lexer.stg';
07/31-12:01:58 ~/spython3-copy
$ antlr4 -Dlanguage=CSharp -lib CSharp Python3Lexer.g4
error(206):  cannot find action templates file Python3Lexer.stg given for Python3Lexer
07/31-12:02:05 ~/spython3-copy
$ pushd
~/spython3-cmd-line ~/spython3-copy
07/31-12:02:07 ~/spython3-cmd-line
$ grep actionTemplates Python3Lexer.g4
07/31-12:02:21 ~/spython3-cmd-line
$ antlr4 -Dlanguage=CSharp -lib CSharp -DactionTemplates=Python3Lexer.stg Python3Lexer.g4
error(206):  cannot find action templates file Python3Lexer.stg given for Python3Lexer

@DavidGregory084
Copy link
Author

@kaby76 that change should already have been implemented as of 84866cc - are you using an earlier commit or is this something that I need to look into?

@ericvergnaud
Copy link
Contributor

@parrt this is not ready to merge, but I guess we have your green light to complete the work ?

@kaby76
Copy link
Contributor

kaby76 commented Aug 30, 2023

@parrt this is not ready to merge, but I guess we have your green light to complete the work ?

I am still waiting for the -lib option fix. That part does not work. I never received a reply from @DavidGregory084 addressing the bug that I found. If he doesn't fix the problem, I would still recommend the merge because it would still be good enough to redo the grammars in grammars-v4. I can fix the bug in a follow-up PR.

@ericvergnaud
Copy link
Contributor

ericvergnaud commented Aug 30, 2023

@parrt this is not ready to merge, but I guess we have your green light to complete the work ?

I am still waiting for the -lib option fix. That part does not work. I never received a reply from @DavidGregory084 addressing the bug that I found. If he doesn't fix the problem, I would still recommend the merge because it would still be good enough to redo the grammars in grammars-v4. I can fix the bug in a follow-up PR.

mmm... I think we would need to add support for this in the maven and the IntelliJ plugins at minimal, and antlr-lab

@DavidGregory084
Copy link
Author

I am still waiting for the -lib option fix. That part does not work. I never received a reply from @DavidGregory084 addressing the bug that I found. If he doesn't fix the problem, I would still recommend the merge because it would still be good enough to redo the grammars in grammars-v4. I can fix the bug in a follow-up PR.

@kaby76 I asked you a question about this in my last comment:

@kaby76 that change should already have been implemented as of 84866cc - are you using an earlier commit or is this something that I need to look into?

Which revision are you using?

@kaby76
Copy link
Contributor

kaby76 commented Aug 30, 2023

Which revision are you using?

I was using the latest. I always use the latest. But of course, mistakes sometimes happen. But not this time.

I created a repo (https://github.com/kaby76/do-over-and-over.git) with a script (https://github.com/kaby76/do-over-and-over/blob/main/do-over-and-over.sh) that clones your repo, does a build and run for three different versions.

  • For commit 84866cc, which you assert fixes the -lib problem--it does not as shown here, "error(206):....".
  • For tip of branch action-templates. The problem still occurs, as shown here, "error(206):....".
  • For tip of branch action-templates, patching the code using fix.txt at this point in the output. My patch works, as shown here with no errors.

@DavidGregory084
Copy link
Author

Cool thanks for confirming - I'll take a look as soon as I can!

@ericvergnaud
Copy link
Contributor

@DavidGregory084 hi there. It's been a while. Any chance you will progress this ?

@ericvergnaud
Copy link
Contributor

@DavidGregory084 we'd like to include this feature in ANTLR5.
Would you be able to migrate this PR accordingly ?

@DavidGregory084
Copy link
Author

Apologies @ericvergnaud I have not had a great deal of time for open source over the past few months. What are the timelines like for ANTLR5? I will try to make some time for this next week!

@ericvergnaud
Copy link
Contributor

The overall timeline is flexible, but this feature will rapidly become critical due to the new architecture.

… unit test

Signed-off-by: David Gregory <2992938+DavidGregory084@users.noreply.github.com>
@DavidGregory084
Copy link
Author

DavidGregory084 commented Apr 15, 2024

@ericvergnaud apologies for the delay - @kaby76 was absolutely right about the cause of the issue, but I wanted to write a unit test to exercise the -lib directory code and never seemed to find the time.

I have incorporated @kaby76's fix now and added a test.

Next I will migrate the PR to the antlr5 repo.

Will you still accept this PR on antlr4 as well, or should we close it here?

@ericvergnaud
Copy link
Contributor

@DavidGregory084 I wouldn't close it. Once it is successfully implemented in antlr5, its usefulness might be proven well enough for Ter to accept it.

@DavidGregory084
Copy link
Author

Opened a PR for the port at antlr/antlr5#51

@kaby76
Copy link
Contributor

kaby76 commented Apr 16, 2024

Note, templates could be applied elsewhere in the grammar e.g., UP_TO_THREE_DOTS: <repeat(3, '.')> ;. Not a great example, but just to illustrate a point. #2577

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants