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

Assert in ParserRuleContext.java makes copyFrom unusable #57

Open
fedorusov opened this issue Oct 22, 2019 · 6 comments
Open

Assert in ParserRuleContext.java makes copyFrom unusable #57

fedorusov opened this issue Oct 22, 2019 · 6 comments

Comments

@fedorusov
Copy link

assert t.getParent() == null || t.getParent() == this;

If you call

context1.copyFrom(context2)

you necessarily call addAnyChild which have this assert, so it fails.
Vanilla antlr doesn't have this, why do you?

@fedorusov fedorusov changed the title Assert makes copyFrom unusable Assert in ParserRuleContext.java makes copyFrom unusable Oct 22, 2019
@sharwell
Copy link
Member

sharwell commented Oct 22, 2019

Can you provide an example showing how this code path is hit?

Vanilla antlr doesn't have this, why do you?

ANTLR made some breaking API changes during earlier releases, which are not allowed in this repository. It's possible this assertion was incorrectly translated during a merge, but I'm surprised I haven't heard about it sooner since this code is more than 2 years old.

@fedorusov
Copy link
Author

fedorusov commented Oct 22, 2019

Before, I used the old 4.5.3 version (without assertion) which occurred to have some performance problems in some cases, which ones are solved in the latest one. You don't need to have any concrete examples here, just try to copy any context and experience assertion error here because if context2 has children you call addAnyChild to context1, so t.getParent() != this.

@sharwell
Copy link
Member

You don't need to have any concrete examples here, just try to copy any context ...

The context passed to copyFrom is not expected to have children. Can you clarify how it would end up with children prior to the copyFrom call?

@fedorusov
Copy link
Author

with 4.5.3 I could have like

ctx1.copyFrom(ctx2);
for (child: ctx2.childrens)
  ctx1.addChild(child)

(I know parents are not reassigned here, but it's ok for me.)
where copyFrom was

	public void copyFrom(ParserRuleContext ctx) {
		this.parent = ctx.parent;
		this.invokingState = ctx.invokingState;
		this.start = ctx.start;
		this.stop = ctx.stop;
	}

and addAnyChild didn't have assertion.
But now I should have my own copyFrom and addAnyChild like it was before and update it every time when I update antlr to have the same code.

@sharwell
Copy link
Member

sharwell commented Dec 2, 2019

@fedorusov Is it OK if I leave this one open? I can't promise it will change but I'd like to think about ways to improve the validation condition.

@sharwell sharwell reopened this Dec 2, 2019
@fedorusov
Copy link
Author

Sure

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

No branches or pull requests

2 participants