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

Making compatibility with actions and predicates in Go better #3174

Merged
merged 1 commit into from
May 21, 2021

Conversation

mitar
Copy link
Contributor

@mitar mitar commented May 4, 2021

This change is a simple workaround which makes grammars which use actions and predicates calling something on this work also in Go.

See antlr/grammars-v4#2177 for a bit of background. This change would allow compiling JavaScript grammar with antlr with a bit less manually patching. (There are few more steps to do to get it working.)

@mitar mitar force-pushed the go-compatibility branch from 878d9eb to 475892c Compare May 4, 2021 05:37
@@ -325,6 +325,9 @@ func (l *<lexer.name>) Sempred(localctx antlr.RuleContext, ruleIndex, predIndex
*/
RuleActionFunction(r, actions) ::= <<
func (l *<lexer.name>) <r.name; format="cap">_Action(localctx <if(r.factory.grammar.lexer)>antlr.RuleContext<else>*<r.ctxType><endif>, actionIndex int) {
this := l
_ = this
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Assignment to _ is needed so that Go does not complain if this does not end up used at the end.

@mitar
Copy link
Contributor Author

mitar commented May 11, 2021

@Joakker, @prnvbn, if you could maybe review this PR?

@mitar mitar closed this May 11, 2021
@mitar mitar reopened this May 11, 2021
@mitar mitar force-pushed the go-compatibility branch from 2a77767 to 8117243 Compare May 11, 2021 06:38
@Joakker
Copy link
Contributor

Joakker commented May 12, 2021

Looks good to me 👍🏽

@mitar
Copy link
Contributor Author

mitar commented May 12, 2021

Not sure why CI is failing. That should be fixed on master, no?

@ericvergnaud
Copy link
Contributor

Not sure why CI is failing. That should be fixed on master, no?

Master is failing too on AppVeyor:
00:01:58] Running org.antlr.v4.test.runtime.go.TestCompositeLexers
[00:02:03] Tests run: 2, Failures: 2, Errors: 0, Skipped: 0, Time elapsed: 5.118 sec <<< FAILURE! - in org.antlr.v4.test.runtime.go.TestCompositeLexers
[00:02:03] testOneGo:LexerDelegatorRuleOverridesDelegate Time elapsed: 1.369 sec <<< FAILURE!
[00:02:03] junit.framework.AssertionFailedError:
[00:02:03] [Go:LexerDelegatorRuleOverridesDelegate] Parse output is incorrect: expectedOutput:<M.A
[00:02:03] [@0,0:1='ab',<1>,1:0]
[00:02:03] [@1,2:1='',<-1>,1:2]
[00:02:03] >; actualOutput:<>; expectedParseErrors:<>; actualParseErrors:<parser\m_lexer.go:9:2: no required module provides package github.com/antlr/antlr4/runtime/Go/antlr: go.mod file not found in current directory or any parent directory; see 'go help modules'
[00:02:03] >; expectedToolErrors:<>; actualToolErrors:<>.

-> thousands of the above

@mitar
Copy link
Contributor Author

mitar commented May 12, 2021

I thought that was fixed with #3176?

@prnvbn
Copy link

prnvbn commented May 12, 2021

Try running go mod tidy and see if that makes a difference

@Joakker
Copy link
Contributor

Joakker commented May 12, 2021

Not sure why CI is failing. That should be fixed on master, no?

Master is failing too on AppVeyor:
00:01:58] Running org.antlr.v4.test.runtime.go.TestCompositeLexers
[00:02:03] Tests run: 2, Failures: 2, Errors: 0, Skipped: 0, Time elapsed: 5.118 sec <<< FAILURE! - in org.antlr.v4.test.runtime.go.TestCompositeLexers
[00:02:03] testOneGo:LexerDelegatorRuleOverridesDelegate Time elapsed: 1.369 sec <<< FAILURE!
[00:02:03] junit.framework.AssertionFailedError:
[00:02:03] [Go:LexerDelegatorRuleOverridesDelegate] Parse output is incorrect: expectedOutput:<M.A
[00:02:03] [@0,0:1='ab',<1>,1:0]
[00:02:03] [@1,2:1='',<-1>,1:2]
[00:02:03] >; actualOutput:<>; expectedParseErrors:<>; actualParseErrors:<parser\m_lexer.go:9:2: no required module provides package github.com/antlr/antlr4/runtime/Go/antlr: go.mod file not found in current directory or any parent directory; see 'go help modules'
[00:02:03] >; expectedToolErrors:<>; actualToolErrors:<>.

-> thousands of the above

I think, and this might be wrong, but I think that the problem is that the testing environment doesn't set up the main module, i.e., the package of the tests isn't a part of a module, so trying to use modules makes the go tool complain. I think I overlooked that part in my own PR

@mitar
Copy link
Contributor Author

mitar commented May 12, 2021

I think I overlooked that part in my own PR

Care to then make a new PR fixing Go tests on master and then I can rebase here?

@Joakker
Copy link
Contributor

Joakker commented May 21, 2021

@mitar There you go, #3185 has been merged and now the go tests are fixed 👍🏽

@mitar
Copy link
Contributor Author

mitar commented May 21, 2021

Awesome! Thanks!

@mitar mitar force-pushed the go-compatibility branch from 8117243 to b915242 Compare May 21, 2021 15:45
@mitar
Copy link
Contributor Author

mitar commented May 21, 2021

I just rebased the branch.

@parrt parrt added the actions label May 21, 2021
@parrt
Copy link
Member

parrt commented May 21, 2021

Running tests now...

@parrt parrt merged commit c5ad59b into antlr:master May 21, 2021
@parrt
Copy link
Member

parrt commented May 21, 2021

Yay! everything passed :)

@mitar mitar deleted the go-compatibility branch May 21, 2021 18:46
@mitar
Copy link
Contributor Author

mitar commented May 21, 2021

Thanks!

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

Successfully merging this pull request may close these issues.

5 participants